Closed Bug 1096054 Opened 10 years ago Closed 10 years ago

Uninitialised value use in Interpret(JSContext*, js::RunState&)

Categories

(Core :: JavaScript Engine, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(1 file)

dom/plugins/test/mochitest/test_propertyAndMethod.html

As-yet undiagnosed, but I can reproduce this in both 32- and 64-bit
builds, which increases the likelyhood of it being real.

Conditional jump or move depends on uninitialised value(s)
   at 0x81E6326: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2277)
   by 0x81EAE44: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:432)
   by 0x81EB57E: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/vm/Interpreter.cpp:501)
   by 0x81EC1B2: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (js/src/vm/Interpreter.cpp:538)
   by 0x80FDC1A: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (js/src/jsapi.cpp:5013)
   by 0x66A5827: mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) (ff-O2-linux64/dom/bindings/EventHandlerBinding.cpp:259)
   by 0x6C28564: void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) (ff-O2-linux64/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:350)
   by 0x6C20151: mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) (dom/events/JSEventHandler.cpp:214)
   by 0x6C0AA55: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (dom/events/EventListenerManager.cpp:964)
   by 0x6C10001: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (dom/events/EventListenerManager.cpp:1079)
   by 0x6C10396: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (dom/events/EventDispatcher.cpp:293)
   by 0x6C113CA: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) (dom/events/EventDispatcher.cpp:607)
   by 0x72B4257: nsDocumentViewer::LoadComplete(tag_nsresult) (layout/base/nsDocumentViewer.cpp:1005)
   by 0x761BF0F: nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) [clone .part.238] (docshell/base/nsDocShell.cpp:7350)
   by 0x761D9D0: nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) (docshell/base/nsDocShell.cpp:7168)
   by 0x6090073: nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) (uriloader/base/nsDocLoader.cpp:1269)

 Uninitialised value was created by a stack allocation
   at 0x81E1153: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:1327)
The complaint occurs in this fragment, beginning at Interpreter.cpp:2274:

    bool succeeded;
    if (!JSObject::deleteGeneric(cx, obj, id, &succeeded))
        goto error;
    if (!succeeded && script->strict()) {  <--- HERE
        obj->reportNotConfigurable(cx, id);
        goto error;
    }

This assumes that if JSObject::deleteGeneric returns true then it also
writes to |succeeded|, but that doesn't always appear to be the case.
deleteGeneric is at jsobjinlines.h:40:

    js::DeleteGenericOp op = obj->getOps()->deleteGeneric;
    if (op)
        return op(cx, obj, id, succeeded);
    return js::baseops::DeleteGeneric(cx, obj.as<js::NativeObject>(), id, succeeded);

In this case |op| is NULL so the call is passed through unchanged to 
js::baseops::DeleteGeneric, and that's where the problem appears to be.

In this test, the final return

    return obj->removeProperty(cx, id) && SuppressDeletedProperty(cx, obj, id);

can (evidently) return true without writing anything to *succeeded, and I
verified this is indeed the case using the patch below [1], which stops V
complaining.

I think there is a second path that can lead to the same result, in which
the function is returned from thusly:

        ...
        nobj->setDenseElementHole(cx, JSID_TO_INT(id));
        return SuppressDeletedProperty(cx, obj, id);


[1]
diff --git a/js/src/vm/NativeObject.cpp b/js/src/vm/NativeObject.cpp
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -2316,5 +2316,6 @@ baseops::DeleteGeneric(JSContext *cx, Ha
     if (!succeeded)
         return true;
 
+    *succeeded = true;
     return obj->removeProperty(cx, id) && SuppressDeletedProperty(cx, obj, id);
 }
Jason/Waldo, thoughts on Julian's analysis?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
This may go back to 4925a84c57cf, bug 858677. Not sure.

I also wonder if those |if (!succeeded)| checks should be |if (!*succeeded)|...
(In reply to Jan de Mooij [:jandem] from comment #3)
> I also wonder if those |if (!succeeded)| checks should be |if
> (!*succeeded)|...

Yeah, I also wondered that.  Particularly given that there are paths
in DeleteGeneric which simply assume |succeeded| is non-null and will
segfault if it is, which makes later "if (!succeeded)" tests pointless.
Changing two instances of |if (!succeeded)| to |if (!*succeeded)|
tidies up baseops::DeleteGeneric.  But the problem then reappears in
one of the callees of CallJSDeletePropertyOp, namely
NPObjWrapper_DelProperty, which can also return true without writing
to *succeeded.  Attached patch fixes both.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Attachment #8522030 - Flags: review?(jwalden+bmo)
Attachment #8522030 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/bb902590c536
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.