Skip to content

Commit 9e286a7

Browse files
arimahArGeoph
arimah
authored and
ArGeoph
committed
Fix out-of-bounds read in statement tail parser (WiseLibs#996)
* Fix out-of-bounds read in statement tail parser * Add comment with link to tail parsing issue * Fix missing increment in tail parser + tests
1 parent 2250e03 commit 9e286a7

File tree

4 files changed

+79
-51
lines changed

4 files changed

+79
-51
lines changed

‎src/better_sqlite3.cpp

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -904,20 +904,27 @@ void Statement::JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info)
904904
if (handle == NULL) {
905905
return ThrowRangeError("The supplied SQL string contains no statements");
906906
}
907-
for (char c; (c = *tail); ++tail) {
908-
if (IS_SKIPPED(c)) continue;
907+
908+
for (char c; (c = *tail); ) {
909+
if (IS_SKIPPED(c)) {
910+
++tail;
911+
continue;
912+
}
909913
if (c == '/' && tail[1] == '*') {
910914
tail += 2;
911915
for (char c; (c = *tail); ++tail) {
912916
if (c == '*' && tail[1] == '/') {
913-
tail += 1;
917+
tail += 2;
914918
break;
915919
}
916920
}
917921
} else if (c == '-' && tail[1] == '-') {
918922
tail += 2;
919923
for (char c; (c = *tail); ++tail) {
920-
if (c == '\n') break;
924+
if (c == '\n') {
925+
++tail;
926+
break;
927+
}
921928
}
922929
} else {
923930
sqlite3_finalize(handle);
@@ -936,9 +943,9 @@ void Statement::JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info)
936943

937944
info.GetReturnValue().Set(info.This());
938945
}
939-
#line 149 "./src/objects/statement.lzz"
946+
#line 156 "./src/objects/statement.lzz"
940947
void Statement::JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info)
941-
#line 149 "./src/objects/statement.lzz"
948+
#line 156 "./src/objects/statement.lzz"
942949
{
943950
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 ) ;
944951
sqlite3* db_handle = db->GetHandle();
@@ -961,9 +968,9 @@ void Statement::JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info)
961968
}
962969
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
963970
}
964-
#line 172 "./src/objects/statement.lzz"
971+
#line 179 "./src/objects/statement.lzz"
965972
void Statement::JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info)
966-
#line 172 "./src/objects/statement.lzz"
973+
#line 179 "./src/objects/statement.lzz"
967974
{
968975
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 ) ;
969976
int status = sqlite3_step(handle);
@@ -978,9 +985,9 @@ void Statement::JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info)
978985
sqlite3_reset(handle);
979986
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
980987
}
981-
#line 187 "./src/objects/statement.lzz"
988+
#line 194 "./src/objects/statement.lzz"
982989
void Statement::JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info)
983-
#line 187 "./src/objects/statement.lzz"
990+
#line 194 "./src/objects/statement.lzz"
984991
{
985992
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 ) ;
986993
v8 :: Local < v8 :: Context > ctx = isolate -> GetCurrentContext ( ) ;
@@ -1001,9 +1008,9 @@ void Statement::JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10011008
if (js_error) db->GetState()->was_js_error = true;
10021009
db -> GetState ( ) -> busy = false ; db -> ThrowDatabaseError ( ) ; if ( ! bound ) { sqlite3_clear_bindings ( handle ) ; } return ;
10031010
}
1004-
#line 208 "./src/objects/statement.lzz"
1011+
#line 215 "./src/objects/statement.lzz"
10051012
void Statement::JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1006-
#line 208 "./src/objects/statement.lzz"
1013+
#line 215 "./src/objects/statement.lzz"
10071014
{
10081015
Addon * addon = static_cast < Addon * > ( info . Data ( ) . As < v8 :: External > ( ) -> Value ( ) ) ;
10091016
v8 :: Isolate * isolate = info . GetIsolate ( ) ;
@@ -1013,9 +1020,9 @@ void Statement::JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10131020
addon->privileged_info = NULL;
10141021
if (!maybeIterator.IsEmpty()) info.GetReturnValue().Set(maybeIterator.ToLocalChecked());
10151022
}
1016-
#line 218 "./src/objects/statement.lzz"
1023+
#line 225 "./src/objects/statement.lzz"
10171024
void Statement::JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1018-
#line 218 "./src/objects/statement.lzz"
1025+
#line 225 "./src/objects/statement.lzz"
10191026
{
10201027
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10211028
if (stmt->bound) return ThrowTypeError("The bind() method can only be invoked once per statement object");
@@ -1026,9 +1033,9 @@ void Statement::JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10261033
stmt->bound = true;
10271034
info.GetReturnValue().Set(info.This());
10281035
}
1029-
#line 229 "./src/objects/statement.lzz"
1036+
#line 236 "./src/objects/statement.lzz"
10301037
void Statement::JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1031-
#line 229 "./src/objects/statement.lzz"
1038+
#line 236 "./src/objects/statement.lzz"
10321039
{
10331040
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10341041
if (!stmt->returns_data) return ThrowTypeError("The pluck() method is only for statements that return data");
@@ -1039,9 +1046,9 @@ void Statement::JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10391046
stmt->mode = use ? Data::PLUCK : stmt->mode == Data::PLUCK ? Data::FLAT : stmt->mode;
10401047
info.GetReturnValue().Set(info.This());
10411048
}
1042-
#line 240 "./src/objects/statement.lzz"
1049+
#line 247 "./src/objects/statement.lzz"
10431050
void Statement::JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1044-
#line 240 "./src/objects/statement.lzz"
1051+
#line 247 "./src/objects/statement.lzz"
10451052
{
10461053
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10471054
if (!stmt->returns_data) return ThrowTypeError("The expand() method is only for statements that return data");
@@ -1052,9 +1059,9 @@ void Statement::JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10521059
stmt->mode = use ? Data::EXPAND : stmt->mode == Data::EXPAND ? Data::FLAT : stmt->mode;
10531060
info.GetReturnValue().Set(info.This());
10541061
}
1055-
#line 251 "./src/objects/statement.lzz"
1062+
#line 258 "./src/objects/statement.lzz"
10561063
void Statement::JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1057-
#line 251 "./src/objects/statement.lzz"
1064+
#line 258 "./src/objects/statement.lzz"
10581065
{
10591066
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10601067
if (!stmt->returns_data) return ThrowTypeError("The raw() method is only for statements that return data");
@@ -1065,9 +1072,9 @@ void Statement::JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info)
10651072
stmt->mode = use ? Data::RAW : stmt->mode == Data::RAW ? Data::FLAT : stmt->mode;
10661073
info.GetReturnValue().Set(info.This());
10671074
}
1068-
#line 262 "./src/objects/statement.lzz"
1075+
#line 269 "./src/objects/statement.lzz"
10691076
void Statement::JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1070-
#line 262 "./src/objects/statement.lzz"
1077+
#line 269 "./src/objects/statement.lzz"
10711078
{
10721079
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10731080
if ( stmt -> db -> GetState ( ) -> busy ) return ThrowTypeError ( "This database connection is busy executing a query" ) ;
@@ -1076,9 +1083,9 @@ void Statement::JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const &
10761083
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 ( ) ; }
10771084
info.GetReturnValue().Set(info.This());
10781085
}
1079-
#line 271 "./src/objects/statement.lzz"
1086+
#line 278 "./src/objects/statement.lzz"
10801087
void Statement::JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info)
1081-
#line 271 "./src/objects/statement.lzz"
1088+
#line 278 "./src/objects/statement.lzz"
10821089
{
10831090
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
10841091
if (!stmt->returns_data) return ThrowTypeError("The columns() method is only for statements that return data");
@@ -1121,9 +1128,9 @@ void Statement::JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info)
11211128

