Skip to content

Fix out-of-bounds read in statement tail parser #996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 2, 2023
Merged

Fix out-of-bounds read in statement tail parser #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d04876
Fix out-of-bounds read in statement tail parser
arimah Apr 25, 2023
99fd6b6
Add comment with link to tail parsing issue
arimah Apr 25, 2023
36cc72a
Fix missing increment in tail parser + tests
arimah Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 33 additions & 26 deletions src/better_sqlite3.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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);
});
});