Last Comment Bug 328778 - nsPluginHostImpl::UserAgent returns NULL for user agent longer than 128 characters
: nsPluginHostImpl::UserAgent returns NULL for user agent longer than 128 chara...
Status: RESOLVED FIXED
[needs test?]
: crash, verified1.9.0.4
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Kathleen Brade
:
Mentors:
: 390290 416286 450919 451944 455627 460879 (view as bug list)
Depends on: 448646
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-27 13:36 PST by Christian Persch (GNOME) (away; not receiving bug mail)
Modified: 2008-10-26 16:50 PDT (History)
21 users (show)
samuel.sidler+old: blocking1.9.0.2-
samuel.sidler+old: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (return truncated UA string) (1.44 KB, patch)
2008-06-06 07:32 PDT, Kathleen Brade
jst: review+
jst: superreview+
dveditz: approval1.9.0.4+
Details | Diff | Review

Description Christian Persch (GNOME) (away; not receiving bug mail) 2006-02-27 13:36:13 PST
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
Comment 1 Christopher Aillon (sabbatical, not receiving bugmail) 2007-03-15 14:54:44 PDT
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.
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-01 11:02:49 PDT
*** Bug 390290 has been marked as a duplicate of this bug. ***
Comment 3 Kathleen Brade 2008-06-05 15:30:12 PDT
This affects all platforms (I saw it on Windows).
Comment 4 Kathleen Brade 2008-06-06 07:32:44 PDT
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.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-09 17:44:23 PDT
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.
Comment 6 Kathleen Brade 2008-06-19 13:39:00 PDT
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 7 Johnny Stenback (:jst, jst@mozilla.com) 2008-07-17 17:53:39 PDT
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Fair enough.
Comment 8 Kathleen Brade 2008-07-18 11:36:03 PDT
Thank you!

Seeking approval to land for Firefox 2.0.0.x and 3.0.x since this fixes plugin crashes.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-07-20 05:30:07 PDT
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.
Comment 10 Kathleen Brade 2008-07-20 07:54:25 PDT
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.
Comment 11 Reed Loden [:reed] (use needinfo?) 2008-07-20 10:32:24 PDT
(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.
Comment 12 Samuel Sidler (old account; do not CC) 2008-07-21 00:08:18 PDT
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...)
Comment 13 Samuel Sidler (old account; do not CC) 2008-08-03 13:04:55 PDT
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.
Comment 14 Kathleen Brade 2008-08-03 13:52:40 PDT
Sorry I haven't been able to land yet; waiting for my Mercurial account...
Comment 15 Daniel Veditz [:dveditz] 2008-08-04 11:51:10 PDT
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.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2008-08-04 14:37:37 PDT
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
Comment 17 Samuel Sidler (old account; do not CC) 2008-08-04 16:08:16 PDT
We won't block on this for 1.9.0.2 either, but will gladly take a baked patch with a test.
Comment 18 timeless 2008-09-03 06:07:48 PDT
*** Bug 451944 has been marked as a duplicate of this bug. ***
Comment 19 timeless 2008-09-03 06:08:20 PDT
*** Bug 416286 has been marked as a duplicate of this bug. ***
Comment 20 Manfred H. Winter 2008-09-03 08:33:46 PDT
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.
Comment 21 Kathleen Brade 2008-09-03 19:00:05 PDT
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.
Comment 22 Daniel Veditz [:dveditz] 2008-09-05 11:22:03 PDT
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?
Comment 23 timeless 2008-09-16 19:34:24 PDT
*** Bug 455627 has been marked as a duplicate of this bug. ***
Comment 24 timeless 2008-09-16 19:35:55 PDT
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.
Comment 25 Samuel Sidler (old account; do not CC) 2008-10-15 09:30:57 PDT
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.
Comment 26 Kathleen Brade 2008-10-16 06:58:25 PDT
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 27 Daniel Veditz [:dveditz] 2008-10-16 17:25:12 PDT
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 timeless 2008-10-18 19:38:53 PDT
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 29 Daniel Veditz [:dveditz] 2008-10-22 15:30:46 PDT
Comment on attachment 324046 [details] [diff] [review]
proposed fix (return truncated UA string)

Approved for 1.9.0.4, a=dveditz for release-drivers
Comment 30 Kathleen Brade 2008-10-23 06:13:59 PDT
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
Comment 31 Al Billings [:abillings] 2008-10-24 12:58:36 PDT
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.
Comment 32 Matthias Versen [:Matti] 2008-10-26 16:44:34 PDT
*** Bug 450919 has been marked as a duplicate of this bug. ***
Comment 33 Matthias Versen [:Matti] 2008-10-26 16:50:17 PDT
*** Bug 460879 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.