463907 - mozStorageConnection::ExecuteAsync does not check that provided statements have not been finalized

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Rain [:rain1] (inactive)]

I shut Thunderbird down in the middle of Gloda indexing, and it crashed with a null pointer dereference in sqlite3.c.

The point where it crashed: http://mxr.mozilla.org/comm-central/source/mozilla/storage/src/mozStorageConnection.cpp#391 -- inside sqlite3_sql (I'm not linking to sqlite3.c :) ). old_stmt is null.

Stack at the time of the crash:
 	sqlite3.dll!sqlite3_sql(sqlite3_stmt * pStmt=0x00000000)  Line 42099 + 0x3 bytes	C
>	strgcmps.dll!mozStorageConnection::ExecuteAsync(mozIStorageStatement * * aStatements=0x0018b9a4, unsigned int aNumStatements=1, mozIStorageStatementCallback * aCallback=0x05cdb258, mozIStoragePendingStatement * * _stmt=0x0018bb58)  Line 392 + 0x11 bytes	C++
 	strgcmps.dll!mozStorageStatement::ExecuteAsync(mozIStorageStatementCallback * aCallback=0x05cdb258, mozIStoragePendingStatement * * _stmt=0x0018bb58)  Line 582	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000022, unsigned int methodIndex=2, unsigned int paramCount=1620808, nsXPTCVariant * params=0x0018ba20)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2405 + 0x21 bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x0044b2e8, JSObject * obj=0x05431660, unsigned int argc=1, long * argv=0x04b444e0, long * vp=0x0018be14)  Line 1477 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x0044b2e8, unsigned int argc=1, long * vp=0x04b444d8, unsigned int flags=2)  Line 1306 + 0x1a bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x0044b2e8)  Line 5015 + 0x16 bytes	C++
 	js3250.dll!SendToGenerator(JSContext * cx=0x0044b2e8, JSGeneratorOp op=JSGENOP_NEXT, JSObject * obj=0x0ee62a80, JSGenerator * gen=0x04b31ad0, long arg=22)  Line 874 + 0x9 bytes	C++
 	js3250.dll!generator_op(JSContext * cx=0x0044b2e8, JSGeneratorOp op=JSGENOP_NEXT, long * vp=0x04b44628, unsigned int argc=0)  Line 987 + 0x19 bytes	C++
 	js3250.dll!generator_next(JSContext * cx=0x0044b2e8, unsigned int argc=0, long * vp=0x04b44628)  Line 1002 + 0x13 bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x0044b2e8)  Line 4998 + 0x17 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x0044b2e8, unsigned int argc=1, long * vp=0x04b4477c, unsigned int flags=0)  Line 1324 + 0x9 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x0b67aff0, unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x03c03b10, nsXPTCMiniVariant * nativeParams=0x0018de10)  Line 1549 + 0x1b bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x03c03b10, nsXPTCMiniVariant * params=0x0018de10)  Line 564	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0bb514a0, unsigned int methodIndex=3, unsigned int * args=0x0018ded0, unsigned int * stackBytesToPop=0x0018dec0)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 424	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 514	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0018df84)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0036ab00, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	xpcom_core.dll!nsThread::Shutdown()  Line 465 + 0xb bytes	C++
 	strgcmps.dll!mozStorageConnection::Close()  Line 237	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000003, unsigned int methodIndex=0, unsigned int paramCount=1630716, nsXPTCVariant * params=0x0018e0b8)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2405 + 0x21 bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x0044b2e8, JSObject * obj=0x03fd29a0, unsigned int argc=0, long * argv=0x04b44770, long * vp=0x0018e4c8)  Line 1477 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x0044b2e8, unsigned int argc=0, long * vp=0x04b44768, unsigned int flags=2)  Line 1306 + 0x1a bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x0044b2e8)  Line 5015 + 0x16 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x0044b2e8, unsigned int argc=1, long * vp=0x05c65080, unsigned int flags=0)  Line 1324 + 0x9 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x0ceb8468, unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x04b9d138, nsXPTCMiniVariant * nativeParams=0x0018f6c8)  Line 1549 + 0x1b bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x04b9d138, nsXPTCMiniVariant * params=0x0018f6c8)  Line 564	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0bb51628, unsigned int methodIndex=5, unsigned int * args=0x0018f788, unsigned int * stackBytesToPop=0x0018f778)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	strgcmps.dll!CompletionNotifier::Run()  Line 191	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0018f7e0)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0036ab00, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 192 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00360fa8, const nsXREAppData * aAppData=0x00364f70)  Line 3263 + 0x25 bytes	C++
 	thunderbird.exe!NS_internal_main(int argc=1, char * * argv=0x00360fa8)  Line 103 + 0x12 bytes	C++
 	thunderbird.exe!wmain(int argc=1, wchar_t * * argv=0x00361d40)  Line 87 + 0xd bytes	C++
 	thunderbird.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	thunderbird.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0xe bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x23 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

