Closed Bug 990247 Opened 6 years ago Closed 6 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, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 --- wontfix
firefox32 --- verified
firefox-esr24 --- wontfix
firefox-esr31 --- wontfix

People

(Reporter: gkw, Unassigned)

Details

(Keywords: sec-moderate, testcase, valgrind, Whiteboard: [adv-main32+])

Attachments

(2 files, 1 obsolete file)

Attached file stack
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)
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)
Julian, any ideas here? Would this possibly be a false positive, or otherwise?
Flags: needinfo?(gary) → needinfo?(jseward)
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?
(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.
Attached patch A possible fix (obsolete) — Splinter Review
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 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 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+
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.
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.
Between that and requiring the profiler, this doesn't sound so bad.
Keywords: sec-moderate
Thanks, jseward!
Attached patch revised patchSplinter Review
Attachment #8411716 - Attachment is obsolete: true
Attachment #8412246 - Flags: review?(kvijayan)
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+
Does this patch require sec-approval before landing?
I would like to know that, since I was just about to land it.
https://hg.mozilla.org/mozilla-central/rev/ac50c746cfd3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Split off bug 1002852, "Investigate why type error in JS_snprintf call was not caught at compile time".
Group: javascript-core-security → core-security
Whiteboard: [adv-main32+]
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.