Closed Bug 659666 Opened 10 years ago Closed 10 years ago
PSM passes uninitialized value for inout check
Value argument of prompt Password
In bug 658351, we added an assertion in the JS engine that checks for malformed JS Bools. An example of this which motivated the assertion can be found in the original description of bug 654126. However, the assert turned quite a few tests orange. One of the stacks <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1306338785.1306339147.7955.gz> has PK11 stuff in it, so we need to investigate how this can possibly happen. I've pushed this to try which will yield hopefully more interesting stack traces. http://tbpl.mozilla.org/?tree=Try&rev=e9bf068c8f4c
For what it's worth, I don't see any PK11 stuff in the log you're linking to... I do see a broken stack (isGlobal() calling js_SetPropertyHelper???)
Ah, that's a different log than comment 0! The relevant C++ is this: 810 PRBool checkState; 811 rv = proxyPrompt->PromptPassword(nsnull, promptString.get(), 812 &password, nsnull, &checkState, &value); and the relevant idl is this: 111 boolean promptPassword(in wstring dialogTitle, 112 in wstring text, 113 inout wstring password, 114 in wstring checkMsg, 115 inout boolean checkValue); Note the passing of an uninitialized value (checkState) as an inout argument. That would be bad.
Summary: PSM possibly creates a badly formed JSBool → PSM passes uninitialized value for inout checkValue argument of promptPassword
And it doesn't matter what the value is, since checkMsg == null means the callee ignores that argument. But in the meantime the marshalling code in XPConnect assumes that converting a PRBool to a JSBool just involves a cast, so we lose. Initializing checkState to PR_FALSE, with a comment about how the exact value does not matter, should fix.
So I've been looking into this, and we've got a few instances of the issue lurking around in the gecko codebase. - nsDocumentViewer.cpp:1204 PRBool dummy is passed to a JS function without being ever initialized (it's, hem, a dummy), - the one bz mentioned in comment #3, in nsNSSCallbacks.cpp:810, - possibly others that are not covered by our tests. After discussing with bz on IRC, the following might be reasonable: - fix these two issues that have come up, - keep the fatal JS_ASSERT in jsapi.h, - have xpcconnect.cpp warn (possibly aggressively) if the boolean is malformed, as it possibly indicates a bug lurking around, but go on and pass !!thePRBool to make sure it doesn't violate the JS engine's invariants. JS hackers, what's your opinion on this? :-) I'll check for other failures in the try run and report here if I figure out another one of these situations.
Fix the issues, reject drek. We require input to be well-formed many places in the JSAPI, and I see no reason why this should be any different, particularly not if would mean more work at runtime to be lenient.
http://tbpl.mozilla.org/?tree=Try&rev=71dc30c95286 This is a try-run on all debug platforms with the JS_ASSERT patch and the two uninitialized PRBools properly initialized. I've got a few oranges remaining, could someone please confirm that these non-related, random oranges?
That doesn't seem to show anything...
Every orange showing is a known intermittent. Looks good to me.
This patch fixes the two bad cases, with a comment, as discussed before.
Assignee: nobody → jonathan.protzenko
Status: NEW → ASSIGNED
Attachment #536250 - Flags: review?(josh)
Comment on attachment 536250 [details] [diff] [review] Fix the two bad cases I'm not certain why you've tagged me for review, but the comment seems pretty sensible.
Attachment #536250 - Flags: review?(josh) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
> After discussing with bz on IRC, the following might be reasonable: Uh... except we did not actually implement the solution we decided was reasonable? What the hell?
And why exactly were callsites not actually audited, as we also discussed?
You need to log in before you can comment on or make changes to this bug.