diff --git a/CHANGELOG.md b/CHANGELOG.md index c69bc42f..e1e6d77b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## next / unreleased +### Security / Stability + +- Raise `RangeError` when a SQL string, a bound String or Blob value, or a String or Blob returned from a custom function or aggregate is longer than `INT_MAX` bytes. Previously the length was truncated to 32 bits, so SQLite could read only part of the string and drop trailing clauses from a query. See [GHSA-4fqr-96q8-xrfx](https://github.com/sparklemotion/sqlite3-ruby/security/advisories/GHSA-4fqr-96q8-xrfx) for more information. #762 @flavorjones +- Fix a crash when a custom aggregate's `finalize` returns a value that can't be converted to a SQLite result. The exception is now raised from the query and the connection remains usable. #762 @flavorjones + ### Improved - `Statement#step` no longer allocates and discards an array when the statement has no more rows. This saves one object per statement execution. #758 @thomasklemm diff --git a/ext/sqlite3/aggregator.c b/ext/sqlite3/aggregator.c index 8f790201..8a380b88 100644 --- a/ext/sqlite3/aggregator.c +++ b/ext/sqlite3/aggregator.c @@ -25,6 +25,7 @@ typedef struct rb_sqlite3_protected_funcall_args { ID method; int argc; VALUE *params; + sqlite3_context *result_ctx; } protected_funcall_args_t; /* why isn't there something like this in the ruby API? */ @@ -34,15 +35,21 @@ rb_sqlite3_protected_funcall_body(VALUE protected_funcall_args_ptr) protected_funcall_args_t *args = (protected_funcall_args_t *)protected_funcall_args_ptr; - return rb_funcall2(args->self, args->method, args->argc, args->params); + VALUE result = rb_funcall2(args->self, args->method, args->argc, args->params); + + if (args->result_ctx) { + set_sqlite3_func_result(args->result_ctx, result); + } + + return result; } static VALUE rb_sqlite3_protected_funcall(VALUE self, ID method, int argc, VALUE *params, - int *exc_status) + sqlite3_context *result_ctx, int *exc_status) { protected_funcall_args_t args = { - .self = self, .method = method, .argc = argc, .params = params + .self = self, .method = method, .argc = argc, .params = params, .result_ctx = result_ctx }; return rb_protect(rb_sqlite3_protected_funcall_body, (VALUE)(&args), exc_status); } @@ -71,7 +78,7 @@ rb_sqlite3_aggregate_instance(sqlite3_context *ctx) inst = rb_class_new_instance(0, NULL, cAggregatorInstance); rb_iv_set(inst, "-handler_instance", rb_sqlite3_protected_funcall( - handler_klass, rb_intern("new"), 0, NULL, &exc_status)); + handler_klass, rb_intern("new"), 0, NULL, NULL, &exc_status)); rb_iv_set(inst, "-exc_status", INT2NUM(exc_status)); rb_ary_push(instances, inst); @@ -141,7 +148,7 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) } } rb_sqlite3_protected_funcall( - handler_instance, rb_intern("step"), argc, params, &exc_status); + handler_instance, rb_intern("step"), argc, params, NULL, &exc_status); if (argc > 1) { ALLOCV_END(params_handle); } @@ -158,11 +165,8 @@ rb_sqlite3_aggregator_final(sqlite3_context *ctx) int exc_status = NUM2INT(rb_iv_get(inst, "-exc_status")); if (!exc_status) { - VALUE result = rb_sqlite3_protected_funcall( - handler_instance, rb_intern("finalize"), 0, NULL, &exc_status); - if (!exc_status) { - set_sqlite3_func_result(ctx, result); - } + rb_sqlite3_protected_funcall( + handler_instance, rb_intern("finalize"), 0, NULL, ctx, &exc_status); } if (exc_status) { diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 5d16ac77..92aebabc 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -529,14 +529,14 @@ set_sqlite3_func_result(sqlite3_context *ctx, VALUE result) sqlite3_result_blob( ctx, (const void *)StringValuePtr(result), - (int)RSTRING_LEN(result), + RSTRING_LENINT(result), SQLITE_TRANSIENT ); } else { sqlite3_result_text( ctx, (const char *)StringValuePtr(result), - (int)RSTRING_LEN(result), + RSTRING_LENINT(result), SQLITE_TRANSIENT ); } diff --git a/ext/sqlite3/statement.c b/ext/sqlite3/statement.c index edaaa98b..35a2cd4e 100644 --- a/ext/sqlite3/statement.c +++ b/ext/sqlite3/statement.c @@ -75,7 +75,7 @@ prepare(VALUE self, VALUE db, VALUE sql) #endif db_ctx->db, (const char *)StringValuePtr(sql), - (int)RSTRING_LEN(sql), + RSTRING_LENINT(sql), &ctx->st, &tail ); @@ -279,7 +279,7 @@ bind_param(VALUE self, VALUE key, VALUE value) ctx->st, index, (const char *)StringValuePtr(value), - (int)RSTRING_LEN(value), + RSTRING_LENINT(value), SQLITE_TRANSIENT ); } else { @@ -290,7 +290,7 @@ bind_param(VALUE self, VALUE key, VALUE value) ctx->st, index, (const char *)StringValuePtr(value), - (int)RSTRING_LEN(value), + RSTRING_LENINT(value), SQLITE_TRANSIENT ); } else { @@ -301,7 +301,7 @@ bind_param(VALUE self, VALUE key, VALUE value) ctx->st, index, (const char *)StringValuePtr(value), - (int)RSTRING_LEN(value), + RSTRING_LENINT(value), SQLITE_TRANSIENT ); } diff --git a/test/helper.rb b/test/helper.rb index e52afbba..bffae82d 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -127,6 +127,16 @@ def skip_if_timing_unreliable def windows? ::RUBY_PLATFORM =~ /mingw|mswin/ end + + # SQLite takes string lengths as int, so it can't be passed a string longer than INT_MAX bytes. + # CRuby allocates a string of NUL bytes with calloc, which stays cheap until it's read. + def string_longer_than_int_max(encoding = Encoding::UTF_8) + skip("a string can't be longer than INT_MAX bytes on this platform") if [0].pack("l!").bytesize < 8 + skip("only CRuby allocates this string cheaply") if RUBY_ENGINE != "ruby" + skip("valgrind's calloc writes every byte") if i_am_running_in_valgrind + skip("suspected of crashing FreeBSD CI, see #762") if RUBY_PLATFORM.include?("freebsd") + ("\0" * (2**31)).force_encoding(encoding) + end end end diff --git a/test/test_database.rb b/test/test_database.rb index a43c059b..555b0b5d 100644 --- a/test/test_database.rb +++ b/test/test_database.rb @@ -585,6 +585,22 @@ def test_function_return_types end end + def test_function_raises_on_returned_text_longer_than_int_max + text = string_longer_than_int_max + @db.define_function("huge") { |a| text } + + assert_raise(RangeError) { @db.execute("select huge(1)") } + assert_equal([[2]], @db.execute("select 1 + 1")) + end + + def test_function_raises_on_returned_blob_longer_than_int_max + blob = string_longer_than_int_max(Encoding::BINARY) + @db.define_function("huge") { |a| blob } + + assert_raise(RangeError) { @db.execute("select huge(1)") } + assert_equal([[2]], @db.execute("select 1 + 1")) + end + def test_function_raise_propagates_and_connection_remains_usable @db.define_function("boom") { |a| raise "boom: #{a}" } diff --git a/test/test_integration_aggregate.rb b/test/test_integration_aggregate.rb index 3c581217..9d9a59d8 100644 --- a/test/test_integration_aggregate.rb +++ b/test/test_integration_aggregate.rb @@ -207,6 +207,29 @@ def test_create_aggregate_with_exception_in_finalize end end + def test_create_aggregate_raises_on_returned_unsupported_type + @db.create_aggregate("unsupported", 1) do + step { |ctx, a| } + finalize { |ctx| ctx.result = {} } + end + + assert_raise(RuntimeError) { @db.get_first_value("select unsupported(a) from foo") } + assert_equal(2, @db.get_first_value("select 1 + 1")) + GC.start + end + + def test_create_aggregate_raises_on_returned_text_longer_than_int_max + text = string_longer_than_int_max + @db.create_aggregate("huge", 1) do + step { |ctx, a| } + finalize { |ctx| ctx.result = text } + end + + assert_raise(RangeError) { @db.get_first_value("select huge(a) from foo") } + assert_equal(2, @db.get_first_value("select 1 + 1")) + GC.start + end + def test_create_aggregate_with_no_data @db.create_aggregate("accumulate", 1) do step do |ctx, a| diff --git a/test/test_statement.rb b/test/test_statement.rb index a22c5626..d7497032 100644 --- a/test/test_statement.rb +++ b/test/test_statement.rb @@ -223,6 +223,32 @@ def test_bind_blob end end + def test_prepare_raises_on_sql_longer_than_int_max + sql = string_longer_than_int_max + assert_raises(RangeError) { SQLite3::Statement.new(@db, sql) } + end + + def test_bind_param_raises_on_text_longer_than_int_max + text = string_longer_than_int_max + stmt = SQLite3::Statement.new(@db, "select ?") + assert_raises(RangeError) { stmt.bind_param(1, text) } + stmt.close + end + + def test_bind_param_raises_on_utf16_text_longer_than_int_max + text = string_longer_than_int_max(Encoding::UTF_16LE) + stmt = SQLite3::Statement.new(@db, "select ?") + assert_raises(RangeError) { stmt.bind_param(1, text) } + stmt.close + end + + def test_bind_param_raises_on_blob_longer_than_int_max + blob = string_longer_than_int_max(Encoding::BINARY) + stmt = SQLite3::Statement.new(@db, "select ?") + assert_raises(RangeError) { stmt.bind_param(1, blob) } + stmt.close + end + def test_decltype_is_utf8 @db.execute %(create table foo(a integer, b "VARCHÄR(255)")) @db.prepare("select a, b from foo") do |stmt|