Closed Bug 932851 Opened 6 years ago Closed 6 years ago

Use of uninitialized value created on stack: : js::Discard() (StructuredClone.cpp:351)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ishikawa, Assigned: sfink)

References

Details

Attachments

(1 file)

I see the following uninitialized value access errors during the |make
mozmill| test suite run of comm-central thunderbird (source refreshed
about 48 hours ago, and this is a DEBUG BUILD)  under valgrind/memcheck.
(I use a wrapper program to run thunderbird under valgrind).

More than two dozen instances were observed (maybe close to three
dozens since the test still runs.)  This did not happen when similar
memcheck run on TB during |make mozmill| test was performed about a
couple of weeks ago.  So I think the regression is fairly recent (in
October?).

This is the error: 

==13404== Conditional jump or move depends on uninitialised value(s)
==13404==    at 0x9C41DA0: js::Discard(unsigned long const*, unsigned long const*) [clone .isra.120] (StructuredClone.cpp:351)
==13404==    by 0x9C429EA: js::SCOutput::~SCOutput() (StructuredClone.cpp:550)
==13404==    by 0x9C473C0: js::WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, unsigned long**, unsigned long*, JSStructuredCloneCallbacks const*, void*, JS::Value) (StructuredClone.cpp:322)
==13404==    by 0x9C4778E: JS_WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, unsigned long**, unsigned long*, JSStructuredCloneCallbacks const*, void*, JS::Handle<JS::Value>) (StructuredClone.cpp:1586)
==13404==    by 0x9C478B5: JSAutoStructuredCloneBuffer::write(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JSStructuredCloneCallbacks const*, void*) (StructuredClone.cpp:1730)
==13404==    by 0x9C478FB: JSAutoStructuredCloneBuffer::write(JSContext*, JS::Handle<JS::Value>, JSStructuredCloneCallbacks const*, void*) (StructuredClone.cpp:1718)
==13404==    by 0x9C47994: JS_StructuredClone(JSContext*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) (StructuredClone.cpp:1639)
==13404==    by 0x8010CD7: xpc::SetSandboxMetadata(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) (Sandbox.cpp:1768)
==13404==    by 0x80147D9: xpc::CreateSandboxObject(JSContext*, JS::MutableHandle<JS::Value>, nsISupports*, xpc::SandboxOptions&) (Sandbox.cpp:1112)
==13404==    by 0x8045922: XPCJSRuntime::GetJunkScope() (XPCJSRuntime.cpp:3387)
==13404==    by 0x8045C58: xpc::GetJunkScopeGlobal() (XPCJSRuntime.cpp:468)
==13404==    by 0x754D3DD: nsDocument::Init() (nsDocument.cpp:2017)
==13404==    by 0x78BB0CB: mozilla::dom::XMLDocument::Init() (XMLDocument.cpp:238)
==13404==    by 0x78B6DD9: NS_NewXMLDocument(nsIDocument**, bool) (XMLDocument.cpp:181)
==13404==    by 0x78BA6D2: NS_NewDOMDocument(nsIDOMDocument**, nsAString_internal const&, nsAString_internal const&, nsIDOMDocumentType*, nsIURI*, nsIURI*, nsIPrincipal*, bool, nsIGlobalObject*, DocumentFlavor) (XMLDocument.cpp:122)
==13404==    by 0x74A70FC: mozilla::dom::DOMParser::SetUpDocument(DocumentFlavor, nsIDOMDocument**) (DOMParser.cpp:517)
==13404==    by 0x74A74EE: mozilla::dom::DOMParser::ParseFromStream(nsIInputStream*, char const*, int, char const*, nsIDOMDocument**) (DOMParser.cpp:229)
==13404==    by 0x74A9283: mozilla::dom::DOMParser::ParseFromStream(nsIInputStream*, nsAString_internal const&, int, mozilla::dom::SupportedType, mozilla::ErrorResult&) (DOMParser.cpp:183)
==13404==    by 0x8DEFA57: mozilla::dom::DOMParserBinding::parseFromStream(JSContext*, JS::Handle<JSObject*>, mozilla::dom::DOMParser*, JSJitMethodCallArgs const&) (DOMParserBinding.cpp:260)
==13404==    by 0x8DEA309: mozilla::dom::DOMParserBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (DOMParserBinding.cpp:406)
==13404==    by 0x9BE32AA: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:220)
==13404==    by 0x9BE052D: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:462)
==13404==    by 0x9BD95BC: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2499)
==13404==    by 0x9BE0308: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:419)
==13404==    by 0x9BE077A: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:481)
==13404==    by 0x9BE0FA9: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:512)
==13404==    by 0x9A3191F: JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) (jsapi.cpp:4988)
==13404==    by 0x806ADE8: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJSClass.cpp:1423)
==13404==    by 0x8062C46: nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJS.cpp:591)
==13404==    by 0x924D77F: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:122)
==13404==    by 0x924CBAA: SharedStub (in /REF-OBJ-DIR/objdir-tb3/mozilla/toolkit/library/libxul.so)
==13404==    by 0x924CA6D: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164)
==13404==    by 0x8075AF4: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2797)
==13404==    by 0x8082219: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1311)
==13404==    by 0x9BE32AA: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:220)
==13404==    by 0x9BE052D: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:462)
==13404==    by 0x9BD95BC: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2499)
==13404==    by 0x9BE0308: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:419)
==13404==    by 0x9BE077A: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:481)
==13404==    by 0x9BE0FA9: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:512)
==13404==    by 0x9BE128F: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:583)
==13404==    by 0x9AEE039: bool NativeGetInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (Shape-inl.h:66)
==13404==    by 0x9AEEBB1: bool GetPropertyHelperInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (jsobj.cpp:4291)
==13404==    by 0x9BD31AC: Interpret(JSContext*, js::RunState&) (jsobj.h:991)
==13404==    by 0x9BE0308: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:419)
==13404==    by 0x9BE077A: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:481)
==13404==    by 0x9BD95BC: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2499)
==13404==    by 0x9BE0308: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:419)
==13404==    by 0x9BE077A: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:481)
==13404==    by 0x9A91BD6: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1030)
==13404==  Uninitialised value was created by a stack allocation
==13404==    at 0x9C46F04: js::WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, unsigned long**, unsigned long*, JSStructuredCloneCallbacks const*, void*, JS::Value) (StructuredClone.cpp:319)
==13404==

