Closed Bug 611910 Opened 12 years ago Closed 12 years ago
Only pass URL schemes to NPRuntime Java plugins that Java can understand
10.51 KB, patch
|Details | Diff | Splinter Review|
5.00 KB, patch
|Details | Diff | Splinter Review|
11.16 KB, patch
|Details | Diff | Splinter Review|
5.77 KB, patch
|Details | Diff | Splinter Review|
As promised in comment #0, my patch changes the NPN_GetProperty() method to only return a document.URL or document.documentURI property that Java will understand. If the "original" document.URL or document.documentURI property isn't a proper URL, or if its scheme isn't in the small lists of schemes/protocols that Java understands, it returns a "fake" URL with the "http" scheme and a random hostname ending in an invalid TLD (currently ".xyz"). The idea is to return a URL that won't make the URL(String) constructor choke, but which also won't cause any usable network privileges to be granted.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
At some point I'll post a tryserver build made with my patch from comment #1 plus some debug logging.
Since bug 589041 only causes Java Plugin2 to crash, this bug isn't really security sensitive. But I suspect it should be kept hidden until bug 589041 is no longer restricted.
If we can't share the code between here and bug 611897 (why not?), then we at least need comments on both chunks of code pointing to each other so bugs will get fixed in both of they're discovered.
(In response to comment #4) I assume you're talking about the MakeRandomString() method and associated code. Tell me where you think it should go, so that it can be shared.
I'll add a MakeRandomInvalidURL() to nsNetUtil.h.
Comment on attachment 490310 [details] [diff] [review] Fix I need to make some changes to this patch, which I'll do next week.
We need this on branch, too, to fix bug 589041. I think it also fixes bug 594443 which should have the same underlying cause.
I've followed Boris' suggestion to move part of my patch to nsNetUtil.h, where it can be shared (notably by my patch for bug 611897). I've asked Boris to review that part of both patches at bug 611897. Josh, please review the rest of the patch (the NPRuntime part).
Comment on attachment 490933 [details] [diff] [review] Fix rev1 (fix various problems) I've already found one mistake: I need to add the following line to nsNetUtil.h. Windows builds fail without it: #include <stdlib.h>
This follows a suggestion made by Boris in bug 611897 comment #16.
This patch follows Boris' suggestions from bug 611897.
Josh, please do your review as soon as possible. There's a branch code freeze tonight, and this bug is supposed block the next releases on both branches. I actually doubt that it should block -- this bug isn't a security hole for Java Plugin2 (just a crash bug). But if there's to be any chance for this bug to be fixed on the branches today, you need to do your review right away.
This bug isn't going to get fixed before tonight's freeze -- we've run out of time.
Comment on attachment 491752 [details] [diff] [review] Fix rev5 (update nsNetUtil.h with changes from final patch for bug 611897) >+ NPUTF8* propertyName = _utf8fromidentifier(property); >+ if (!propertyName) >+ return true; >+ bool notURL = >+ (PL_strcasecmp(propertyName, "URL") && >+ PL_strcasecmp(propertyName, "documentURI")); >+ nsMemory::Free(propertyName); In case '_memfree' uses a different free memory function in the future you should probably just use '_memfree' instead of 'nsMemory::Free'. That way it will always match the 'utf8fromidentifier' allocator. >diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.h b/modules/plugin/base/src/nsNPAPIPluginInstance.h >--- a/modules/plugin/base/src/nsNPAPIPluginInstance.h >+++ b/modules/plugin/base/src/nsNPAPIPluginInstance.h ... > public: > // True while creating the plugin, or calling NPP_SetWindow() on it. > PRPackedBool mInPluginInitCall; > >+ PRPackedBool mIsJavaPlugin; I don't like duplicating data like this, it leads to sync issues. If you want to know if an instance is a Java instance get it's plugin (nsNPAPIPluginInstance::GetPlugin) and then get the tag for the plugin (nsPluginHost::TagForPlugin). That has an "mIsJavaPlugin" variable.
Attachment #491752 - Flags: review?(joshmoz) → review-
Comment on attachment 491858 [details] [diff] [review] Fix rev6 (follow Josh's suggestions) >+ nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata; >+ nsNPAPIPlugin* plugin = inst->GetPlugin(); I realize "ndata" should never be null here but you may want to consider a null check anyway, just to be safe.
Attachment #491858 - Flags: review?(joshmoz) → review+
> I realize "ndata" should never be null here but you may want to consider a null > check anyway, just to be safe. OK, I'll do that when I land the patch. New patches coming up for the branches. I'll ask you to review them when they're ready.
Comment on attachment 491858 [details] [diff] [review] Fix rev6 (follow Josh's suggestions) Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/16c2e141d418
Backed out, due to apparent leaks during crashtest: backout: http://hg.mozilla.org/mozilla-central/rev/0f7432f2cb5d merge: http://hg.mozilla.org/mozilla-central/rev/0830d8f97936 There were leaks on Linux Debug crashtest runs for this push & the one after: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192341.1290192765.26978.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192624.1290192989.28478.gz The leaks both looked like this: > TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 196 bytes during test execution > TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsLocalFile with size 124 bytes > TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsPluginHost with size 60 bytes > TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsStringBuffer with size 8 bytes > TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTArray_base with size 4 bytes
Seems to affect Linux Debug Reftest, too. (That run just completed & had the exact same objects leaked): http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192456.1290193921.32328.gz
I wish you'd let the tests run for one more cycle -- this may be another random orange. I'll try to reproduce the leaks locally.
Though if it *is* a random orange, it may keep happening after my patch was backed out. I hadn't at first notice that you let the tests run for two cycles.
I didn't see these leaks when I tested on the tryserver yesterday.
At backout time, I was pretty sure it was nonrandom because (a) it was the exact same leak for two subsequent crashtest-runs (and now two reftest runs, too*) (b) I couldn't find a matching intermittent leak in bugzilla (c) one of the leaked objects is nsPluginHost, and this bug's push touched nsPluginHost.h and 2 other plugin-related files * Second reftest failure with this leak: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290192801.1290194448.2190.gz So, this looks almost certainly nonrandom. Odd that it didn't show up on TryServer, though -- did your TryServer run have Linux Debug cycles?
> did your TryServer run have Linux Debug cycles? Yes. I ran two sets of tests, and both sets had 32-bit and 64-bit Linux cycles (opt and debug). You're probably right this isn't random. But I can't see how my patch could have caused it. Could you trigger new runs of the tests that failed? I'll run a new set of tryserver tests and see what happens.
(In reply to comment #28) > Could you trigger new runs of the tests that failed? I can't, but RelEng can. They actually prefer that bugs be filed for re-triggers, so they can track the various types of requests that come in. So, I filed Bug 613555 to get reftest & crashtest re-triggered on your push's cycle.
Oh shit, never mind. Now I see what I did wrong :-( (The patches I tested yesterday were slightly different than the one I landed today.) I'll try again in a bit. Thanks very much for filing that bug to re-run the tests ... even though I now see it won't be necessary.
Ok, resolved the re-running-tests bug as INVALID. Glad you figured out what was going on. :)
Comment on attachment 491858 [details] [diff] [review] Fix rev6 (follow Josh's suggestions) Landed again on trunk: http://hg.mozilla.org/mozilla-central/rev/73b2cd4fbd95 The problem with the previous patch is that I called nsPluginHost::GetInst() without releasing what it returned.
Comment on attachment 491973 [details] [diff] [review] 1.9.2-branch patch Oops. Part of patch is missing.
Comment on attachment 491973 [details] [diff] [review] 1.9.2-branch patch No, nothing is missing. My changes to nsNetUtil.h have already been landed.
Comment on attachment 491973 [details] [diff] [review] 1.9.2-branch patch >+ nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata; >+ if (!inst || !inst->mLibrary) >+ return false; >+ nsNPAPIPlugin* plugin = ((PluginPRLibrary *)inst->mLibrary)->GetPlugin(); I'm not sure the assumption you're making with this cast is valid. The "mLibrary" member variable is of type "PluginLibrary", which is a base class for "PluginPRLibrary" and "PluginModuleParent". If the plugin is out-of-process then this variable could be of type "PluginModuleParent". Maybe you should move the 'nsNPAPIPlugin' member for the library into the base class.
Attachment #491973 - Flags: review?(joshmoz) → review-
>>+ nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata; >>+ if (!inst || !inst->mLibrary) >>+ return false; >>+ nsNPAPIPlugin* plugin = ((PluginPRLibrary *)inst->mLibrary)->GetPlugin(); > > I'm not sure the assumption you're making with this cast is > valid. The "mLibrary" member variable is of type "PluginLibrary", > which is a base class for "PluginPRLibrary" and > "PluginModuleParent". If the plugin is out-of-process then this > variable could be of type "PluginModuleParent". Sigh. > Maybe you should move the 'nsNPAPIPlugin' member for the library > into the base class. Rather than changing a cross-module API, and then only on the 1.9.2 branch, I think it's best to add a (weak) mPlugin member to nsNSAPIPluginInstance (like the one that already exists on the trunk). In fact this is probably what I should have done from the beginning.
Comment on attachment 492235 [details] [diff] [review] 1.9.2-branch patch rev1 (fix problem) Landed on the 1.9.2 branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3f47fec824f9
I'd like to get this into 18.104.22.168/22.214.171.124 if we need a respin for any reason. We've already taken bug 611897 which sort of gives the game away, and this variant will affect more 3.6 people than the OJI version.
blocking1.9.1: .17+ → ?
blocking1.9.2: .14+ → ?
Whiteboard: [sg:high] → [sg:high] (goes with bug 611897)
Comment on attachment 491987 [details] [diff] [review] 1.9.1-branch patch Josh, please review my 1.9.1-branch patch, so that it's ready to land when/where it's judged to be needed.
Comment on attachment 491987 [details] [diff] [review] 1.9.1-branch patch Landed on the 1.9.1 branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fd2fcd4a4686
I believe this patch ended up being included in the 126.96.36.199 and 188.8.131.52 releases (as the result of respins). Is that correct?
No, it wasn't included in those earlier releases because of respins - this bug's "status" markers are still correct. Respins generally occur on "relbranches" (mini-branches off the "main" branch), so they don't automatically pick up later changes landed on the "main" branch.
OK, thanks. I've now confirmed what you said by looking under the appropriate tags here (http://hg.mozilla.org/releases/mozilla-1.9.2/tags) and here (http://hg.mozilla.org/releases/mozilla-1.9.1/tags). I misread some of the recent entries in the branches' tinderbox pushlogs, and this led me to think my patch had been included in the 184.108.40.206 and 220.127.116.11 releases.
Are the STR to verify the fix for this using the testcase from bug 589041 on Windows or Linux (since OS X didn't crash there)?
Whiteboard: [sg:high] (goes with bug 611897) → [sg:high] (goes with bug 611897) [qa-needs-STR] [qa-examined-191] [qa-examined-192]
This bug was reproducible on Windows, Linux and OS X -- but only with Java Plugin2. So you need to make sure you're using Java Plugin2 (aka the new Java plugin) when you try to reproduce this bug (using the testcase from bug 589041). Of course you also need to test with builds that don't (yet) have my patch.
(In reply to comment #48) > This bug was reproducible on Windows, Linux and OS X -- but only with > Java Plugin2. So you need to make sure you're using Java Plugin2 (aka > the new Java plugin) when you try to reproduce this bug (using the > testcase from bug 589041). Thanks. > Of course you also need to test with builds that don't (yet) have my > patch. If I didn't know this, then I'd be unqualified for my job, Steven. Basic QA. :-)
Where does one get the new Java Plugin2? Perhaps I'm misunderstanding but I looked at http://javaplugin.sourceforge.net/ and the JEP is only for OS X. I'm attempting to look at this on XP.
Apple's port of Sun/Oracle's Java Plugin2 "comes with" on both OS X 10.5.8 and 10.6.X (it's been bundled with Apple's Java Updates for both OSs for about a year and a half). To be sure you have it, make sure you've installed Apple's latest Java Update for whichever OS version (10.5.8 or 10.6.X) you're testing with -- Java for Mac OS X 10.5 Update 8 or Java for Mac OS X 10.6 Update 3. (Apple's Java Plugin2 is in /Library/Internet Plugin-Ins/JavaPlugin2_NPAPI.plugin.) Then you need to remove JavaEmbeddingPlugin.bundle and MRJPlugin.plugin from the Contents/MacOS/plugins/ directory of whatever distro you're testing with. (When the browser is looking for plugins, this is the directory it looks in first (before other directories like /Library/Internet Plug-Ins/). So when the JEP is here, Firefox will use it instead of whatever other Java plugin is installed elsewhere.) Java Plugin2 is included with Java 6 on Windows and Linux, and is (I think) always the default on Windows. It's usually also the default on Linux ... though there can be exceptions (I can go into greater detail about this if necessary). Sun/Oracle's OJI plugin is also included with Sun/Oracle's Java 6 on both Windows and Linux. But (like I just said) it's normally not used. (The OJI plugin *isn't* included on OS X -- the JEP (which also uses OJI) takes its place there.)
For XP, I installed the latest Java 6 and I ran both the testcases and had no crashes whatsoever in 18.104.22.168, which is pre-fix. I'll take a look on OS X 10.6.
> For XP, I installed the latest Java 6 and I ran both the testcases > and had no crashes whatsoever in 22.214.171.124, which is pre-fix. Remember that these are client-process crashes (crashes in Java Plugin2's client process (not Firefox's client process)). So you might have missed them (I did the first time I tested). Here's the best way to test for them: 1) Run Firefox and load any Java applet (e.g. the Clock applet at http://java.sun.com/applets/jdk/1.4/demo/applets/Clock/example1.html). 2) Load bug 589041's testcase in a different tab or window and run one of its tests. 3) Check to see if the applet from step 1 is still running -- it won't be if Java Plugin2's client process has crashed. (For more info see bug 589041 comment #13.)
Whiteboard: [sg:high] (goes with bug 611897) [qa-needs-STR] [qa-examined-191] [qa-examined-192] → [sg:high] (goes with bug 611897) [qa-examined-191] [qa-examined-192]
Thanks. Verified for 126.96.36.199 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) Gecko/20110111 Namoroka/3.6.14pre ( .NET CLR 3.5.30729) on XP. Running as discussed in comment 53 crashes the other applet pre-fix and does not do so post-fix. Verified for 184.108.40.206 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:220.127.116.11pre) Gecko/20110111 Shiretoko/3.5.17pre ( .NET CLR 3.5.30729).
Unhiding since this is only a plugin process abort.
Whiteboard: [sg:high] (goes with bug 611897) → (goes with bug 611897)
I backed this out of 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8f0513110015
Chris and Dan: Given the large number of bug reports on this issue on the 1.9.2 branch (FF 3.6.14), you might want to consider backing out the patches for bug 611910 and bug 620773 on the trunk, too -- at least sometime before the FF4 release. As best I can tell, the original bug reported here is seldom encountered, and not terribly severe (a plugin crash). So the side effects of my patches (as they now stand) currently look much worse than the problem my patches fixed. I currently have no idea why these problems are happening. It will take at least a couple of days of careful digging to get to the bottom of this. And it might take quite a bit longer to fix the problem (whatever it turns out to be) -- especially considering how fragmented the market for Java applets is, and how difficult it is to do comprehensive testing. It's also puzzling that I didn't discover these problems in my original tests (of which I did quite a lot), and that they weren't reported until quite a long time after my patches had landed. But here it's easier to understand the reasons why. Many Java applets are proprietary, and have a relatively small customer base. So it's possible for us to break them and not realize it until we've done a major release. I'll do my best to find a way to fix my patches for bug 611910 and bug 620773. But then I think it's best that we try to reach out for testing (of whatever I've come up with) to those who've reported side effects of my current patches.
Since we're no longer using JEP on trunk, but using javaplugin2, and we cannot reproduce the bug on trunk, I don't think there's value in backing this out on trunk.
This has nothing to do with the JEP. There are some indications that the problems reported on the 1.9.2 branch can also be reproduced on the trunk. But, of course, most users of Java applets aren't testing on the trunk. I'll know more once I can reproduce the problem(s) myself.
I checked 18.104.22.168 (where this is removed) on pogo.com and the Java applets appear to be working fine there.
> There are some indications that the problems reported on the 1.9.2 > branch can also be reproduced on the trunk. I'm finally able to reproduce bug 629030 on the 1.9.2 branch, but not on the trunk (see bug 629030 comment #50). As best I can currently tell, what people see on the trunk is a different problem, unconnected with my patch for bug 611910, and even with Firefox (since it also happens in IE8) -- see bug 629030 comment #49.
We would like to look into the test case for bug 589041 to see if there is anything could be done in Java plugin side. Could someone help us get access to it?
CC'd you on the bug.
You need to log in before you can comment on or make changes to this bug.