11221129
info.GetReturnValue().Set(columns);
11231130
}
1124-
#line 314 "./src/objects/statement.lzz"
1131+
#line 321 "./src/objects/statement.lzz"
11251132
void Statement::JS_busy (v8::Local <v8 :: String> _, v8::PropertyCallbackInfo <v8 :: Value> const & info)
1126-
#line 314 "./src/objects/statement.lzz"
1133+
#line 321 "./src/objects/statement.lzz"
11271134
{
11281135
Statement* stmt = node :: ObjectWrap :: Unwrap <Statement>(info.This());
11291136
info.GetReturnValue().Set(stmt->alive && stmt->locked);

‎src/better_sqlite3.hpp

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -351,47 +351,47 @@ class Statement : public node::ObjectWrap
351351
explicit Statement (Database * db, sqlite3_stmt * handle, sqlite3_uint64 id, bool returns_data);
352352
#line 85 "./src/objects/statement.lzz"
353353
static void JS_new (v8::FunctionCallbackInfo <v8 :: Value> const & info);
354-
#line 149 "./src/objects/statement.lzz"
354+
#line 156 "./src/objects/statement.lzz"
355355
static void JS_run (v8::FunctionCallbackInfo <v8 :: Value> const & info);
356-
#line 172 "./src/objects/statement.lzz"
356+
#line 179 "./src/objects/statement.lzz"
357357
static void JS_get (v8::FunctionCallbackInfo <v8 :: Value> const & info);
358-
#line 187 "./src/objects/statement.lzz"
358+
#line 194 "./src/objects/statement.lzz"
359359
static void JS_all (v8::FunctionCallbackInfo <v8 :: Value> const & info);
360-
#line 208 "./src/objects/statement.lzz"
360+
#line 215 "./src/objects/statement.lzz"
361361
static void JS_iterate (v8::FunctionCallbackInfo <v8 :: Value> const & info);
362-
#line 218 "./src/objects/statement.lzz"
362+
#line 225 "./src/objects/statement.lzz"
363363
static void JS_bind (v8::FunctionCallbackInfo <v8 :: Value> const & info);
364-
#line 229 "./src/objects/statement.lzz"
364+
#line 236 "./src/objects/statement.lzz"
365365
static void JS_pluck (v8::FunctionCallbackInfo <v8 :: Value> const & info);
366-
#line 240 "./src/objects/statement.lzz"
366+
#line 247 "./src/objects/statement.lzz"
367367
static void JS_expand (v8::FunctionCallbackInfo <v8 :: Value> const & info);
368-
#line 251 "./src/objects/statement.lzz"
368+
#line 258 "./src/objects/statement.lzz"
369369
static void JS_raw (v8::FunctionCallbackInfo <v8 :: Value> const & info);
370-
#line 262 "./src/objects/statement.lzz"
370+
#line 269 "./src/objects/statement.lzz"
371371
static void JS_safeIntegers (v8::FunctionCallbackInfo <v8 :: Value> const & info);
372-
#line 271 "./src/objects/statement.lzz"
372+
#line 278 "./src/objects/statement.lzz"
373373
static void JS_columns (v8::FunctionCallbackInfo <v8 :: Value> const & info);
374-
#line 314 "./src/objects/statement.lzz"
374+
#line 321 "./src/objects/statement.lzz"
375375
static void JS_busy (v8::Local <v8 :: String> _, v8::PropertyCallbackInfo <v8 :: Value> const & info);
376-
#line 319 "./src/objects/statement.lzz"
376+
#line 326 "./src/objects/statement.lzz"
377377
Database * const db;
378-
#line 320 "./src/objects/statement.lzz"
378+
#line 327 "./src/objects/statement.lzz"
379379
sqlite3_stmt * const handle;
380-
#line 321 "./src/objects/statement.lzz"
380+
#line 328 "./src/objects/statement.lzz"
381381
Extras * const extras;
382-
#line 322 "./src/objects/statement.lzz"
382+
#line 329 "./src/objects/statement.lzz"
383383
bool alive;
384-
#line 323 "./src/objects/statement.lzz"
384+
#line 330 "./src/objects/statement.lzz"
385385
bool locked;
386-
#line 324 "./src/objects/statement.lzz"
386+
#line 331 "./src/objects/statement.lzz"
387387
bool bound;
388-
#line 325 "./src/objects/statement.lzz"
388+
#line 332 "./src/objects/statement.lzz"
389389
bool has_bind_map;
390-
#line 326 "./src/objects/statement.lzz"
390+
#line 333 "./src/objects/statement.lzz"
391391
bool safe_ints;
392-
#line 327 "./src/objects/statement.lzz"
392+
#line 334 "./src/objects/statement.lzz"
393393
char mode;
394-
#line 328 "./src/objects/statement.lzz"
394+
#line 335 "./src/objects/statement.lzz"
395395
bool const returns_data;
396396
};
397397
#line 1 "./src/objects/statement-iterator.lzz"

