nsPluginHostImpl::UserAgent returns NULL for user agent longer than 128 characters

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Assigned: Kathleen Brade)

Tracking

({crash, verified1.9.0.4})

Trunk
crash, verified1.9.0.4
Points:
---
Bug Flags:
blocking1.9.0.2 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs test?])

Attachments

(1 attachment)

Which some plugins, e.g. java (cf. bug 83376 comment 154) don't like. This is related to bug 83376, but I'd like to discuss this separately since that bug has become too cluttered.
I've run into this bug myself with the Epiphany RPMs for Mandriva2006 linux distribution (has been fixed by them by shortening their UA string additions).
This is esp. concerning since adding extra UA portions is now very easy with the eneral.useragent.extra.* prefs from bug 274928; the current limit is 128 characters [http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#2778] which is already easily attained with one or two extras.

I therefore propose to eliminate the 128 characters limit from nsPluginHostImpl::mUserAgent.

Now I can see a few problems arise:
- currently user agent is stored in a static char[128]. While NPN_UserAgent returns a const char*, some plugins might have relied on this string being static (i.e. outliving themselves), and stored it internally
- some plugins may rely on the 128 characters limit. Worse, two of mozilla's sample plugins actually do this:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/samples/4x-scriptable/plugin.h#59 + http://lxr.mozilla.org/seamonkey/source/modules/plugin/samples/4x-scriptable/plugin.cpp#63
and http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/sdk/samples/scriptable/mac/plugin.h#61 + http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/sdk/samples/scriptable/mac/plugin.cpp#299
For what it's worth, it appears that this limit was imposed as an implementation detail after npapi had been around for a while.  See bug 58095 for the patch that introduced it.  It's also not advertised in the api itself that there's a limit, so it's unlikely that plugins will do bad things with >= 128 characters.  However, there's an easy way to test.  Raise the limit and test as many plugins as possible.
Duplicate of this bug: 390290
(Assignee)

Comment 3

9 years ago
This affects all platforms (I saw it on Windows).
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 4

9 years ago
Created attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Given the fact that some plugins may assume the UA string returned will be < 128 characters, I think the best solution is to return a truncated UA string.  My proposed fix truncates at the right-most space character that is within the length we have to work with.
Assignee: nobody → brade
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #324046 - Flags: superreview?(jst)
I think I'd rather just remove the limit and fix our internal retarded samples to not depend on that. I wonder if Safari has a similar limit too? If not, I'd say we just remove the limit and fix the samples. And now would be a good time to do that, get it out there and see if plugins start crashing left and right or not.
(Assignee)

Comment 6

9 years ago
I propose that we land the current patch (assuming it passes reviews) since it will fix the Flash plugin crash.  Then we can file a new bug to remove the limit and adjust the sample plugins.

Right now, I don't have the resources to take on the work needed to create a patch for the sample plugins.
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Fair enough.
Attachment #324046 - Flags: superreview?(jst)
Attachment #324046 - Flags: superreview+
Attachment #324046 - Flags: review+

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 8

9 years ago
Thank you!

Seeking approval to land for Firefox 2.0.0.x and 3.0.x since this fixes plugin crashes.
Flags: blocking1.9.0.2?
Flags: blocking1.8.0.15?
blocking1.8.0.15 is for the (officially) discontinued Firefox 1.5.0.x releases. You want blocking1.8.1.17 for the 2.0.0.x branch.
(Assignee)

Comment 10

9 years ago
Thanks, I must have misread the tooltip.

Requesting approval to land for Firefox 2.0.0.x and 3.0.x since this fixes plugin crashes.
Flags: blocking1.8.0.15? → blocking1.8.1.17?
Attachment #324046 - Flags: approval1.9.0.2?
Attachment #324046 - Flags: approval1.8.1.17?
Attachment #324046 - Flags: approval1.8.0.15?
(In reply to comment #10)
> Requesting approval to land for Firefox 2.0.0.x and 3.0.x since this fixes
> plugin crashes.

You need to set the approval? flags on the patch. I went ahead and did that for you.
Can we get an automated test for this? We also need this landed on mozilla-central before approving for either branch.

(I know, I know, it's only been a few hours since the bug was last touched...)
Flags: in-testsuite?
Whiteboard: [needs to land on mozilla-central before 1.9 approval]
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Please re-request approval after this lands and bakes on mozilla-central.
Attachment #324046 - Flags: approval1.9.0.2?
Attachment #324046 - Flags: approval1.8.1.17?
Attachment #324046 - Flags: approval1.8.0.15?
(Assignee)

Comment 14

9 years ago
Sorry I haven't been able to land yet; waiting for my Mercurial account...
Depends on: 448646
This bug doesn't fit the 1.8 "blocking" criteria (not security, not recent regression, not topcrash), but if this gets into the trunk and has a testcase QA can use to verify we could approve a branch patch.
Flags: blocking1.8.1.17?
Kathy, I just committed this for you, so this is fixed now. Thanks for the fix!

http://hg.mozilla.org/mozilla-central/index.cgi/rev/3884f9437753
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
We won't block on this for 1.9.0.2 either, but will gladly take a baked patch with a test.
Flags: blocking1.9.0.2? → blocking1.9.0.2-
Whiteboard: [needs to land on mozilla-central before 1.9 approval] → [needs test?]
Keywords: checkin-needed

Updated

9 years ago
Duplicate of this bug: 451944

Updated

9 years ago
Duplicate of this bug: 416286

Comment 20

9 years ago
Is this bug only fixed on trunk?
It still crashes PDF plugin on branch (Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3pre) Gecko/2008090306 GranParadiso/3.0.3pre) as I reported in bug 451944.

Updated

9 years ago
Attachment #324046 - Flags: approval1.9.0.3?
Attachment #324046 - Flags: approval1.9.0.2?
(Assignee)

Comment 21

9 years ago
Currently, this bug is fixed only on the trunk (FF 3.1.*).
I would love to check in the patch to the 3.0 tree but it lacks automated tests.

This patch fixes crashes within plugins but I'm not sure if or how those crashes would be reflected in topcrash data since the stack may be different depending on which plugin crashes.
Attachment #324046 - Flags: approval1.9.0.3?
Attachment #324046 - Flags: approval1.9.0.2?
Attachment #324046 - Flags: approval1.9.0.2-
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Too late for 1.9.0.2, and still no test so no point in sticking on the 1.9.0.3 list yet.

Doesn't mozilla-central also require tests?

Updated

9 years ago
Duplicate of this bug: 455627

Comment 24

9 years ago
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

dveditz: this isn't reasonble, we are going to help out roboform but not acrobat plugin? this patch has baked.
Attachment #324046 - Flags: approval1.9.0.3?
Attachment #324046 - Flags: approval1.9.0.2?
Attachment #324046 - Flags: approval1.9.0.2-
Attachment #324046 - Flags: approval1.9.0.2?
timeless: This bug really needs a test before we're willing to take it on branch. An automated test would be best, but a manual test that QA can run would likely be sufficient for us to look at this.
Keywords: crash
(Assignee)

Comment 26

9 years ago
I'm sorry; I thought I had included this in the bug.  Here is a manual test case for Windows XP (Firefox 3.0.*):

1) Make sure you have Adobe Reader 8 installed (I have 8.1.2) and that it is configured so that the plugin is active in Firefox (show PDF files in browser).

2) Use about:config to add this new string preference:
     name:   general.useragent.extra.bug328778
     value:  1234567890123456789012345678901234567890
  (The value needs to be long enough so the UA string is more than 128 chars.)

3) Load any PDF file in Firefox, such as
     http://www.mozilla.org/foundation/donate_form.pdf

4) Firefox will crash.
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Couldn't this whole chunk have been a single call PL_strncpyz(resultString, uaString.get(), NS_RETURN_UASTRING_SIZE); ? I guess that wouldn't truncate on a space, not as nice but do we really care?

Comment 28

9 years ago
personally, i'd rather keep braces if there's an else clause that has one.

the code could possibly be simpler w/ strrchr, but i like the space condition.
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #324046 - Flags: approval1.9.0.4? → approval1.9.0.4+
(Assignee)

Comment 30

9 years ago
This patch has now landed in CVS (for Firefox 3.0.x).
  modules/plugin/base/src/nsPluginHostImpl.cpp
  new revision: 1.609; previous revision: 1.608
Keywords: fixed1.9.0.4
Verified for 1.9.0.4 using the testcase in comment 26 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102405 GranParadiso/3.0.4pre.
Keywords: fixed1.9.0.4 → verified1.9.0.4
Duplicate of this bug: 450919
Duplicate of this bug: 460879
You need to log in before you can comment on or make changes to this bug.