763842 - Uninitialised value use in dosprintf(SprintfStateStr*, char const*, std::__va_list)

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Julian Seward [:jseward]]

DOM: Other is probably the wrong component; not sure of a better one tho.

Whilst starting B2G on GalaxyS2, I saw the complaint below, plus some
probable duplicates.

From a few minutes peering at the code, what might have happened is:
(w/ some guessing, so don't take this as certain)

WorkerPrivate::DoRunLoop(JSContext*) at WorkerPrivate.cpp:2668 does

  mMemoryReporter = new WorkerMemoryReporter(this);

The constructor called is inlined at this point, hence the origin of
the uninitialised value is listed here.  Looking at the constructor's
code, WorkerMemoryReporter(WorkerPrivate*), starting around line 153,
there is (lines 168/169)

      uint32_t addressSize =
        JS_snprintf(address, sizeof(address), "0x%llx", aWorkerPrivate);

and I think that leads to the dosprintf call that is complained about.
In particular it's complaining about a 64 bit comparison against zero
(jsprf.cpp:276) in cvt_ll

... int64_t num ...
    while (num != 0) {

The JS_snprintf line above looks suspicious to me.  It takes
aWorkerPrivate (a pointer type, hence 4 bytes on ARM) and asks it to
be printed with a %llx (64-bit) format.  I'd guess that the complaints
arises because that arg is passed on the stack without first being
widened to 64 bits, hence JS_sprintf hauls 64 bits out of memory, half
of which are garbage, and attempts to print it.

Uninitialised data from the same source is also reported to surface at
a second place, LimitStuff(SprintfStateStr*, char const*, unsigned
int), which reduces the likelyhood that this is a false positive from
Valgrind.


Conditional jump or move depends on uninitialised value(s)
   at 0x5C513AA: dosprintf(SprintfStateStr*, char const*, std::__va_list) (js/src/jsprf.cpp:276)
   by 0x5C516F5: JS_vsnprintf (js/src/jsprf.cpp:1178)
   by 0x5C51877: JS_snprintf (js/src/jsprf.cpp:1158)
   by 0x550E8E5: mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) (dom/workers/WorkerPrivate.cpp:168)
   by 0x5505B2B: (anonymous namespace)::WorkerThreadRunnable::Run() (dom/workers/RuntimeService.cpp:333)
   by 0x59B08EF: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:624)
   by 0x598BC05: NS_ProcessNextEvent_P(nsIThread*, bool) (/home/sewardj/B2G-1/B2G/objdir-gecko/xpcom/build/nsThreadUtils.cpp:213)
   by 0x59B0E53: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:257)
   by 0x4CE7867: ??? (in /system/b2g/libnspr4.so)

 Uninitialised value was created by a stack allocation
   at 0x550E6F4: mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) (dom/workers/WorkerPrivate.cpp:2626)

Comment 1

[Julian Seward [:jseward]]

The offending values are also surfacing in a helper routine for
integer division.  They have the same origin as the one in comment #0.
Not sure why I didn't see these extras the first time.

Conditional jump or move depends on uninitialised value(s)
   at 0x48940E2: __udivdi3 (libgcc2.c:915)
   by 0x4892EB3: __gnu_uldivmod_helper (bpabi.c:53)
   by 0x4892E8B: __aeabi_uldivmod (bpabi.S:95)
 Uninitialised value was created by a stack allocation
   at 0x5558324: mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) (WorkerPrivate.cpp:2626)

Conditional jump or move depends on uninitialised value(s)
   at 0x4892488: __udivsi3 (lib1funcs.asm:697)
   by 0x48924F9: __aeabi_uidivmod (lib1funcs.asm:853)
 Uninitialised value was created by a stack allocation
   at 0x5558324: mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) (WorkerPrivate.cpp:2626)

Comment 2

[Nicholas Nethercote [inactive]]

Julian, if you change it to "0x%lx" or "%p" does that fix the warning?

Comment 3

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Julian, if you change it to "0x%lx" or "%p" does that fix the warning?

Yes, %p gets rid of all of them.  It looks like a pretty straightforward
case of format-string mismatch.  I notice that JS_vsnprintf is not declared
with format string checking -- that might have caught it earlier.


diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
index 240f8e3..7e7757b 100644
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -165,7 +165,7 @@ public:
       // 64bit address plus '0x' plus null terminator.
       char address[21];
       uint32_t addressSize =
-        JS_snprintf(address, sizeof(address), "0x%llx", aWorkerPrivate);
+        JS_snprintf(address, sizeof(address), "%p", aWorkerPrivate);
       if (addressSize != uint32_t(-1)) {
         mAddressString.Assign(address, addressSize);
       }

Comment 4

[Nicholas Nethercote [inactive]]

bent, are you happy with this change?  I am.

Comment 5

[JP Rosevear [:jpr]]

Doesn't release block, but please land this nice fix :-).

Comment 6

[Ryan VanderMeulen [:RyanVM]]

....please attach a patch next time before requesting checkin-needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/25a4204f2e0c

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/25a4204f2e0c

Comment 8

[Jason Smith [:jsmith]]

Does QA need to verify this fix from an end-user perspective? This sounds like an internal code change, but want to be sure.