365166 - crash [@ strlen] calling mozIStorageStatement::getColumnName of a statement created with "PRAGMA user_version" or "PRAGMA schema_version"

Status: RESOLVED FIXED

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

Description

[Alexander M Beedie]

User-Agent:       Opera/9.10 (Windows NT 5.1; U; en)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

firefox crashes immediately and unrecoverably with the two sqlite version PRAGMA queries (user_version and schema_version) if you call the created statement's getColumnName() method on the results

Reproducible: Always

Steps to Reproduce:
sample code:

----------------------------------
var sql = "PRAGMA schema_version";
var storage_svc = Components.classes["@mozilla.org/storage/service;1"].getService(Components.interfaces.mozIStorageService);
var conn = storage_svc.openDatabase(ANY_SQLITE_DB);
var statement = conn.createStatement(sql);
var data = [];
while (statement.executeStep()) {
  var row = [];
  for (var i=0,k=statement.columnCount; i<k; i++) {
    row[statement.getColumnName(i)] = statement.getUTF8String(i);
  }
  data.push(row);
}
----------------------------------

stepping in via the venkman debugger i can see that the specific part crashing firefox is "statement.getColumnName(i)". 
Actual Results:  
firefox crashes unrecoverably

Expected Results:  
a result for the sqlite schema_version or user_version PRAGMA commands

the crash is severe, and could possibly be indicative of some nasty edge case lower down in the sqlite wrapper. still, i guess not too many people are routinely calling PRAGMA commands. 

(i've put a workaround in my own extension to explicitly detect and handle these sql constructions separately)

many thanks,

-alex

Comment 1

[Alexander M Beedie]

note: can't seem to submit bugs to the Storage component from the bugzilla front page, hence putting it in General (apologies)

Comment 2

[Alexander M Beedie]

Attachment: working code that reproduces the crash

ZIP containing one XUL file and one JS file. unzip them to any dir, drag the XUL file onto firefox to open it. press one of the two buttons on the displayed page to see the crash.

note: UniversalXPConnect security manager is called so that the storage service can be called from a file:// uri, so make sure you "allow" that. (take a look at the JS to confirm i'm doing nothing sinister!)

Comment 3

[Nickolay_Ponomarev]

Thanks for the testcase.

The crash happens at http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/storage/src/mozStorageStatement.cpp&rev=1.15&mark=255#246 because cname is null. sqlite docs are not very clear on this:

"The first argument is a prepared SQL statement. This function returns the column heading for the Nth column of that statement, where N is the second function argument. The string returned is UTF-8 for sqlite3_column_name() and UTF-16 for sqlite3_column_name16()."

Comment 4

[Nickolay_Ponomarev]

Attachment: simplified testcase (crashes onload; open locally)

Comment 5

[Nickolay_Ponomarev]

Apparently it's similar to bug 333306.

Comment 6

[Nickolay_Ponomarev]

okay, opened http://www.sqlite.org/cvstrac/tktview?tn=2143 for sqlite docs and I think we should just use mColumnNames instead of asking sqlite about the column name again.

Comment 7

[Nickolay_Ponomarev]

Attachment: minimal patch

So this just adds the necessary null-checks and an xpcshell testcase (I also fixed a similar crasher in mozStorageStatementWrapper).

Looking at the code I find it weird that everyone tries to cache the column names.
http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=mColumnNames

I think we can remove the mColumnNames member from statement classes, and just get the column names in mozStorageStatementWrapper::GetRow and mozStorageStatement::GetColumnName.

Comment 8

[Nickolay_Ponomarev]

Hm, the ticket I opened against sqlite is fixed already, they made sqlite3_column_name return a proper string for these queries. So if we want this on branches, the above patch can be used, otherwise we can wait for the next version of sqlite to be landed on trunk.

Or we can be overly cautious and add the null-check anyway..

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 250520 [details] [diff] [review]
minimal patch

If you could kindly un-bit rot this, I'll gladly review it.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 250520 [details] [diff] [review]
minimal patch

Actually, this looks good - I just need something that applies still to check in :)

Comment 11

[Nickolay_Ponomarev]

Comment on attachment 250520 [details] [diff] [review]
minimal patch

We probably should wait for bug 341137 to get fixed, then just check in the testcase and remove the existing null-checks of sqlite3_column_name.

Comment 12

[Shawn Wilsher :sdwilsh]

We have until July 18th to land this.  I'm willing to drop whatever it is that I'm working on to get the review done to make sure it gets in (or loose sleep, which ever is less important)

Nickolay, could you get an updated patch now that we've upgraded please?

Comment 13

[Nickolay_Ponomarev]

Attachment: patch: remove unnecessary null-check and add a testcase now that the crash is fixed upstream

Comment 14

[Nickolay_Ponomarev]

Since the crash itself was already fixed upstream, this is not something that should be blocking, imo.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 270499 [details] [diff] [review]
patch: remove unnecessary null-check and add a testcase now that the crash is fixed upstream

r=sdwilsh.  Thanks!

Comment 16

[Shawn Wilsher :sdwilsh]

Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.18; previous revision: 1.17
Checking in storage/test/unit/test_bug-365166.js;
initial revision: 1.1