393952 - crash when I try to VACUUM (StorageUnicodeFunctions::likeFunction() should handle null aArgv[0] and aArgv[1])

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[(not reading, please use [e-mail] instead)]

crash when I try to VACUUM (should StorageUnicodeFunctions::likeFunction() handle null aArgv[0] and aArgv[1]?)

SQLITE_PRIVATE const void *sqlite3ValueText(sqlite3_value* pVal, u8 enc){
  if( !pVal ) return 0;
  assert( (enc&3)==(enc&~SQLITE_UTF16_ALIGNED) );

  if( pVal->flags&MEM_Null ){
    return 0;
  }

is returning 0, and so we crash here:

>	strgcmps.dll!nsCharTraits<unsigned short>::length(const unsigned short * s=0x00000000)  Line 369 + 0x3 bytes	C++
 	strgcmps.dll!nsDependentString::nsDependentString(const unsigned short * data=0x00000000)  Line 90 + 0x12 bytes	C++
 	strgcmps.dll!StorageUnicodeFunctions::likeFunction(sqlite3_context * p=0x0012eea4, int aArgc=2, Mem * * aArgv=0x052615e8)  Line 192	C++
 	sqlite3.dll!sqlite3VdbeExec(Vdbe * p=0x051b1f48)  Line 34808 + 0x20 bytes	C
 	sqlite3.dll!sqlite3Step(Vdbe * p=0x051b1f48)  Line 32853 + 0x9 bytes	C
 	sqlite3.dll!sqlite3_step(sqlite3_stmt * pStmt=0x051b1f48)  Line 32909 + 0x9 bytes	C
 	sqlite3.dll!execExecSql(sqlite3 * db=0x01bfd358, const char * zSql=0x035fe4f0)  Line 58938 + 0x9 bytes	C
 	sqlite3.dll!sqlite3RunVacuum(char * * pzErrMsg=0x0495b7ec, sqlite3 * db=0x01bfd358)  Line 59035 + 0xe bytes	C
 	sqlite3.dll!sqlite3VdbeExec(Vdbe * p=0x0495b5c0)  Line 38186 + 0x13 bytes	C
 	sqlite3.dll!sqlite3Step(Vdbe * p=0x0495b5c0)  Line 32853 + 0x9 bytes	C
 	sqlite3.dll!sqlite3_step(sqlite3_stmt * pStmt=0x0495b5c0)  Line 32909 + 0x9 bytes	C
 	sqlite3.dll!sqlite3_exec(sqlite3 * db=0x01bfd358, const char * zSql=0x01d74410, int (void *, int, char * *, char * *)* xCallback=0x00000000, void * pArg=0x00000000, char * * pzErrMsg=0x00000000)  Line 51055 + 0x9 bytes	C
 	strgcmps.dll!mozStorageConnection::ExecuteSimpleSQL(const nsACString_internal & aSQLStatement={...})  Line 296 + 0x2a bytes	C++
 	places.dll!nsNavHistory::PerformVacuumIfIdle()  Line 3358 + 0x35 bytes	C++
 	places.dll!nsNavHistory::VacuumTimerCallback(nsITimer * aTimer=0x02dc8d80, void * aClosure=0x01b621f8)  Line 3371	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 384 + 0x13 bytes	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 459	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fa00)  Line 491	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bcf1b8, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00bc9688, const nsXREAppData * aAppData=0x00bc9aa8)  Line 3069 + 0x25 bytes	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00bc9688)  Line 153 + 0x12 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]

Comment 1

[(not reading, please use [e-mail] instead)]

see http://lxr.mozilla.org/seamonkey/source/db/sqlite3/src/sqlite3.c#48475, where the sqlite implementation checks "if( zA && zB ){"

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 278515 [details] [diff] [review]
patch

>+  if (!sqlite3_value_text16(aArgv[0]) || !sqlite3_value_text16(aArgv[1]))
>+    return;
>+
>   nsDependentString A(static_cast<const PRUnichar *>(sqlite3_value_text16(aArgv[1])));
>   nsDependentString B(static_cast<const PRUnichar *>(sqlite3_value_text16(aArgv[0])));
>   NS_ASSERTION(!B.IsEmpty(), "LIKE string must not be null!");
Is this assertion being triggered?  I would expect it to be.

Also, instead of calling sqlite3_value_text16 multiple times, can we just check A and B?  (we'd still want the assertion)

Lastly, can this crash be reproduced with an xpcshell testcase?

Comment 4

[(not reading, please use [e-mail] instead)]

> Is this assertion being triggered?  I would expect it to be.

no because we crash before we get there.

> Lastly, can this crash be reproduced with an xpcshell testcase?

not yet.  (I tried a simple test, and no luck.)  there might be something special about my places.sqlite

Comment 5

[(not reading, please use [e-mail] instead)]

to clarify:  aArgv[0] and aArgv[1] are not null, but sqlite3ValueText() returns null for them.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 278515 [details] [diff] [review]
patch

So we are crashing when assigning them to the string.  Sounds like a string bug too ;)

r=sdwilsh
It'd be awesome if you can come up with a test, but I'm not to concerned if you cannot.  This probably needs approval, but I'm not 100% sure...

Comment 7

[(not reading, please use [e-mail] instead)]

looking at the crash, here's the statement that is causing the crash:

SELECT 'CREATE INDEX vacuum_db.' || substr(sql,14,100000000)  FROM sqlite_master WHERE sql LIKE 'CREATE INDEX %'

reducing to:

select * from sqlite_master WHERE sql LIKE 'CREATE INDEX %'

I can see it is the sqlite_master rows with out any sql data.

specifically:

select * FROM sqlite_master WHERE sql is null

shows me some auto indexes.

working on a unit test.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: unit test, this will crash without the fix.

Comment 9

[(not reading, please use [e-mail] instead)]

fixed.

Checking in mozStorageUnicodeFunctions.cpp;
/cvsroot/mozilla/storage/src/mozStorageUnicodeFunctions.cpp,v  <--  mozStorageUn
icodeFunctions.cpp
new revision: 1.3; previous revision: 1.2
done

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

not a string bug, passing null there isn't legal.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 278529 [details] [diff] [review]
unit test, this will crash without the fix.

>+// This file tests our LIKE implementation since we override it for unicode
This isn't quite correct - please update.

Otherwise, r=sdwilsh.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: unit test (as checked in)

Comment 13

[(not reading, please use [e-mail] instead)]

> This isn't quite correct - please update.

thanks for catching my copy-and-paste mistake.  I've fixed it before checking in.

Checking in test_bug-393952.js;
/cvsroot/mozilla/storage/test/unit/test_bug-393952.js,v  <--  test_bug-393952.js

initial revision: 1.1
done