The default bug view has changed. See this FAQ.
Bug 883686 (CVE-2013-1728)

valgrind errors in JS testsuite ("conditional jumps on uninitialized data")

RESOLVED FIXED in Firefox 24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sunfish, Assigned: h4writer)

Tracking

(Depends on: 1 bug, {csectype-uninitialized, regression, sec-moderate})

Trunk
mozilla24
csectype-uninitialized, regression, sec-moderate
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite ?

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main24+])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(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
Status: UNCONFIRMED → NEW
Depends on: 883688
Ever confirmed: true
Summary: valgrind errors in JS testsuite → valgrind errors in JS testsuite ("conditional jumps on uninitialized data")
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.
(Reporter)

Comment 3

4 years ago
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.
This needs a JS dev to look at. Setting needinfo from Naveed.
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 5

4 years ago
Taking. I think I reviewed the blamed code. At least I know what could be the issue and how to solve it.
Assignee: general → hv1989
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 6

4 years ago
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.
Attachment #765005 - Flags: review?(sstangl)
Hannes, any idea what the regressing changeset is?
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?
Attachment #765005 - Flags: review?(sstangl) → review+
(Assignee)

Updated

4 years ago
Depends on: 885335
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/415afe797a6e
https://hg.mozilla.org/mozilla-central/rev/415afe797a6e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Does this affect branches?
Flags: needinfo?(hv1989)
(Assignee)

Comment 12

4 years ago
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.
Flags: needinfo?(hv1989)
(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.
Blocks: 837312
status-b2g18: --- → unaffected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox-esr17: --- → unaffected
tracking-firefox24: --- → +
Keywords: csec-uninitialized, regression, sec-moderate
Whiteboard: [adv-main24+]
Alias: CVE-2013-1728
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty-
Group: core-security
You need to log in before you can comment on or make changes to this bug.