Closed Bug 741821 Opened 8 years ago Closed 8 years ago

Don't access a null sql stmt

Categories

(Toolkit :: Storage, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch don't crash (obsolete) — Splinter Review
I have noticed this crash on try:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10577572&tree=Try&full=1#error1

What I think is happening is

* regress-617935.js makes us allocate a lot of memory
* another thread is calling sqlite3_prepare_v2 which indirectly calls sqlite3VdbeSetSql.
* sqlite3VdbeSetSql tries to init zSql by calling sqlite3DbStrNDup
* sqlite3DbStrNDup calls sqlite3DbMallocRaw which returns null
* the machine is still under load because of regress-617935.js, so we try to report a slow sql statement
* sqlite3_sql returns zSql which is null
* We create a nsDependentCString and then a nsACString with the null pointer. This crashes trying to compute its length.

I am amazed that the sqlite api is eating this error. Should we change it? This patch instead just avoids the crash :-(
Comment on attachment 611832 [details] [diff] [review]
don't crash

There isn't much we can do in general if we're out of memory, Firefox likely can't keep running. I'm ok with this patch just avoiding the segfault here.
Attachment #611832 - Flags: review?(vdjeric) → review+
OK, I will check this in. I reported the issue to sqlite in

http://thread.gmane.org/gmane.comp.db.sqlite.general/73439

and if they fix it we can revert this.
https://hg.mozilla.org/mozilla-central/rev/485381749e29
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Reopening since the problem was with us passing around a null stmt. The root problem was fixed in 743650.

A dummy try with an assert instead of an error code is at
https://tbpl.mozilla.org/?tree=Try&rev=efb3a59cfb65

I will attach the real patch in a sec.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch does 3 things:

*) Revert the previous "fix", it should never see a NULL
*) Add an assert about it, so that we don't need to get lucky with a timeout again to find this.
*) Fix our usage of sqlite3_prepare_v2 to not assume that an OK result means it produced an statement.

https://tbpl.mozilla.org/?tree=Try&rev=b4a35f4a1251
Assignee: nobody → respindola
Attachment #611832 - Attachment is obsolete: true
Attachment #613667 - Flags: review?(vdjeric)
Comment on attachment 613667 [details] [diff] [review]
fix usage of sqlite3_prepare_v2

Looks good to me but I don't know if I am the right reviewer since I'm not module owner/peer for storage :P
Attachment #613667 - Flags: review?(vdjeric) → review+
Comment on attachment 613667 [details] [diff] [review]
fix usage of sqlite3_prepare_v2

changing r? to sdwilsh.
Attachment #613667 - Flags: review+ → review?(sdwilsh)
Comment on attachment 613667 [details] [diff] [review]
fix usage of sqlite3_prepare_v2

Lets try Andrew Sutherland
Attachment #613667 - Flags: review?(sdwilsh) → review?(bugmail)
Attachment #613667 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/c3a3ef4f4f2e

(Merged by Ed Morley)
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.