883686 - (CVE-2013-1728) valgrind errors in JS testsuite ("conditional jumps on uninitialized data")

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Gohman [:sunfish]]

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]]

(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] (NOT official MoCo now)]

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]]

Attachment: 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] (NOT official MoCo now)]

This needs a JS dev to look at. Setting needinfo from Naveed.

Comment 5

[Hannes Verschore [:h4writer]]

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]]

Attachment: 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] (NOT official MoCo now)]

Hannes, any idea what the regressing changeset is?

Comment 8

[Sean Stangl [:sstangl]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/415afe797a6e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/415afe797a6e

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Does this affect branches?

Comment 12

[Hannes Verschore [:h4writer]]

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]]

(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.