Created attachment 666368 [details] Valgrind stack Number.prototype.toPrecision('') throws a lot of "Conditional jump or move depends on uninitialised value(s)" messages on js opt shell on m-c changeset cd82278e2bb8 without any CLI arguments with Valgrind. Not sure if this is truly s-s, feel free to open up if this is not the case.
Gary, for these uninitialised-value error reports, can you also report the origin(s) of the uninitialised values(s), as produced by --track-origins=yes? These make it a lot easier to figure out what the problem is.
Is this correct? ==3542== Conditional jump or move depends on uninitialised value(s) ==3542== at 0x4E647C: ComputePrecisionInRange(JSContext*, int, int, JS::Value const&, int*) [clone .constprop.103] (jsatominlines.h:88) ==3542== by 0x4E698E: num_toExponential(JSContext*, unsigned int, JS::Value*) (jsnum.cpp:854) ==3542== by 0x4D2D3A: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:370) ==3542== by 0x4CDAE5: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2460) ==3542== by 0x4D2A0A: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324) ==3542== by 0x4D3ABA: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509) ==3542== by 0x43FBD4: JS_ExecuteScript (jsapi.cpp:5640) ==3542== by 0x421CB1: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:439) ==3542== by 0x425B65: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4688) ==3542== by 0x41834F: main (js.cpp:4947) ==3542== Uninitialised value was created by a stack allocation ==3542== at 0x4E6806: num_toExponential(JSContext*, unsigned int, JS::Value*) (jsnum.cpp:863) ==3542==
Looks plausible. Do all of the errors in comment #0 have the same origin point, that is, a stack allocation in num_toExponential?
The "int *precision" passed to ComputePrecisionInRange is not initialized and passed to IntToCString: ToCStringBuf cbuf; char *numStr = IntToCString(&cbuf, *precision); With a Clang debug build: js> Number.prototype.toPrecision('') typein:1:0 RangeError: precision 34685472 out of range
I'm guessing bug 771744 was the regressor based on the line number: http://hg.mozilla.org/mozilla-central/rev/4483ab804f1e
5 years ago
Created attachment 666786 [details] [diff] [review] Patch with tests Sigh, helper-method-extraction fail. A truly intelligent compiler, that could figure out the dependence on undefined behavior here, could perfectly well compile this to code to nonsense. All the relevant code is easily visible even to file-at-a-time compilers, so I have no difficulty believing some compilers can figure out this use-of-undefined-value and optimize based upon it. The question is would the optimization enable black magic to be completed; I'm pretty sure only a compiler expert from each major compiler could say what might be possible here. Compilers not so smart, that just passed the undefined |*precision| value to IntToCString, would produce a garbage error message (with some indeterminate integer in the output string) but would not be exploitable. So whether this is a critical problem, or just one that produces bogus output, depends how aggressively compilers optimize. Paranoia and uncertainty and lack of knowledge suggest assuming this is a critical issue, although it's very much possible it isn't (or isn't for certain compilers, but is for others).
Pushed to Try. https://tbpl.mozilla.org/?tree=Try&rev=70bffe879810
I was thinking of just pushing this to inbound -- the patch is clear enough that I don't think try's worth the resources. But there is the little matter of security-sensitivity of this. I guess you want it on trunk before you take it on branches, so maybe pushing is just the right thing and I'm just angsting overmuch (especially given that this is only questionably a vulnerability). Anyway, I think probably we want this on as many branches as we can, ideally as quickly as possible, to be on the safe side.
FWIW, it tagged along with some other stuff, so it's not a big drain on Try's resources :)
Oh hey, excellent timing! Since we haven't shipped this yet, this doesn't need security-approval specifically (although it does need the usual branch approvals). Let's get 'er in ASAP and avoid that issue entirely. :-) And I guess since this isn't shipped yet, so indeed I was angsting overmuch, and we should just fix it everywhere and not overworry about any issue that might or might not temporarily exist in alphas/betas/nightlies.
Comment on attachment 666786 [details] [diff] [review] Patch with tests Or no, I misread the sec-approval process mail. Sigh. [Security approval request comment] How easily can the security issue be deduced from the patch? Pretty easily -- it's easy to track back to the callers and see that |*precision| is uninitialized memory, and that it's a hidden bug should indicate that the patch is worth looking at to find that. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nothing mentions it as such, just as a bug implementing the relevant APIs (and testing them), but I didn't attempt to obfuscate anything. The patch lets the cat out of the bag; all the rest here doesn't add much, I think. Which older supported branches are affected by this flaw? Aurora, Beta, everything but actual shipped-to-the-public-at-large releases. If not all supported branches, which bug introduced the flaw? Bug 771744 introduced this, thinko when making changes requested by the reviewer. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Haven't attempted backporting, likely trivial -- this code hasn't changed much, if at all, since the bug was introduced. How likely is this patch to cause regressions; how much testing does it need? Unlikely as basically an obvious thinko, shouldn't need testing beyond what the included test provides. Although, I'd be a little happier if we were running the added test under valgrind or something on occasion to catch the UMR. :-\ (Side issue, I know.)
(In reply to Ryan VanderMeulen from comment #7) > https://tbpl.mozilla.org/?tree=Try&rev=70bffe879810 Looks green.
Already wontfixing for FF16 since this doesn't seem critical enough to take between our final beta and release.
Comment on attachment 666786 [details] [diff] [review] Patch with tests This looks like a correctness problem not an exploitable security issue for any value of the undefined int. I wouldn't over-worry about what bogosities compiler optimizations might introduce until we see evidence for it. sec-approved, and I think we can unhide the bug.
> sec-approved, and I think we can unhide the bug. Unhiding, and this can be landed.
This needs mozilla-beta uplift nomination so we can get it into 17.
Comment on attachment 666786 [details] [diff] [review] Patch with tests [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 771744 User impact if declined: bogus results from some method calls if you call some Number.prototype.to* with invalid arguments. Testing completed (on m-c, etc.): It's baked on m-c, patch includes a test. Risk to taking this patch (and alternatives if risky): Not much, pretty clear what the issue is, pretty clear what the fix should be. String or UUID changes made by this patch: N/A
Pushed to Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/7e12261924cd
[qa-] for verification since this has a in-testsuite coverage.
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.