Closed
Bug 990247
Opened 10 years ago
Closed 10 years ago
Conditional jump or move depends on uninitialised value(s) and Use of uninitialised value of size 4 [@ dosprintf]
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla32
People
(Reporter: gkw, Unassigned)
Details
(Keywords: sec-moderate, testcase, valgrind, Whiteboard: [adv-main32+])
Attachments
(2 files, 1 obsolete file)
6.09 KB,
text/plain
|
Details | |
756 bytes,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
enableSPSProfilingAssertions(false) function f() { for (var j = 0; j < 1; ++j) { [][0] } } f() $ valgrind --track-origins=yes --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --show-leak-kinds=definite --smc-check=all-non-file ./js-opt-32-dm-vg-ts-linux-ccd91b78561f --no-asmjs --ion-parallel-compile=off --ion-eager w3-reduced.js shows an uninitialised value may have been created by a stack allocation at js::jit::Invalidate, by Valgrind. Tested on m-c rev ccd91b78561f on Linux. My configure flags are: CC="gcc -m32" CXX="g++ -m32" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh /home/gkwong/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR options> s-s because SPS profiler bugs can be security-related, so locking prior to further analysis. Setting needinfo? from Kannan as he has worked on SPS bugs recently.
Flags: needinfo?(kvijayan)
Comment 1•10 years ago
|
||
I don't know how to track this. I've peered at the stack trace and source code where it happens. The variable indicated as potentially uninitialized is definitely initialized in the code. It maybe that I'm looking at the wrong place, but it seems like it's: "num" in cvt_ll (inlined into dosprintf). That's passed in as |u.ll| from sprintf, and |u.ll| is initialized on all paths that arrive at |cvt_ll|. Not sure how to respond to this bug.
Flags: needinfo?(kvijayan) → needinfo?(gary)
Reporter | ||
Comment 2•10 years ago
|
||
Julian, any ideas here? Would this possibly be a false positive, or otherwise?
Flags: needinfo?(gary) → needinfo?(jseward)
Comment 3•10 years ago
|
||
Other ways forward -- turn on compiler debug settings that check for uninitialized values (for example, in MSVS it's -RTCu debug build flag -- run under another tool like MEMSAN that check for uninitialized values. If nothing shows up maybe this is a bug in the valgrind instrumentation?
Comment 4•10 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #1) > I don't know how to track this. I've peered at the stack trace and source > code where it happens. The variable indicated as potentially uninitialized > is definitely initialized in the code. It maybe that I'm looking at the > wrong place, but it seems like it's: "num" in cvt_ll (inlined into > dosprintf). That's passed in as |u.ll| from sprintf, and |u.ll| is > initialized on all paths that arrive at |cvt_ll|. u.ll is always assigned, but Valgrind's saying that the value assigned to it is itself (fully or partially) undefined. And the undefinedness originates in a stack allocation in Invalidate(). The relevant line in Invalidate() is this: > JS_snprintf(buf, len, "Invalidate %s:%llu", filename, script->lineno()); It's the %llu/script->lineno() combination that it's complaining about. I'm not sure why it says there's a stack allocation involved.
Comment 5•10 years ago
|
||
This is a format string error. Normally I'd expect this kind of thing to be picked up at compile time, providing JS_snprintf is marked for gcc format string checking. This isn't the first time this has happened with printf-style functions inside js/src: see for example bug 960603. What happens is, the format %llu requires a 64 bit int, but script->lineno() provides a size_t, which is 64 bit on a 64-bit target but 32 bits on a 32 bit target. On 32-bit x86, args are passed on the stack. So gcc generates code to push one word (the size_t) but the printf implementation pulls two words off the stack, in accordance with %llu. I'd guess what it reads is doubleword-aligned, and one of the two words is an alignment (padding) word, which is why V complains that the uninitialised value is stack allocated in jit::Invalidate(). One fix (attached) is to cast the argument to a 64-bit type. IMO it's a bit strange to use size_t for a script line number, since that has nothing to do with the machine word size and it's particularly strange given that class JSScript actually stores the line number (::lineno_) as a uint32_t, but the accessor function lineno() casts it to size_t. The patch stops V complaining. I don't know if the printed string is still as expected since I don't know how to check that.
Flags: needinfo?(jseward)
Comment 6•10 years ago
|
||
Comment on attachment 8411716 [details] [diff] [review] A possible fix Requesting feedback. I don't know whether the cast to "unsigned long long" generates a value which is compatible with %llu on win64 or win32 -- someone with more windows-fu than me needs to comment.
Attachment #8411716 -
Flags: feedback?(kvijayan)
Comment 7•10 years ago
|
||
Comment on attachment 8411716 [details] [diff] [review] A possible fix Review of attachment 8411716 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Ion.cpp @@ +2768,5 @@ > return false; > > // Construct the descriptive string. > + JS_snprintf(buf, len, "Invalidate %s:%llu", filename, > + (unsigned long long)script->lineno()); I think just '%u' and cast-to-unsigned-int should suffice here.
Attachment #8411716 -
Flags: feedback?(kvijayan) → feedback+
Comment 8•10 years ago
|
||
Does anyone have a good idea how dangerous (or not) this is? We need to get a security rating on it in order to go in.
Comment 9•10 years ago
|
||
Well, it can cause a totally bogus number to be printed in this "Invalidate" string. But no overrun of the allocated buffer AFAICS, since it's written to by JS_snprintf. Does printing a bogus number matter? I don't know.
Comment 10•10 years ago
|
||
Between that and requiring the profiler, this doesn't sound so bad.
Keywords: sec-moderate
Comment 11•10 years ago
|
||
Thanks, jseward!
Comment 12•10 years ago
|
||
Attachment #8411716 -
Attachment is obsolete: true
Attachment #8412246 -
Flags: review?(kvijayan)
Comment 13•10 years ago
|
||
Comment on attachment 8412246 [details] [diff] [review] revised patch Review of attachment 8412246 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review. r=me. Nb: there are several other places in the code base where we cast the result of lineno() to a 32-bit type. So I was about to file a bug to change lineno() to return uint32_t instead of size_t, but now I see that we'd possibly need to change all of the following: length(), dataSize(), column(), mainOffset(), natoms(), nslots(), sourceStart(), sourceEnd(). Well... maybe the ones that are byte sizes (dataSize(), mainOffset(), source{Start,End}()?) could stay as size_t.
Attachment #8412246 -
Flags: review?(kvijayan) → review+
Comment 14•10 years ago
|
||
Does this patch require sec-approval before landing?
Comment 15•10 years ago
|
||
I would like to know that, since I was just about to land it.
Comment 16•10 years ago
|
||
sec-moderate can land without approval. https://wiki.mozilla.org/Security/Bug_Approval_Process
https://hg.mozilla.org/mozilla-central/rev/ac50c746cfd3
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox29:
--- → ?
status-firefox30:
--- → ?
status-firefox31:
--- → ?
status-firefox32:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 19•10 years ago
|
||
Split off bug 1002852, "Investigate why type error in JS_snprintf call was not caught at compile time".
Updated•10 years ago
|
Group: javascript-core-security → core-security
Updated•10 years ago
|
status-firefox-esr24:
--- → wontfix
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [adv-main32+]
Comment 20•10 years ago
|
||
Reproduced the original issue from comment #0 using changeset ccd91b78561f in m-c, received the same "Uninitialised value was created by a stack allocation" message under valgrind several times. Went through the following verification: m-c using changeset 47c9418fbc28: [PASSED] - ensured that the "Uninitialised value was created by a stack allocation" wasn't appearing under valgrind aurora using changeset da558e9dcae3: [PASSED] - ensured that the "Uninitialised value was created by a stack allocation" wasn't appearing under valgrind beta using changeset ebd0ee3e97dc: [PASSED] - ensured that the "Uninitialised value was created by a stack allocation" wasn't appearing under valgrind
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•