Skip to content

Commit

Permalink
Fix out-of-bounds read in statement tail parser (#996)
Browse files Browse the repository at this point in the history 
* Fix out-of-bounds read in statement tail parser

* Add comment with link to tail parsing issue

* Fix missing increment in tail parser + tests
  • Loading branch information
@arimah
arimah committed May 2, 2023
1 parent 2843e1f commit c37b7ee
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 51 deletions.
59 changes: 33 additions & 26 deletions src/better_sqlite3.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -859,20 +859,27 @@ void Statement::JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info)
if (handle == NULL) {
return ThrowRangeError("The supplied SQL string contains no statements");
}
for (char c; (c = *tail); ++tail) {
if (IS_SKIPPED(c)) continue;

for (char c; (c = *tail); ) {
if (IS_SKIPPED(c)) {
++tail;
continue;
}
if (c == '/' && tail[1] == '*') {
tail += 2;
for (char c; (c = *tail); ++tail) {
if (c == '*' && tail[1] == '/') {
tail += 1;
tail += 2;
break;
}
}
} else if (c == '-' && tail[1] == '-') {
tail += 2;
for (char c; (c = *tail); ++tail) {
if (c == '\n') break;
if (c == '\n') {
++tail;
break;
}
}
} else {
sqlite3_finalize(handle);
Expand All @@ -891,9 +898,9 @@ void Statement::JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info)

info.GetReturnValue().Set(info.This());
}
#line 149 "./src/objects/statement.lzz"
#line 156 "./src/objects/statement.lzz"
void Statement::JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 149 "./src/objects/statement.lzz"
#line 156 "./src/objects/statement.lzz"
{
Statement * stmt = node :: ObjectWrap :: Unwrap < Statement > ( info . This ( ) ) ; ( ( void ) 0 ) ; sqlite3_stmt * handle = stmt -> handle ; Database * db = stmt -> db ; if ( ! db -> GetState ( ) -> open ) return ThrowTypeError ( "The database connection is not open" ) ; if ( db -> GetState ( ) -> busy ) return ThrowTypeError ( "This database connection is busy executing a query" ) ; if ( stmt -> locked ) return ThrowTypeError ( "This statement is busy executing a query" ) ; if ( ! db -> GetState ( ) -> unsafe_mode ) { if ( db -> GetState ( ) -> iterators ) return ThrowTypeError ( "This database connection is busy executing a query" ) ; } ( ( void ) 0 ) ; const bool bound = stmt -> bound ; if ( ! bound ) { Binder binder ( handle ) ; if ( ! binder . Bind ( info , info . Length ( ) , stmt ) ) { sqlite3_clear_bindings ( handle ) ; return ; } ( ( void ) 0 ) ; } else if ( info . Length ( ) > 0 ) { return ThrowTypeError ( "This statement already has bound parameters" ) ; } ( ( void ) 0 ) ; db -> GetState ( ) -> busy = true ; v8 :: Isolate * isolate = info . GetIsolate ( ) ; if ( db -> Log ( isolate , handle ) ) { db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ; } ( ( void ) 0 ) ;
sqlite3* db_handle = db->GetHandle();
Expand All @@ -916,9 +923,9 @@ void Statement::JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info)
}
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
}
#line 172 "./src/objects/statement.lzz"
#line 179 "./src/objects/statement.lzz"
void Statement::JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 172 "./src/objects/statement.lzz"
#line 179 "./src/objects/statement.lzz"
{
Statement * stmt = node :: ObjectWrap :: Unwrap < Statement > ( info . This ( ) ) ; if ( ! stmt -> returns_data ) return ThrowTypeError ( "This statement does not return data. Use run() instead" ) ; sqlite3_stmt * handle = stmt -> handle ; Database * db = stmt -> db ; if ( ! db -> GetState ( ) -> open ) return ThrowTypeError ( "The database connection is not open" ) ; if ( db -> GetState ( ) -> busy ) return ThrowTypeError ( "This database connection is busy executing a query" ) ; if ( stmt -> locked ) return ThrowTypeError ( "This statement is busy executing a query" ) ; const bool bound = stmt -> bound ; if ( ! bound ) { Binder binder ( handle ) ; if ( ! binder . Bind ( info , info . Length ( ) , stmt ) ) { sqlite3_clear_bindings ( handle ) ; return ; } ( ( void ) 0 ) ; } else if ( info . Length ( ) > 0 ) { return ThrowTypeError ( "This statement already has bound parameters" ) ; } ( ( void ) 0 ) ; db -> GetState ( ) -> busy = true ; v8 :: Isolate * isolate = info . GetIsolate ( ) ; if ( db -> Log ( isolate , handle ) ) { db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ; } ( ( void ) 0 ) ;
int status = sqlite3_step(handle);
Expand All @@ -933,9 +940,9 @@ void Statement::JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info)
sqlite3_reset(handle);
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
}
#line 187 "./src/objects/statement.lzz"
#line 194 "./src/objects/statement.lzz"
void Statement::JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 187 "./src/objects/statement.lzz"
#line 194 "./src/objects/statement.lzz"
{
Statement * stmt = node :: ObjectWrap :: Unwrap < Statement > ( info . This ( ) ) ; if ( ! stmt -> returns_data ) return ThrowTypeError ( "This statement does not return data. Use run() instead" ) ; sqlite3_stmt * handle = stmt -> handle ; Database * db = stmt -> db ; if ( ! db -> GetState ( ) -> open ) return ThrowTypeError ( "The database connection is not open" ) ; if ( db -> GetState ( ) -> busy ) return ThrowTypeError ( "This database connection is busy executing a query" ) ; if ( stmt -> locked ) return ThrowTypeError ( "This statement is busy executing a query" ) ; const bool bound = stmt -> bound ; if ( ! bound ) { Binder binder ( handle ) ; if ( ! binder . Bind ( info , info . Length ( ) , stmt ) ) { sqlite3_clear_bindings ( handle ) ; return ; } ( ( void ) 0 ) ; } else if ( info . Length ( ) > 0 ) { return ThrowTypeError ( "This statement already has bound parameters" ) ; } ( ( void ) 0 ) ; db -> GetState ( ) -> busy = true ; v8 :: Isolate * isolate = info . GetIsolate ( ) ; if ( db -> Log ( isolate , handle ) ) { db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ; } ( ( void ) 0 ) ;
v8 :: Local < v8 :: Context > ctx = isolate -> GetCurrentContext ( ) ;
Expand All @@ -956,9 +963,9 @@ void Statement::JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info)
if (js_error) db->GetState()->was_js_error = true;
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
}
#line 208 "./src/objects/statement.lzz"
#line 215 "./src/objects/statement.lzz"
void Statement::JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 208 "./src/objects/statement.lzz"
#line 215 "./src/objects/statement.lzz"
{
Addon * addon = static_cast < Addon * > ( info . Data ( ) . As < v8 :: External > ( ) -> Value ( ) ) ;
v8 :: Isolate * isolate = info . GetIsolate ( ) ;
Expand All @@ -968,9 +975,9 @@ void Statement::JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info)
addon->privileged_info = NULL;
if (!maybeIterator.IsEmpty()) info.GetReturnValue().Set(maybeIterator.ToLocalChecked());
}
#line 218 "./src/objects/statement.lzz"
#line 225 "./src/objects/statement.lzz"
void Statement::JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 218 "./src/objects/statement.lzz"
#line 225 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if (stmt->bound) return ThrowTypeError("The bind() method can only be invoked once per statement object");
Expand All @@ -981,9 +988,9 @@ void Statement::JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info)
stmt->bound = true;
info.GetReturnValue().Set(info.This());
}
#line 229 "./src/objects/statement.lzz"
#line 236 "./src/objects/statement.lzz"
void Statement::JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 229 "./src/objects/statement.lzz"
#line 236 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if (!stmt->returns_data) return ThrowTypeError("The pluck() method is only for statements that return data");
Expand All @@ -994,9 +1001,9 @@ void Statement::JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info)
stmt->mode = use ? Data::PLUCK : stmt->mode == Data::PLUCK ? Data::FLAT : stmt->mode;
info.GetReturnValue().Set(info.This());
}
#line 240 "./src/objects/statement.lzz"
#line 247 "./src/objects/statement.lzz"
void Statement::JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 240 "./src/objects/statement.lzz"
#line 247 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if (!stmt->returns_data) return ThrowTypeError("The expand() method is only for statements that return data");
Expand All @@ -1007,9 +1014,9 @@ void Statement::JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info)
stmt->mode = use ? Data::EXPAND : stmt->mode == Data::EXPAND ? Data::FLAT : stmt->mode;
info.GetReturnValue().Set(info.This());
}
#line 251 "./src/objects/statement.lzz"
#line 258 "./src/objects/statement.lzz"
void Statement::JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 251 "./src/objects/statement.lzz"
#line 258 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if (!stmt->returns_data) return ThrowTypeError("The raw() method is only for statements that return data");
Expand All @@ -1020,9 +1027,9 @@ void Statement::JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info)
stmt->mode = use ? Data::RAW : stmt->mode == Data::RAW ? Data::FLAT : stmt->mode;
info.GetReturnValue().Set(info.This());
}
#line 262 "./src/objects/statement.lzz"
#line 269 "./src/objects/statement.lzz"
void Statement::JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 262 "./src/objects/statement.lzz"
#line 269 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if ( stmt -> db -> GetState ( ) -> busy ) return ThrowTypeError ( "This database connection is busy executing a query" ) ;
Expand All @@ -1031,9 +1038,9 @@ void Statement::JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const &
else { if ( info . Length ( ) <= ( 0 ) || ! info [ 0 ] -> IsBoolean ( ) ) return ThrowTypeError ( "Expected " "first" " argument to be " "a boolean" ) ; stmt -> safe_ints = ( info [ 0 ] . As < v8 :: Boolean > ( ) ) -> Value ( ) ; }
info.GetReturnValue().Set(info.This());
}
#line 271 "./src/objects/statement.lzz"
#line 278 "./src/objects/statement.lzz"
void Statement::JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info)
#line 271 "./src/objects/statement.lzz"
#line 278 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
if (!stmt->returns_data) return ThrowTypeError("The columns() method is only for statements that return data");
Expand Down Expand Up @@ -1076,9 +1083,9 @@ void Statement::JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info)