‎src/objects/statement.lzz

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,20 +113,27 @@ private:
113113
if (handle == NULL) {
114114
return ThrowRangeError("The supplied SQL string contains no statements");
115115
}
116-
for (char c; (c = *tail); ++tail) {
117-
if (IS_SKIPPED(c)) continue;
116+
// https://github.com/WiseLibs/better-sqlite3/issues/975#issuecomment-1520934678
117+
for (char c; (c = *tail); ) {
118+
if (IS_SKIPPED(c)) {
119+
++tail;
120+
continue;
121+
}
118122
if (c == '/' && tail[1] == '*') {
119123
tail += 2;
120124
for (char c; (c = *tail); ++tail) {
121125
if (c == '*' && tail[1] == '/') {
122-
tail += 1;
126+
tail += 2;
123127
break;
124128
}
125129
}
126130
} else if (c == '-' && tail[1] == '-') {
127131
tail += 2;
128132
for (char c; (c = *tail); ++tail) {
129-
if (c == '\n') break;
133+
if (c == '\n') {
134+
++tail;
135+
break;
136+
}
130137
}
131138
} else {
132139
sqlite3_finalize(handle);

‎test/13.database.prepare.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ describe('Database#prepare()', function () {
3737
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);CREATE TABLE animals (name TEXT)')).to.throw(RangeError);
3838
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/')).to.throw(RangeError);
3939
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);-')).to.throw(RangeError);
40+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);--\n/')).to.throw(RangeError);
41+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);--\nSELECT 123')).to.throw(RangeError);
42+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);-- comment\nSELECT 123')).to.throw(RangeError);
43+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/**/-')).to.throw(RangeError);
44+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/**/SELECT 123')).to.throw(RangeError);
45+
expect(() => this.db.prepare('CREATE TABLE people (name TEXT);/* comment */SELECT 123')).to.throw(RangeError);
4046
});
4147
it('should create a prepared Statement object', function () {
4248
const stmt1 = this.db.prepare('CREATE TABLE people (name TEXT) ');
@@ -57,4 +63,12 @@ describe('Database#prepare()', function () {
5763
assertStmt(this.db.prepare('BEGIN EXCLUSIVE'), 'BEGIN EXCLUSIVE', this.db, false, false);
5864
assertStmt(this.db.prepare('DELETE FROM data RETURNING *'), 'DELETE FROM data RETURNING *', this.db, true, false);
5965
});
66+
it('should create a prepared Statement object ignoring trailing comments and whitespace', function () {
67+
assertStmt(this.db.prepare('SELECT 555; '), 'SELECT 555; ', this.db, true, true);
68+
assertStmt(this.db.prepare('SELECT 555;-- comment'), 'SELECT 555;-- comment', this.db, true, true);
69+
assertStmt(this.db.prepare('SELECT 555;--abc\n--de\n--f'), 'SELECT 555;--abc\n--de\n--f', this.db, true, true);
70+
assertStmt(this.db.prepare('SELECT 555;/* comment */'), 'SELECT 555;/* comment */', this.db, true, true);
71+
assertStmt(this.db.prepare('SELECT 555;/* comment */-- comment'), 'SELECT 555;/* comment */-- comment', this.db, true, true);
72+
assertStmt(this.db.prepare('SELECT 555;-- comment\n/* comment */'), 'SELECT 555;-- comment\n/* comment */', this.db, true, true);
73+
});
6074
});

0 commit comments

Comments
 (0)