Closed
Bug 1096054
Opened 10 years ago
Closed 10 years ago
Uninitialised value use in Interpret(JSContext*, js::RunState&)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jseward, Assigned: jseward)
Details
Attachments
(1 file)
1.95 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•10 years ago
|
||
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); }
Comment 2•10 years ago
|
||
Jason/Waldo, thoughts on Julian's analysis?
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Comment 3•10 years ago
|
||
This may go back to 4925a84c57cf, bug 858677. Not sure. I also wonder if those |if (!succeeded)| checks should be |if (!*succeeded)|...
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8522030 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb902590c536
Comment 7•10 years ago
|
||
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.
Description
•