info.GetReturnValue().Set(columns);
}
#line 314 "./src/objects/statement.lzz"
#line 321 "./src/objects/statement.lzz"
void Statement::JS_busy (v8::Local <v8 :: String> _, v8::PropertyCallbackInfo <v8 :: Value> const & info)
#line 314 "./src/objects/statement.lzz"
#line 321 "./src/objects/statement.lzz"
{
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
info.GetReturnValue().Set(stmt->alive && stmt->locked);
Expand Down
42 changes: 21 additions & 21 deletions src/better_sqlite3.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,47 +340,47 @@ class Statement : public node::ObjectWrap
explicit Statement (Database * db, sqlite3_stmt * handle, sqlite3_uint64 id, bool returns_data);
#line 85 "./src/objects/statement.lzz"
static void JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 149 "./src/objects/statement.lzz"
#line 156 "./src/objects/statement.lzz"
static void JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 172 "./src/objects/statement.lzz"
#line 179 "./src/objects/statement.lzz"
static void JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 187 "./src/objects/statement.lzz"
#line 194 "./src/objects/statement.lzz"
static void JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 208 "./src/objects/statement.lzz"
#line 215 "./src/objects/statement.lzz"
static void JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 218 "./src/objects/statement.lzz"
#line 225 "./src/objects/statement.lzz"
static void JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 229 "./src/objects/statement.lzz"
#line 236 "./src/objects/statement.lzz"
static void JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 240 "./src/objects/statement.lzz"
#line 247 "./src/objects/statement.lzz"
static void JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 251 "./src/objects/statement.lzz"
#line 258 "./src/objects/statement.lzz"
static void JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 262 "./src/objects/statement.lzz"
#line 269 "./src/objects/statement.lzz"
static void JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 271 "./src/objects/statement.lzz"
#line 278 "./src/objects/statement.lzz"
static void JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info);
#line 314 "./src/objects/statement.lzz"
#line 321 "./src/objects/statement.lzz"
static void JS_busy (v8::Local <v8 :: String> _, v8::PropertyCallbackInfo <v8 :: Value> const & info);
#line 319 "./src/objects/statement.lzz"
#line 326 "./src/objects/statement.lzz"
Database * const db;
#line 320 "./src/objects/statement.lzz"
#line 327 "./src/objects/statement.lzz"
sqlite3_stmt * const handle;
#line 321 "./src/objects/statement.lzz"
#line 328 "./src/objects/statement.lzz"
Extras * const extras;
#line 322 "./src/objects/statement.lzz"
#line 329 "./src/objects/statement.lzz"
bool alive;
#line 323 "./src/objects/statement.lzz"
#line 330 "./src/objects/statement.lzz"
bool locked;
#line 324 "./src/objects/statement.lzz"
#line 331 "./src/objects/statement.lzz"
bool bound;
#line 325 "./src/objects/statement.lzz"
#line 332 "./src/objects/statement.lzz"
bool has_bind_map;
#line 326 "./src/objects/statement.lzz"
#line 333 "./src/objects/statement.lzz"
bool safe_ints;
#line 327 "./src/objects/statement.lzz"
#line 334 "./src/objects/statement.lzz"
char mode;
#line 328 "./src/objects/statement.lzz"
#line 335 "./src/objects/statement.lzz"
bool const returns_data;
};
#line 1 "./src/objects/statement-iterator.lzz"
Expand Down
15 changes: 11 additions & 4 deletions src/objects/statement.lzz
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,20 +113,27 @@ private:
if (handle == NULL) {
return ThrowRangeError("The supplied SQL string contains no statements");
}
for (char c; (c = *tail); ++tail) {
if (IS_SKIPPED(c)) continue;
// https://github.com/WiseLibs/better-sqlite3/issues/975#issuecomment-1520934678
for (char c; (c = *tail); ) {
if (IS_SKIPPED(c)) {
++tail;
continue;
}
if (c == '/' && tail[1] == '*') {
tail += 2;
for (char c; (c = *tail); ++tail) {
if (c == '*' && tail[1] == '/') {
tail += 1;
tail += 2;
break;
}
}
} else if (c == '-' && tail[1] == '-') {
tail += 2;
for (char c; (c = *tail); ++tail) {
if (c == '\n') break;
if (c == '\n') {
++tail;
break;
}
}
} else {
sqlite3_finalize(handle);
Expand Down
14 changes: 14 additions & 0 deletions test/13.database.prepare.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ describe('Database#prepare()', function () {
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);CREATE TABLE animals (name TEXT)')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);-')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);--\n/')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);--\nSELECT 123')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);-- comment\nSELECT 123')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/**/-')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/**/SELECT 123')).to.throw(RangeError);
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/* comment */SELECT 123')).to.throw(RangeError);
});
it('should create a prepared Statement object', function () {
const stmt1 = this.db.prepare('CREATE TABLE people (name TEXT) ');
Expand All @@ -57,4 +63,12 @@ describe('Database#prepare()', function () {
assertStmt(this.db.prepare('BEGIN EXCLUSIVE'), 'BEGIN EXCLUSIVE', this.db, false, false);
assertStmt(this.db.prepare('DELETE FROM data RETURNING *'), 'DELETE FROM data RETURNING *', this.db, true, false);
});
it('should create a prepared Statement object ignoring trailing comments and whitespace', function () {
assertStmt(this.db.prepare('SELECT 555; '), 'SELECT 555; ', this.db, true, true);
assertStmt(this.db.prepare('SELECT 555;-- comment'), 'SELECT 555;-- comment', this.db, true, true);
assertStmt(this.db.prepare('SELECT 555;--abc\n--de\n--f'), 'SELECT 555;--abc\n--de\n--f', this.db, true, true);
assertStmt(this.db.prepare('SELECT 555;/* comment */'), 'SELECT 555;/* comment */', this.db, true, true);
assertStmt(this.db.prepare('SELECT 555;/* comment */-- comment'), 'SELECT 555;/* comment */-- comment', this.db, true, true);
assertStmt(this.db.prepare('SELECT 555;-- comment\n/* comment */'), 'SELECT 555;-- comment\n/* comment */', this.db, true, true);
});
});

0 comments on commit c37b7ee

Please sign in to comment.