Last Comment Bug 763842 - Uninitialised value use in dosprintf(SprintfStateStr*, char const*, std::__va_list)
: Uninitialised value use in dosprintf(SprintfStateStr*, char const*, std::__va...
Status: RESOLVED FIXED
[soft], [qa-]
:
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Julian Seward [:jseward]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 00:10 PDT by Julian Seward [:jseward]
Modified: 2012-09-02 08:07 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments

Description Julian Seward [:jseward] 2012-06-12 00:10:14 PDT
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] 2012-06-13 16:09:33 PDT
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 [:njn] 2012-06-13 16:24:27 PDT
Julian, if you change it to "0x%lx" or "%p" does that fix the warning?
Comment 3 Julian Seward [:jseward] 2012-06-14 05:49:42 PDT
(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 [:njn] 2012-06-14 07:33:15 PDT
bent, are you happy with this change?  I am.
Comment 5 JP Rosevear [:jpr] 2012-06-19 13:36:04 PDT
Doesn't release block, but please land this nice fix :-).
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-06-19 16:04:50 PDT
....please attach a patch next time before requesting checkin-needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/25a4204f2e0c
Comment 7 Ed Morley [:emorley] 2012-06-20 02:19:24 PDT
https://hg.mozilla.org/mozilla-central/rev/25a4204f2e0c
Comment 8 Jason Smith [:jsmith] 2012-06-26 23:12:16 PDT
Does QA need to verify this fix from an end-user perspective? This sounds like an internal code change, but want to be sure.

Note You need to log in before you can comment on or make changes to this bug.