From the inspection of the code below, in
mozilla/js/src/vm/StructuredClone.cpp, I can see the area pointed by
|begin| has an uninitialized value at/near the beginning. (depending
on the endian?)

Running on 64-bit Debian GNU/Linux.

345 Discard(const uint64_t *begin, const uint64_t *end)
346 {
347    const uint64_t *point = begin;
348
349    uint64_t u = LittleEndian::readUint64(point++);
350    uint32_t tag = uint32_t(u >> 32);       <===  Culprit.
351    if (tag != SCTAG_TRANSFER_MAP_HEADER)   <===
        return;

545 SCOutput::SCOutput(JSContext *cx) : cx(cx), buf(cx) {}
546
547 SCOutput::~SCOutput()
548 {
549    // Free any transferable data left lying around in the buffer
550    Discard(rawBuffer(), rawBuffer() + count());
551 }

rawBuffer() is:

140    uint64_t count() const { return buf.length(); }
141    uint64_t *rawBuffer() { return buf.begin(); }   <--- here
142
143  private:
144    JSContext *cx;
145    js::Vector<uint64_t> buf;

The stack allocation seems to happen here:

315 bool
316 WriteStructuredClone(JSContext *cx, HandleValue v, uint64_t **bufp, size_t *nbytesp,
317                     const JSStructuredCloneCallbacks *cb, void *cbClosure,
318                     jsval transferable)
319 {
320     SCOutput out(cx);  <==== This seems to be the culprit.
321     JSStructuredCloneWriter w(out, cb, cbClosure, transferable);
322     return w.init() && w.write(v) && out.extractBuffer(bufp, nbytesp);
323 }

Analysis/Flow.

    So on line 320:  |out| in |WriteStructuredClone()|  is initialized with
    |cx|. 

    Durinng the initializer invocation for |out|, online 545, |buf| is
    set to |cx|.

    Then eventually, when destructor ~SCOutput() is called,
    |buf.begin()+ points to a 64 bit data which is undefined 

    This indicates that the first time WriteStructuredClone() is
    called the first 64bit of |cx| is not properly initialized, (and
    it is not initialized for the life time of the buffer area until
    |Discard()| is called?)?

   
TIA
>about 48 hours ago,

Make it 'yesterday'. Sorry about the confusion.

> 545 SCOutput::SCOutput(JSContext *cx) : cx(cx), buf(cx) {}

I know nothing about the code, but |buf(cx)| looks suspicious.
|buf(cx.some_member_of_cx)| is more like it. No?

TIA
buf is a Vector. The constructor argument cx only sets up an allocator for the Vector.

Steve can have this one. :)
Yeah, this is me. I haven't looked closely, but this sounds real. Might just be a leak if the uninit area happens to make it think the buffer has already been discarded. Might be worse than that.
(In reply to Steve Fink [:sfink] from comment #3)
> Yeah, this is me. I haven't looked closely, but this sounds real. Might just
> be a leak if the uninit area happens to make it think the buffer has already
> been discarded. Might be worse than that.

Obviously, this is reported during many tests of |make mozmill| since the code seems to
be in the hear of  VM of JavaScript. I can try memcheck run if you can post a work-in-progress patch.

TIA
Duplicate of this bug: 927936
Argh, sorry. I accidentally folded up the fix for this with a pending patch for bug 912456. I'll split it out here.
Do not attempt to read the header of an empty buffer.
Attachment #832527 - Flags: review?(jorendorff)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Duplicate of this bug: 938729
Duplicate of this bug: 937346
Duplicate of this bug: 936183
Duplicate of this bug: 933948
Attachment #832527 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/ac08f00a8ebc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.