JS stack at the time of the crash:
0 [native frame]
1 gloda_ds_beginTransaction() ["file:///D:/mozilla-build/msys/comm-central/obj-i
686-pc-mingw32/mozilla/dist/bin/modules/datastore.js":1102]
    this = [object Object]
2 gloda_index_workBatch() ["file:///D:/mozilla-build/msys/comm-central/obj-i686-
pc-mingw32/mozilla/dist/bin/modules/indexer.js":1036]
    commitTokens = 10
    this = [object Object]
3 gloda_index_callbackDriver() ["file:///D:/mozilla-build/msys/comm-central/obj-
i686-pc-mingw32/mozilla/dist/bin/modules/indexer.js":935]
    this = [object Object]
4 gloda_index_wrapCallbackDriver([xpconnect wrapped nsITimer @ 0x4d6e4b0 (native
 @ 0x4d6e448)]) ["file:///D:/mozilla-build/msys/comm-central/obj-i686-pc-mingw32
/mozilla/dist/bin/modules/indexer.js":876]
    this = function gloda_index_wrapCallbackDriver() {
    GlodaIndexer.callbackDriver();
}

The gloda dump log just before the stack:
2008-11-09 21:42:47     gloda.indexer   INFO    Shutting Down
2008-11-09 21:42:47     gloda.indexer   INFO    Event-Driven Indexing is now fal
se
--WEBSHELL 003CABB8 == 2
2008-11-09 21:42:47     gloda.datastore INFO    Closing db connection
###!!! ASSERTION: Statement must be from this database connection!: 'sqlite3_db_
handle(old_stmt) == mDBConn', file D:/mozilla-build/msys/comm-central/src/mozill
a/storage/src/mozStorageConnection.cpp, line 385
0 [native frame]

I assume the assertion is because the old_stmt is null.

I haven't been able to reliably reproduce the crash, unfortunately. It just seems to happen if I shut Tb down in the middle of indexing.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Right, so a statement which has had finalize called on it is being passed to executeAsync.  This is clearly a bad thing for the code in question (gloda) to be doing, but storage should probably not crash, either.  Although the assertion is technically not a crash, if the assertion wasn't there, the call to sqlite3_sql(NULL) would attempt to deref the NULL, which would be (a crash).

It looks like the wrapper helpers use the potentially dangerous GetNativeStatementPointer too, but they are both safe for various reasons. mozStorageStatementRow is saved by a call to mozStorageStatement::GetColumnIndex.  mozStorageStatementParams' call to sqlite3_bind_parameter returns 0, which is properly treated as an error.
However, un the wrapper itself, mozStorageStatementWrapper::Initialize could get burnt by its failure to check return values and end up counting to 2^32, growing an nsStringArray as it goes.  I'll log a separate bug for that.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 bail on ExecuteAsync if we hit a statement sans native statement, unit test it [Checkin: Comment 6]

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #1)
> However, un the wrapper itself, mozStorageStatementWrapper::Initialize could
> get burnt by its failure to check return values and end up counting to 2^32,
> growing an nsStringArray as it goes.  I'll log a separate bug for that.

Logged bug 465299.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 348526 [details] [diff] [review]
v1 bail on ExecuteAsync if we hit a statement sans native statement, unit test it
[Checkin: Comment 6]

r=sdwilsh

Comment 5

[Shawn Wilsher :sdwilsh]

Ack - this has been ready to land for two months now

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 348526 [details] [diff] [review]
v1 bail on ExecuteAsync if we hit a statement sans native statement, unit test it
[Checkin: Comment 6]


http://hg.mozilla.org/mozilla-central/rev/39219f4ec810

Comment 7

[Frank Wein [:mcsmurf]]

Needed for Bug 483980 for 1.9.1 branch as this one looks like it fixes the crash on the tinderboxen as mentioned in Bug 483980 Comment 96.

Comment 8

[Frank Wein [:mcsmurf]]

Comment on attachment 348526 [details] [diff] [review]
v1 bail on ExecuteAsync if we hit a statement sans native statement, unit test it
[Checkin: Comment 6]

Requesting approval for 1.9.1.3 as this would probably fix the tinderbox crash as mentioned in Bug 483980 Comment 96. The risk of taking this patch should be small as it's only a null check and setting the appropriate return value in case it's null.

Comment 9

[Samuel Sidler (old account; do not CC)]

Comment on attachment 348526 [details] [diff] [review]
v1 bail on ExecuteAsync if we hit a statement sans native statement, unit test it
[Checkin: Comment 6]

Approved for 1.9.1.3. a=ss

In the future, can you at least try your patch on the tryserver where things like this are likely to be caught?

Comment 10

[Frank Wein [:mcsmurf]]

(In reply to comment #9)
> In the future, can you at least try your patch on the tryserver where things
> like this are likely to be caught?

I did not write it down in the bug, but I did test it on the try server. There the crash did not occur.

Comment 11

[Frank Wein [:mcsmurf]]

Pushed to 1.9.1 branch, http://hg.mozilla.org/releases/mozilla-1.9.1/rev/11268a3502e5