58095 - we use use NETLIB for user-agent string in nsPluginHostImpl.cpp

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Peter Lubczynski]

Looking through nsPluginHostImpl.cpp, I noticed that in UserAgent() we are using
a hard-coded agent string ["Mozilla/5.0 [en] (Windows;I)"] instead of getting
the right one for the correct version/platform/localization.

It looks like a simple fix because the code is already in place. In fact, the
#DEFINE is commented out at the top and it looks like Gagan was working on this
at one time.

Comment 1

[Gagan]

I may have then, but won't now :)
->av

Comment 2

[Peter Lubczynski]

I think this is something which needs to get fixed for RTM as we will report 
the wrong UserAgent in the plugin API. Adding that keyword amd cc:ing Eric.

http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginHostImpl.c
pp#1568

Comment 3

[ekrock's old account (dead)]

Andrei will investigate. We need to know (1) where this incorrect UA string is
being reported to (the plug-in? the server, directly from the browser? the
plug-in, and then passed on to the server later?) and what the risks are. It
could be very dangerous for our browser to be reporting varying UA strings from
time to time. Need to leave on RTM radar until we have severity determined.
cc:ing valeski in case Andrei needs advice about how to get the UA string correctly.

Comment 4

[Judson Valeski]

Attachment: fix for underlying impl (will cause leak under current usage)

Comment 5

[Judson Valeski]

oh mama! I don't know the plugin code, but I'm guessing that for all 4.x style
plugins that want to get our user agent, we will hand them "Mozilla/5.0 [en]
(Windows;I)", regardless of platform (that string is _hardcoded_).

Someone who knows plugins better than I do needs to verify what plugins will be
affected by this. It's possible that all new style plugins will suffer this fate.

I've attatched a patch that retrieves the correct UA string. However, the
hardcoded string was static and I generate new memory for the returned stream.
The caller of this function
(http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/ns4xPlugin.cpp#1070)
is not expecting new memory. Subsequently, as is, my patch will cause a leak of
the string. ns4xPlugin should keep track of this memory and delete it when it's
done.

IMO, this is very serious, and depending on which plugins are affected by it,
I'd stop ship for it.

Comment 6

[ekrock's old account (dead)]

Nick, Liz, Sean, Patrcik -- Severity assessment on this issue please? Any idea
how this might affect your plug-ins, if at all?

Comment 7

[sean echevarria]

incorrect platform and localization in userAgent string will not affect Beatnik.

Comment 8

[av (gone)]

Eric, this string is only reported to the plugin when it asks for it via the 
Plugin API. I don't think that plugin is interested in platform when it queries 
for UA, the browser version is probably more important. The way it is now, we 
will tell plugins that we are Mozilla not 4.x and this should be enough for 
old-school plugins for now. This can be important later when we fix bugs or say 
change API (which is unlikely) and a plugin could want to distinguish between 
different versions of Mozilla. Localization is another issue which could be 
important, but here we have to ask plugin makers.

Comment 9

[av (gone)]

Judson, to your patch. Why not to allocate a static string of say 128 byte size, 
copy UA string there, return it to the caller and then free the memory?

Comment 10

[av (gone)]

Attachment: modified patch with no leak

Comment 11

[ekrock's old account (dead)]

Andrei, please get review and super-review ASAP for this patch and then mark it
rtm+ for consideration as a patch for limbo acceptance. Thanks!

PDT: The presence of a hardcoded user-agent string within the Plug-in API (that
doesn't match the actual user agent string) was completely overlooked. This
inconsistency *might* trigger problems that we haven't anticipated. The fix is
extremely low risk (it simply copies and returns the correct UA string) so let's
please take this along with other limbo fixes if we respin to reduce the risk of
unforeseen problems for our plug-in partners in upgrading and
cross-browser/cross-point-release support.

Comment 12

[Judson Valeski]

r=valeski after the following two items are addressed:
1. rename "retString" to something that differs more from "retstring"
2. we should zero out the "retString" before copying in the new string so we
don't wind up w/ overlapping memory.

Comment 13

[av (gone)]

Attachment: corrected patch

Comment 14

[av (gone)]

Attachment: There was a typo in a previous patch, please ignore

Comment 15

[av (gone)]

I talked to Judson and we agreed that #2 in his comment is not actually needed.

Comment 16

[av (gone)]

Attachment: Took away correction for #2 comment at 2000-10-30 15:42

Comment 17

[Judson Valeski]

r=valeski on the 17:04 patch

Comment 18

[Chris Waterson]

Honestly, at this point I think fixing the bug is much more risky than not
fixing it. Here's why. You're trading *one week* of QA for the last "n" months
where we've been delivering this bogus UA string to plugins. I would have
thought that any problems that would've surfaced would have been reported by
now, right? If we *change* the UA string we're delivering, isn't it possible
that some plugins that were merrily expecting the bogo-string could start to
freak out?

Anyway, the code changes themeselves look fine, so sr=waterson: this should
*definitely* go onto the trunk. However, I'd think long and hard before putting
this on the branch.

Comment 19

[selmer (gone)]

rtm-, not seeing the benefit being worth any risk at this point.

Comment 20

[av (gone)]

Checked in into the trunk. It will not go to the branch. Marking fixed.

Comment 21

[av (gone)]

Attachment: Tester plugin for verification purposes.

Comment 22

[av (gone)]

Attachment: Test case for verification purpose

Comment 23

[av (gone)]

To verify the fix please place the attached plugin in the Plugins folder, 
restart and load the attached html source.

Comment 24

[av (gone)]

*** Bug 61454 has been marked as a duplicate of this bug. ***