Last Comment Bug 883686 - (CVE-2013-1728) valgrind errors in JS testsuite ("conditional jumps on uninitialized data")
(CVE-2013-1728)
: valgrind errors in JS testsuite ("conditional jumps on uninitialized data")
Status: RESOLVED FIXED
[adv-main24+]
: csectype-uninitialized, regression, sec-moderate
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Hannes Verschore [:h4writer]
:
Mentors:
Depends on: 885335 883688
Blocks: 837312
  Show dependency treegraph
 
Reported: 2013-06-16 16:32 PDT by Dan Gohman [:sunfish]
Modified: 2014-11-19 20:11 PST (History)
12 users (show)
dveditz: sec‑bounty-
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
fixed
unaffected
unaffected


Attachments
valgrind log (337.46 KB, text/plain)
2013-06-19 12:09 PDT, Dan Gohman [:sunfish]
no flags Details
Patch (1.17 KB, patch)
2013-06-19 14:24 PDT, Hannes Verschore [:h4writer]
sstangl: review+
Details | Diff | Review

Description Dan Gohman [:sunfish] 2013-06-16 16:32:16 PDT
I happened to run js/src/tests/jstests.py in valgrind mode and found two unique errors, which I have confirmed. I don't yet know the scope of the problems. I believe both are relatively easy to fix.

First, the variable gotLambda in js/src/ion/IonBuilder.cpp is being used uninitialized. It is declared without an initializer, and the statement which would assign to it is inside an if statement and is not always executed. Several uses of gotLambda follow. This can probably be fixed by just initializing the gotLambda variable up front.

Second, js1_5/extensions/regress-341956-01.js and js1_2/Array/splice1.js see conditional jumps on uninitialized data. It appears a block gets created by js/src/ion/IonBuilder.cpp (inlineBlock) which is later discarded ("Natives may veto inlining"), but it still has a resume point which has a use of a value in a block that is not discarded. When range analysis walks def-use graphs, it finds this use and wanders into the discarded block, which has an uninitialized domIndex_ field. This can probably be fixed by clearing out the resume points of the block when it is discarded.
Comment 1 Nicolas B. Pierron [:nbp] 2013-06-17 03:25:29 PDT
(In reply to Dan Gohman from comment #0)
> First, the variable gotLambda in js/src/ion/IonBuilder.cpp is being used
> uninitialized. It is declared without an initializer, and the statement
> which would assign to it is inside an if statement and is not always
> executed. Several uses of gotLambda follow. This can probably be fixed by
> just initializing the gotLambda variable up front.

This one is being addressed in Bug 883688
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2013-06-19 11:14:18 PDT
Hi Dan, do you mind posting the Valgrind logs? For such errors, please run Valgrind with --smc-check=all-non-file as well as --track-origins=yes.
Comment 3 Dan Gohman [:sunfish] 2013-06-19 12:09:24 PDT
Created attachment 764925 [details]
valgrind log

Attached is the log from running this valgrind command:

valgrind --smc-check=all-non-file --track-origins=yes /home/sunfish/project/build-dbg/js -f shell.js -f js1_5/shell.js -f js1_5/extensions/shell.js -f js1_5/extensions/regress-341956-01.js 

It contains a lot more errors than I saw before, though this is a valgrind on a different machine than the one I ran the original tests on. I haven't investigated them at all.

The error I described in this bug report is the last error in the log, the conditional jump on uninitialized data at MIRGraph.cpp:818.
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2013-06-19 13:31:59 PDT
This needs a JS dev to look at. Setting needinfo from Naveed.
Comment 5 Hannes Verschore [:h4writer] 2013-06-19 14:20:21 PDT
Taking. I think I reviewed the blamed code. At least I know what could be the issue and how to solve it.
Comment 6 Hannes Verschore [:h4writer] 2013-06-19 14:24:28 PDT
Created attachment 765005 [details] [diff] [review]
Patch

This is the same code we use in IonBuilder.cpp restartLoop to nuke all information.

I don't see the MIRGraph.cpp issues with this patch anymore. Jit-tests also approves.
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2013-06-19 14:27:14 PDT
Hannes, any idea what the regressing changeset is?
Comment 8 Sean Stangl [:sstangl] 2013-06-19 14:31:04 PDT
Comment on attachment 765005 [details] [diff] [review]
Patch

Review of attachment 765005 [details] [diff] [review]:
-----------------------------------------------------------------

Could we have a discardBlock() function that calls all these discardFoo() functions for us?
Comment 9 Hannes Verschore [:h4writer] 2013-06-20 07:39:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/415afe797a6e
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-06-20 17:03:52 PDT
https://hg.mozilla.org/mozilla-central/rev/415afe797a6e
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2013-06-20 17:12:04 PDT
Does this affect branches?
Comment 12 Hannes Verschore [:h4writer] 2013-06-22 08:35:12 PDT
Sorry about the delay. Just tried aurora and the valgrind error about IonBuilder is not present. On the other hand, the issue I fixed was introduced in bug 837312 (FF22+). Though we never had any bugs caused by it. So I assume it's not that important to uplift.
Comment 13 Daniel Veditz [:dveditz] 2013-08-23 16:28:07 PDT
(In reply to Hannes Verschore [:h4writer] from comment #12)
> Though we never had any bugs caused by it.
> So I assume it's not that important to uplift.

Uninitialized memory may not be noticed or cause bugs in day to day use, but if you know they're there you could try to craft an exploit that makes sure the previous allocation in the requisite spot has more useful values.

Note You need to log in before you can comment on or make changes to this bug.