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
:
: Benjamin Smedberg [:bsmedberg]
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 | Splinter Review

Description User image 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 User image 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 User image 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 User image Kathleen Brade 2008-06-05 15:30:12 PDT
This affects all platforms (I saw it on Windows).
Comment 4 User image 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 User image 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 User image 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 User image 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 User image 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 User image :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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image timeless 2008-09-03 06:07:48 PDT
*** Bug 451944 has been marked as a duplicate of this bug. ***
Comment 19 User image timeless 2008-09-03 06:08:20 PDT
*** Bug 416286 has been marked as a duplicate of this bug. ***
Comment 20 User image 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 User image 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 User image 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 User image timeless 2008-09-16 19:34:24 PDT
*** Bug 455627 has been marked as a duplicate of this bug. ***
Comment 24 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Matthias Versen [:Matti] 2008-10-26 16:44:34 PDT
*** Bug 450919 has been marked as a duplicate of this bug. ***
Comment 33 User image 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.