Closed
Bug 912935
Opened 11 years ago
Closed 11 years ago
mWALModeEnable is used uninitialized (in thunderbird)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: ishikawa, Assigned: ishikawa)
Details
Attachments
(1 file, 2 obsolete files)
I found that a member variable for DOMStorageDBThread, mWALModeEnabled is used uninitialized during the execution of DEBUG BUILD of thunderbird under valgrind. I think proper patch would be something like the attached, but I am not sure. The use of valgrind slows down the execution of the program quite a bit, thus widens the windows of vulnerability between the time an object is created and its members get properly initialized. So maybe there are some thread-race issues that may use mWALModeEnabled before proper setting. Although MWALModeEnabled is set in TryJournalMode(), mWALModeEnabled gets referenced by SyncPreload before that, I think under some conditions. This happened only once in the run of |make mozmill| test suite of thunderbird under 64 bit Debian GNU/Linux. It may or not may happen in other runs (due to the nature of thread races). TIA PS: If I put MWALModeEnabled(false) before mDBREady(false) [above the current position in the attached pach ], I get a warning about initialization reorder. A good reminder that the initialization is done in the declaration order.
Assignee | ||
Updated•11 years ago
|
Attachment #800064 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 1•11 years ago
|
||
Sorry, here is the excerpt of the session log that shows the uninitialized use : Line 145 is DOMStorageDBThread.cpp is 142 // Bypass sync load when an update is pending in the queue to write, we would 143 // get incosistent data in the cache. Also don't allow sync main-thread preload 144 // when DB open and init is still pending on the background thread. 145 if (mWALModeEnabled && mDBReady) { 146 bool pendingTasks; mDBReady is already initialized to false upon creation. mWALModeEnabled was not. It is only set within the call to TryJournalMode(). ==31341== Conditional jump or move depends on uninitialised value(s) ==31341== at 0x7A751C2: mozilla::dom::DOMStorageDBThread::SyncPreload(mozilla::dom::DOMStorageCacheBridge*, bool) (DOMStorageDBThread.cpp:145) ==31341== by 0x7A697C4: mozilla::dom::DOMStorageCache::WaitForPreload(mozilla::Telemetry::ID) (DOMStorageCache.cpp:345) ==31341== by 0x7A69F9D: mozilla::dom::DOMStorageCache::GetItem(mozilla::dom::DOMStorage const*, nsAString_internal const&, nsAString_internal&) (DOMStorageCache.cpp:452) ==31341== by 0x7A68155: mozilla::dom::DOMStorage::GetItem(nsAString_internal const&, nsAString_internal&) (DOMStorage.cpp:90) ==31341== by 0x7FA4B81: nsIDOMStorage_GetItem(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:693) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489) ==31341== by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== by 0xA07A6E1: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1026) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489) ==31341== by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539) ==31341== by 0xA00586F: JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) (jsapi.cpp:5411) ==31341== by 0x7F7EBF2: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJSClass.cpp:1445) ==31341== by 0x7F76678: nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJS.cpp:587) ==31341== by 0x9503AE9: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:122) ==31341== by 0x9502EF6: SharedStub (in /TEST-MAIL-DIR/objdir-tb3/mozilla/toolkit/library/libxul.so) ==31341== by 0x763CE29: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener, nsIDOMEventListener> const&, nsIDOMEvent*, mozilla::dom::EventTarget*, nsCxPusher*) (nsEventListenerManager.cpp:977) ==31341== by 0x763D14A: nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) (nsEventListenerManager.cpp:1048) ==31341== by 0x763532D: nsEventTargetChainItem::HandleEventTargetChain(nsTArray<nsEventTargetChainItem>&, nsEventChainPostVisitor&, nsDispatchingCallback*, ELMCreationDetector&, nsCxPusher*) (nsEventListenerManager.h:330) ==31341== by 0x7638265: nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) (nsEventDispatcher.cpp:605) ==31341== by 0x7638E0F: nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) (nsEventDispatcher.cpp:669) ==31341== by 0x75260CF: nsINode::DispatchEvent(nsIDOMEvent*, bool*) (nsINode.cpp:1140) ==31341== by 0x746A427: nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) (nsContentUtils.cpp:3348) ==31341== by 0x746A511: nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) (nsContentUtils.cpp:3319) ==31341== by 0x74C1B41: nsDocument::DispatchContentLoadedEvents() (nsDocument.cpp:4639) ==31341== by 0x74DE6F3: nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() (nsThreadUtils.h:363) ==31341== by 0x94D6CDC: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622) ==31341== by 0x94674B5: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238) ==31341== by 0x80E5F51: nsXULWindow::ShowModal() (nsXULWindow.cpp:365) ==31341== by 0x80D3335: nsContentTreeOwner::ShowAsModal() (nsContentTreeOwner.cpp:533) ==31341== by 0x8084176: nsWindowWatcher::OpenWindowInternal(nsIDOMWindow*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, nsIDOMWindow**) (nsWindowWatcher.cpp:998) ==31341== by 0x8084D7F: nsWindowWatcher::OpenWindow2(nsIDOMWindow*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, nsIDOMWindow**) (nsWindowWatcher.cpp:416) ==31341== by 0x797202D: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIPrincipal*, JSContext*, nsIDOMWindow**) (nsGlobalWindow.cpp:10086) ==31341== by 0x7971666: nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIPrincipal*, JSContext*, nsIDOMWindow**) (nsGlobalWindow.cpp:9969) ==31341== by 0x7972F39: nsGlobalWindow::OpenDialog(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMWindow**) (nsGlobalWindow.cpp:6530) ==31341== by 0x9502DC6: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164) ==31341== by 0x7F8D930: CallMethodHelper::Call() (XPCWrappedNative.cpp:2809) ==31341== by 0x7F89C58: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2115) ==31341== by 0x7F9928E: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1316) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489) ==31341== by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== Uninitialised value was created by a heap allocation ==31341== at 0x402A914: malloc (vg_replace_malloc.c:291) ==31341== by 0x404633B: moz_xmalloc (mozalloc.cpp:54) ==31341== by 0x7A6ABF3: mozilla::dom::DOMStorageCache::StartDatabase() (mozalloc.h:201) ==31341== by 0x7A6AD2B: mozilla::dom::DOMStorageCache::Preload() (DOMStorageCache.cpp:230) ==31341== by 0x7A6AF18: mozilla::dom::DOMStorageCache::Init(mozilla::dom::DOMStorageManager*, bool, nsIPrincipal*, nsACString_internal const&) (DOMStorageCache.cpp:137) ==31341== by 0x7A7BB41: mozilla::dom::DOMStorageManager::PutCache(nsACString_internal const&, nsIPrincipal*) (DOMStorageManager.cpp:291) ==31341== by 0x7A7C5AD: mozilla::dom::DOMStorageManager::GetStorageInternal(bool, nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:352) ==31341== by 0x7A7C734: mozilla::dom::DOMStorageManager::CreateStorage(nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:379) ==31341== by 0x7A793F8: mozilla::dom::DOMStorageManager::GetLocalStorageForPrincipal(nsIPrincipal*, nsAString_internal const&, bool, nsIDOMStorage**) (DOMStorageManager.cpp:465) ==31341== by 0x9502DC6: NS_InvokeByIndex (xptcinvoke_x86_64_unix.cpp:164) ==31341== by 0x7F8D930: CallMethodHelper::Call() (XPCWrappedNative.cpp:2809) ==31341== by 0x7F89C58: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2115) ==31341== by 0x7F9928E: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1316) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489) ==31341== by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== by 0xA07A1D1: js_fun_call(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:873) ==31341== by 0xA07A60D: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:912) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC4F90: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:489) ==31341== by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539) ==31341== by 0xA12201A: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:445) ==31341== by 0xA18256E: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jswrapper.cpp:438) ==31341== by 0xA122B2C: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:2626) ==31341== by 0xA122C64: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3203) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC50B3: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:482) ==31341== by 0x9ECBCA4: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2484) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539) ==31341== by 0xA12201A: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:445) ==31341== by 0xA18256E: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jswrapper.cpp:438) ==31341== by 0xA122B2C: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (jsproxy.cpp:2626) ==31341== by 0xA122C64: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3203) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219) ==31341== by 0x9EC50B3: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:482) ==31341== by 0x9ECF3BF: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:539) ==31341== by 0x9ECF5C0: js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:610) ==31341== by 0xA0E2585: 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, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (Shape-inl.h:256) ==31341== by 0xA0E33BF: 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, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (jsobj.cpp:4242) ==31341== by 0xA0E3853: js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>) (jsobj.cpp:4251) ==31341== by 0x9ED6A39: GetPropertyOperation(JSContext*, js::StackFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) (Interpreter.cpp:292) ==31341== by 0x9EC77C8: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2323) ==31341== by 0x9ECE813: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:446) ==31341== by 0x9EC4FA4: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:508) ==31341== by 0xA07A6E1: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1026) ==31341== by 0x9ED1A76: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:219)
Comment 2•11 years ago
|
||
Comment on attachment 800064 [details] [diff] [review] initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() I think the more proper way is to reverse the condition if (mWALModeEnabled && mDBReady) { to if (mDBReady && mWALModeEnabled) {
Attachment #800064 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 3•11 years ago
|
||
Here is an updated patch. I think it doesn't hurt to initialize the value upon creation. So I keep the initialization, and changed the order of variable accesses in the if expression. TIA
Attachment #802094 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•11 years ago
|
Attachment #800064 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ishikawa
Assignee | ||
Updated•11 years ago
|
Attachment #802094 -
Attachment description: initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. → initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Two)
Comment 4•11 years ago
|
||
Comment on attachment 802094 [details] [diff] [review] initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Two) Review of attachment 802094 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: dom/src/storage/DOMStorageDBThread.cpp @@ +57,5 @@ > DOMStorageDBThread::DOMStorageDBThread() > : mThread(nullptr) > , mMonitor("DOMStorageThreadMonitor") > , mStopIOThread(false) > +, mWALModeEnabled(false) // valgrind caught uninitialized usage. I think you don't need the comment.
Attachment #802094 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Thank you for the review. I removed the comment. I will put the checkin-needed keyword.
Assignee | ||
Updated•11 years ago
|
Attachment #802164 -
Attachment description: initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Three) [removed unnecessary comment] → initialize mWALModeEnabled to false upon creation of DOMStorageDBThread::DOMStorageDBThread() and also reorder of access in an if-expression. (Take Three) [removed unnecessary comment] (already review+)
Assignee | ||
Updated•11 years ago
|
Attachment #802094 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/67dfa713bd14
Keywords: checkin-needed
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/67dfa713bd14
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•