From 2709b1b30db14c3402450c854f83b55b6df563ca Mon Sep 17 00:00:00 2001 From: Marco Bambini Date: Sun, 1 Nov 2026 07:36:27 +0100 Subject: [PATCH 1/3] fix: eight bugs from a read-through of src/ Two of them are memory-safety faults reachable from ordinary SQL, the rest are correctness or validation gaps. Each one was reproduced before being fixed and has a regression test in test_malformed_input(). vCursorFilterCommon() passed a size_t byte count to sqlite3_malloc(), whose parameter is an int. k comes straight from the query, so k=536870913 asked for 0x100000008 bytes, got 8, and the memset that follows overran the top-k arrays. Both arrays now go through sqlite3_malloc64(), which refuses anything near 2GB, so an absurd k is a clean SQLITE_NOMEM. row_count also drops to zero before either allocation: it is what tells the next filter whether the arrays can be reused, and a cursor that failed between the two used to keep the old count beside a freed array. sqlite_unserialize() passed sqlite3_column_text() straight to strcmp(). _sqliteai_vector is an ordinary writable table, and PRIMARY KEY on a rowid table does not imply NOT NULL, so one row with a NULL key crashed the next vector_init(). vector_rebuild_quantization() writes q_type/q_bits/scale/offset into the table context before the pass that can still fail, and vector_quantize() rolls the shadow table back when it does. The context was left describing a layout the table no longer had, and the next scan read the chunks at the wrong stride - wrong rowids, wrong distances, no error at all. Those fields are now snapshotted and restored on the cleanup path, together with the TurboQuant cache that rebuild() drops whenever it quantizes to anything else. Zero is the streaming scans' "load the next chunk" sentinel, so a chunk claiming no vectors slipped past the length check beside it, which degenerates to "bytes < 0", and a row was read from past the end of its blob. Every count test is now <= 0, and preload rejects such a chunk when it loads rather than leaving it to query time. vector_as_*() ignored its optional dimension argument for non-BIT blobs, though API.md documents it as a stricter check and both the BIT and the JSON paths already enforced it. A wrong-sized vector was stored happily and then skipped in silence by every scan, so it never appeared in a result. The new check reports the size it wanted with %lld, because for a float type that size is dimension * 4 and can exceed INT_MAX - with a %d it said "expected 0 bytes". The BIT branch beside it moves to the same format for consistency, though there the value is (dimension + 7) / 8 and cannot overflow. context_result_error() carries no printf format attribute, so only the new runtime test catches this. human_to_number() signalled failure by returning 0, which also made a literal "0" unparseable - and zero is the one value the quantizer reads as "use whatever memory the index needs", so that setting could not be expressed through the option string. It now reports success separately, and an unparseable max_memory is an error like every other bad option instead of being dropped. The int8 and uint8 cosine kernels of the scalar and SSE2 backends accumulated in 32 bits, which wrapped around past roughly 133k and 66k dimensions; NEON, AVX2 and AVX-512 already used 64-bit accumulators. SSE2 now also computes sqrt(a)*sqrt(b) like every other backend instead of sqrtf(a*b), which could overflow in float, so the five backends compute the same cosine. The vector lane accumulators in SSE2 and RVV are still 32-bit: widening those means restructuring the kernels, and the limit is above half a million dimensions. Also renamed a local in vector_rebuild_quantization() that shadowed the caller's out-parameter, which was the build's only warning under -Wshadow. make unittest: 1473 passed, 0 failed, and the same under ASan and UBSan. Co-Authored-By: Claude Opus 5 --- src/distance-cpu.c | 21 ++-- src/distance-sse2.c | 28 +++--- src/sqlite-vector.c | 135 ++++++++++++++++++++------ test/test_vector.c | 228 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 363 insertions(+), 49 deletions(-) diff --git a/src/distance-cpu.c b/src/distance-cpu.c index a7ce837..2916508 100644 --- a/src/distance-cpu.c +++ b/src/distance-cpu.c @@ -515,9 +515,11 @@ float uint8_distance_cosine_cpu (const void *v1, const void *v2, int n) { const uint8_t *a = (const uint8_t *)v1; const uint8_t *b = (const uint8_t *)v2; - uint32_t dot = 0; - uint32_t norm_a2 = 0; - uint32_t norm_b2 = 0; + // 64-bit for the same reason as the int8 kernel below: a product is at most 255*255, + // so uint32_t wrapped around silently past roughly 66k dimensions + uint64_t dot = 0; + uint64_t norm_a2 = 0; + uint64_t norm_b2 = 0; int i = 0; for (; i <= n - 4; i += 4) { @@ -544,7 +546,7 @@ float uint8_distance_cosine_cpu (const void *v1, const void *v2, int n) { return 1.0f; } - float cosine_similarity = dot / (sqrtf((float)norm_a2) * sqrtf((float)norm_b2)); + float cosine_similarity = (float)((double)dot / (sqrt((double)norm_a2) * sqrt((double)norm_b2))); if (cosine_similarity > 1.0f) cosine_similarity = 1.0f; if (cosine_similarity < -1.0f) cosine_similarity = -1.0f; return 1.0f - cosine_similarity; @@ -629,9 +631,12 @@ float int8_distance_cosine_cpu (const void *v1, const void *v2, int n) { const int8_t *a = (const int8_t *)v1; const int8_t *b = (const int8_t *)v2; - int32_t dot = 0; - int32_t norm_a2 = 0; - int32_t norm_b2 = 0; + // 64-bit accumulators, as the NEON/AVX2/AVX-512 kernels use: a product is at most + // 127*127, so int32_t saturated at roughly 133k dimensions and the loop wrapped + // around silently. Each group of four products still fits in 32 bits. + int64_t dot = 0; + int64_t norm_a2 = 0; + int64_t norm_b2 = 0; int i = 0; for (; i <= n - 4; i += 4) { @@ -658,7 +663,7 @@ float int8_distance_cosine_cpu (const void *v1, const void *v2, int n) { return 1.0f; } - float cosine_similarity = dot / (sqrtf((float)norm_a2) * sqrtf((float)norm_b2)); + float cosine_similarity = (float)((double)dot / (sqrt((double)norm_a2) * sqrt((double)norm_b2))); if (cosine_similarity > 1.0f) cosine_similarity = 1.0f; if (cosine_similarity < -1.0f) cosine_similarity = -1.0f; return 1.0f - cosine_similarity; diff --git a/src/distance-sse2.c b/src/distance-sse2.c index 06db316..9b16025 100644 --- a/src/distance-sse2.c +++ b/src/distance-sse2.c @@ -786,9 +786,11 @@ float uint8_distance_cosine_sse2 (const void *v1, const void *v2, int n) { _mm_storeu_si128((__m128i *)a2, acc_a2); _mm_storeu_si128((__m128i *)b2, acc_b2); - int32_t total_dot = dot[0] + dot[1] + dot[2] + dot[3]; - int32_t total_a2 = a2[0] + a2[1] + a2[2] + a2[3]; - int32_t total_b2 = b2[0] + b2[1] + b2[2] + b2[3]; + // 64-bit totals, as the NEON/AVX2/AVX-512 kernels use: the four lanes together can + // hold more than a 32-bit sum, and the scalar tail adds to the same running total + int64_t total_dot = (int64_t)dot[0] + dot[1] + dot[2] + dot[3]; + int64_t total_a2 = (int64_t)a2[0] + a2[1] + a2[2] + a2[3]; + int64_t total_b2 = (int64_t)b2[0] + b2[1] + b2[2] + b2[3]; for (; i < n; ++i) { int va = (int)a[i]; @@ -798,9 +800,9 @@ float uint8_distance_cosine_sse2 (const void *v1, const void *v2, int n) { total_b2 += vb * vb; } - float denom = sqrtf((float)total_a2 * (float)total_b2); - if (denom == 0.0f) return 1.0f; // orthogonal or zero - float cosine_sim = total_dot / denom; + double denom = sqrt((double)total_a2) * sqrt((double)total_b2); + if (denom == 0.0) return 1.0f; // orthogonal or zero + float cosine_sim = (float)((double)total_dot / denom); if (cosine_sim > 1.0f) cosine_sim = 1.0f; if (cosine_sim < -1.0f) cosine_sim = -1.0f; return 1.0f - cosine_sim; // cosine distance @@ -1003,9 +1005,11 @@ float int8_distance_cosine_sse2 (const void *v1, const void *v2, int n) { _mm_storeu_si128((__m128i *)_a, acc_a2); _mm_storeu_si128((__m128i *)_b, acc_b2); - int32_t total_dot = _d[0] + _d[1] + _d[2] + _d[3]; - int32_t total_a2 = _a[0] + _a[1] + _a[2] + _a[3]; - int32_t total_b2 = _b[0] + _b[1] + _b[2] + _b[3]; + // 64-bit totals, as the NEON/AVX2/AVX-512 kernels use: the four lanes together can + // hold more than a 32-bit sum, and the scalar tail adds to the same running total + int64_t total_dot = (int64_t)_d[0] + _d[1] + _d[2] + _d[3]; + int64_t total_a2 = (int64_t)_a[0] + _a[1] + _a[2] + _a[3]; + int64_t total_b2 = (int64_t)_b[0] + _b[1] + _b[2] + _b[3]; // Handle tail for (; i < n; ++i) { @@ -1016,9 +1020,9 @@ float int8_distance_cosine_sse2 (const void *v1, const void *v2, int n) { total_b2 += vb * vb; } - float denom = sqrtf((float)total_a2 * (float)total_b2); - if (denom == 0.0f) return 1.0f; - float cosine_sim = total_dot / denom; + double denom = sqrt((double)total_a2) * sqrt((double)total_b2); + if (denom == 0.0) return 1.0f; + float cosine_sim = (float)((double)total_dot / denom); if (cosine_sim > 1.0f) cosine_sim = 1.0f; if (cosine_sim < -1.0f) cosine_sim = -1.0f; return 1.0f - cosine_sim; diff --git a/src/sqlite-vector.c b/src/sqlite-vector.c index 9048b99..883b0a5 100644 --- a/src/sqlite-vector.c +++ b/src/sqlite-vector.c @@ -552,7 +552,11 @@ static int sqlite_unserialize (sqlite3_context *context, table_context *ctx) { if (rc == SQLITE_DONE) {rc = SQLITE_OK; break;} if (rc != SQLITE_ROW) break; + // _sqliteai_vector is an ordinary writable table, and PRIMARY KEY on a rowid + // table does not imply NOT NULL: a row with a NULL key used to reach strcmp() const char *key = (const char *)sqlite3_column_text(vm, 0); + if (!key) continue; + if (strcmp(key, OPTION_KEY_QUANTTYPE) == 0) { ctx->options.q_type = (vector_qtype)sqlite3_column_int(vm, 1); continue; @@ -1584,10 +1588,15 @@ static bool parse_keyvalue_string (sqlite3_context *context, const char *str, ke return true; } -static uint64_t human_to_number (const char *s) { +// Returns false when the string is not a size at all, and writes the value out otherwise. +// The previous version signalled failure by returning 0, which also made a literal "0" +// unparseable - and zero is the one value the quantizer reads as "use whatever memory the +// index needs", so that setting could not be expressed through the option string. +static bool human_to_number (const char *s, uint64_t *out) { char *end = NULL; double d = strtod(s, &end); - if ((d == 0) || (d == HUGE_VAL)) return 0; + if (end == s) return false; // no digits at all + if (!isfinite(d)) return false; // overflowed to HUGE_VAL, or a NaN literal // skip whitespace before suffix SKIP_SPACES(end); @@ -1596,11 +1605,12 @@ static uint64_t human_to_number (const char *s) { if (strncasecmp(end, "KB", 2) == 0) d *= 1024; else if (strncasecmp(end, "MB", 2) == 0) d *= 1024 * 1024; else if (strncasecmp(end, "GB", 2) == 0) d *= 1024 * 1024 * 1024; - else if (*end != 0) return 0; // invalid suffix + else if (*end != 0) return false; // invalid suffix // sanity check - if (d < 0 || d > (double)INT64_MAX) return 0; - return (uint64_t)d; + if (d < 0 || d > (double)INT64_MAX) return false; + *out = (uint64_t)d; + return true; } bool vector_keyvalue_callback (sqlite3_context *context, void *xdata, const char *key, int key_len, const char *value, int value_len) { @@ -1639,8 +1649,11 @@ bool vector_keyvalue_callback (sqlite3_context *context, void *xdata, const char } if (KEY_MATCH(OPTION_KEY_MAXMEMORY)) { - uint64_t max_memory = human_to_number(buffer); - if (max_memory > 0) options->max_memory = max_memory; + uint64_t max_memory = 0; + // reported like every other unparseable option instead of being dropped, and 0 is + // now a value you can ask for: it means "use as much memory as the index needs" + if (!human_to_number(buffer, &max_memory)) return context_result_error(context, SQLITE_ERROR, "Invalid max_memory value: expected a size such as 64MB, got '%s'", buffer); + options->max_memory = max_memory; return true; } @@ -1901,9 +1914,10 @@ static int vector_rebuild_quantization (sqlite3_context *context, const char *ta // max_memory == 0 means use all required memory if (max_memory == 0) { sqlite3_snprintf(sizeof(sql), sql, "SELECT COUNT(*) FROM \"%w\";", table_name); - int64_t count = sqlite_read_int64(db, sql); - max_memory = (count == 0) ? DEFAULT_MAX_MEMORY : (uint64_t)count * (uint64_t)q_size; - if (count <= 0) { + // named apart from the caller's out-parameter, which it used to shadow + int64_t nrows = sqlite_read_int64(db, sql); + max_memory = (nrows == 0) ? DEFAULT_MAX_MEMORY : (uint64_t)nrows * (uint64_t)q_size; + if (nrows <= 0) { // no vectors t_ctx->options.q_type = (qtype == VECTOR_QUANT_AUTO) ? VECTOR_QUANT_U8BIT : qtype; t_ctx->options.q_bits = q_bits; @@ -2197,7 +2211,9 @@ static void vector_quantize_preload (sqlite3_context *context, int argc, sqlite3 int bytes = sqlite3_column_bytes(vm, 1); const uint8_t *data = (const uint8_t *)sqlite3_column_blob(vm, 1); - if (!data || n < 0 || bytes < 0 || (sqlite3_int64)bytes > required - seek) { + // n <= 0: every chunk this extension writes holds at least one vector, so a + // chunk claiming none is malformed and is caught here rather than at query time + if (!data || n <= 0 || bytes < 0 || (sqlite3_int64)bytes > required - seek) { rc = SQLITE_CORRUPT; break; } @@ -2241,6 +2257,16 @@ static int vector_quantize (sqlite3_context *context, const char *table_name, co char sql[STATIC_SQL_SIZE]; sqlite3 *db = sqlite3_context_db_handle(context); + // vector_rebuild_quantization() writes q_type/q_bits/scale/offset into t_ctx before + // the pass that can still fail, and the cleanup path below rolls the shadow table + // back. Snapshot them here so the in-memory description of the index goes back too: + // otherwise a failed call left the context describing a layout the table no longer + // has, and the next scan read the chunks at the wrong stride - wrong rowids, wrong + // distances, no error. Taken before the first goto so the label can always restore. + const vector_options saved_options = t_ctx->options; + const float saved_scale = t_ctx->scale; + const float saved_offset = t_ctx->offset; + bool savepoint_open = false; rc = sqlite3_exec(db, "SAVEPOINT quantize;", NULL, NULL, NULL); if (rc != SQLITE_OK) goto quantize_cleanup; @@ -2294,6 +2320,22 @@ quantize_cleanup: { sqlite3_exec(db, "RELEASE quantize;", NULL, NULL, NULL); } + // put the context back in step with the table the rollback restored. The turbo + // cache is keyed by (bits, dim) and rebuild() drops it whenever it quantizes to + // anything else, so a restored TURBO index needs it built again; if that fails + // the cache simply stays absent and the scans report SQLITE_MISUSE rather than + // reading at the wrong stride. + sqlite3_mutex_enter(qmutex); + t_ctx->options = saved_options; + t_ctx->scale = saved_scale; + t_ctx->offset = saved_offset; + if (t_ctx->options.q_type == VECTOR_QUANT_TURBO) { + if (table_context_ensure_turbo_codebook(t_ctx, t_ctx->options.q_bits, t_ctx->options.v_dim) == SQLITE_OK) { + table_context_ensure_turbo_plan(t_ctx, t_ctx->options.v_dim); + } + } + sqlite3_mutex_leave(qmutex); + // only replace the message when SQLite actually has one: the callees set their own // through context_result_error, and overwriting it reported "not an error" if (errmsg) { @@ -2529,19 +2571,33 @@ static void vector_as_type (sqlite3_context *context, vector_type type, int argc size_t expected_size = vector_bytes_for_dim(type, dimension); if ((size_t)value_size != expected_size) { context_result_error(context, SQLITE_ERROR, - "Invalid BLOB size for format '%s': expected %d bytes for %d dimensions (got %d bytes)", - vector_type_to_name(type), (int)expected_size, dimension, value_size); + "Invalid BLOB size for format '%s': expected %lld bytes for %d dimensions (got %d bytes)", + vector_type_to_name(type), (long long)expected_size, dimension, value_size); return; } } } else { - // the only check we can perform is that the blob size is an exact multiplier of the vector type + // without a dimension the only check we can perform is that the blob size is + // an exact multiplier of the vector type if (value_size % vector_type_to_size(type) != 0) { context_result_error(context, SQLITE_ERROR, "Invalid BLOB size for format '%s': size must be a multiple of %d bytes", vector_type_to_name(type), vector_type_to_size(type)); return; } + + // with one, hold the blob to it, as the BIT branch above and the JSON branch + // below already do: a wrong-sized vector is otherwise stored happily and then + // skipped in silence by every scan, so it never appears in any result + if (dimension > 0) { + size_t expected_size = vector_bytes_for_dim(type, dimension); + if ((size_t)value_size != expected_size) { + context_result_error(context, SQLITE_ERROR, + "Invalid BLOB size for format '%s': expected %lld bytes for %d dimensions (got %d bytes)", + vector_type_to_name(type), (long long)expected_size, dimension, value_size); + return; + } + } } sqlite3_result_value(context, value); @@ -2736,22 +2792,29 @@ static int vCursorFilterCommon (sqlite3_vtab_cursor *cur, int idxNum, const char } if (c->row_count != k) { + // sqlite3_malloc() takes an int: k * sizeof() is a size_t and k comes straight + // from the query, so the byte count was silently truncated - k=536870913 asked + // for 0x100000008 bytes and got 8, which the memset below then overran. + // sqlite3_malloc64() refuses anything near 2GB, so an absurd k is a clean NOMEM. + // + // row_count also has to drop to zero before either allocation: it is what tells + // the next filter whether these arrays can be reused, and a cursor that failed + // half way through used to keep the old count next to one freed array. if (c->rowids) sqlite3_free(c->rowids); - c->rowids = (int64_t *)sqlite3_malloc(k * sizeof(int64_t)); - if (c->rowids == NULL) { - if (vector_allocated) sqlite3_free((void *)vector); - return SQLITE_NOMEM; - } - if (c->distance) sqlite3_free(c->distance); - c->distance = (double *)sqlite3_malloc(k * sizeof(double)); - if (c->distance == NULL) { + c->rowids = NULL; + c->distance = NULL; + c->row_count = 0; + + c->rowids = (int64_t *)sqlite3_malloc64((sqlite3_uint64)k * sizeof(int64_t)); + c->distance = (double *)sqlite3_malloc64((sqlite3_uint64)k * sizeof(double)); + if (c->rowids == NULL || c->distance == NULL) { if (vector_allocated) sqlite3_free((void *)vector); return SQLITE_NOMEM; } } - memset(c->rowids, 0, k * sizeof(int64_t)); + memset(c->rowids, 0, (size_t)k * sizeof(int64_t)); for (int i=0; idistance[i] = INFINITY; c->size = 0; @@ -2964,7 +3027,13 @@ static int vFullScanCursorNext (sqlite3_vtab_cursor *cur){ c->stream.data = (uint8_t *)sqlite3_column_blob(vm, 1); c->stream.data_bytes = sqlite3_column_bytes(vm, 1); c->stream.dindex = 0; - if (c->stream.data == NULL || c->stream.dcounter < 0 || c->stream.data_bytes < 0 || (sqlite3_uint64)c->stream.data_bytes < (sqlite3_uint64)c->stream.dcounter * (sqlite3_uint64)total_stride) { + // dcounter == 0 is this loop's "load the next chunk" sentinel, so a chunk + // that claims zero vectors has to be rejected and not merely skipped: the + // length check below degenerates to "bytes < 0" and the row would then be + // read from past the end of the blob. vector_serialize_quantization() never + // writes such a chunk, so seeing one means the shadow table was tampered + // with - the same thing the counter and length tests here already guard. + if (c->stream.data == NULL || c->stream.dcounter <= 0 || c->stream.data_bytes < 0 || (sqlite3_uint64)c->stream.data_bytes < (sqlite3_uint64)c->stream.dcounter * (sqlite3_uint64)total_stride) { return SQLITE_CORRUPT; } } @@ -3045,7 +3114,9 @@ static int vFullScanCursorNext (sqlite3_vtab_cursor *cur){ c->stream.data = (uint8_t *)sqlite3_column_blob(vm, 1); c->stream.data_bytes = sqlite3_column_bytes(vm, 1); c->stream.dindex = 0; // reset index for the new chunk - if (c->stream.data == NULL || c->stream.dcounter < 0 || c->stream.data_bytes < 0 || + // dcounter <= 0, not < 0: zero is this loop's "chunk exhausted" sentinel, so a + // chunk claiming no vectors would otherwise be read past the end of its blob + if (c->stream.data == NULL || c->stream.dcounter <= 0 || c->stream.data_bytes < 0 || (sqlite3_uint64)c->stream.data_bytes < (sqlite3_uint64)c->stream.dcounter * (sqlite3_uint64)total_stride) { return SQLITE_CORRUPT; } @@ -3206,9 +3277,11 @@ static int vQuantRunMemory(vFullScanCursor *c, const preload_index *idx, uint8_t const size_t total_stride = rowid_size + vector_size; // the preloaded index is built from an ordinary writable table, so never trust its - // row count against its length (the TurboQuant paths already do this) + // row count against its length (the TurboQuant paths already do this). A count of + // zero over a non-empty buffer is malformed too - preload refuses to build an index + // with no bytes at all, so every chunk in it must claim at least one vector const sqlite3_int64 data_bytes = idx->bytes; - if (!data || counter < 0 || data_bytes < 0 || (sqlite3_uint64)data_bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { + if (!data || counter <= 0 || data_bytes < 0 || (sqlite3_uint64)data_bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { sqlite_vtab_set_error(c->base.pVtab, "Corrupted quantization data preloaded for '%s.%s'", c->table->t_name, c->table->c_name); return SQLITE_CORRUPT; } @@ -3276,7 +3349,9 @@ static int vTurboRunPackedRows (vFullScanCursor *c, const uint8_t *data, sqlite3 vector_distance distance_type = c->table->options.v_distance; size_t packed_bytes = turbo_bytes_for_dim(dim, bits); size_t total_stride = sizeof(int64_t) + sizeof(float) + packed_bytes; - if (!data || counter < 0 || data_bytes < 0 || (sqlite3_uint64)data_bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { + // a chunk that carries bytes but claims no vectors is malformed, same as one whose + // count does not fit its length: reject it here as the streaming paths now do + if (!data || counter <= 0 || data_bytes < 0 || (sqlite3_uint64)data_bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { return SQLITE_CORRUPT; } @@ -3462,7 +3537,9 @@ static int vQuantRun (sqlite3 *db, vFullScanCursor *c, const void *v1, int v1siz int counter = sqlite3_column_int(vm, 0); uint8_t *data = (uint8_t *)sqlite3_column_blob(vm, 1); int bytes = sqlite3_column_bytes(vm, 1); - if (!data || counter < 0 || bytes < 0 || (sqlite3_uint64)bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { + // counter <= 0: a chunk with bytes but no vectors is as malformed as one whose + // count overruns its length, and the streaming paths reject it too + if (!data || counter <= 0 || bytes < 0 || (sqlite3_uint64)bytes < (sqlite3_uint64)counter * (sqlite3_uint64)total_stride) { sqlite_vtab_set_error(c->base.pVtab, "Corrupted quantization data for '%s.%s'", c->table->t_name, c->table->c_name); sqlite3_finalize(vm); sqlite3_free(v); diff --git a/test/test_vector.c b/test/test_vector.c index 1cec092..300c405 100644 --- a/test/test_vector.c +++ b/test/test_vector.c @@ -1007,6 +1007,231 @@ static void test_distance_functions_hamming(sqlite3 *db) { } } +/* ---------- Test: malformed input and failure paths ---------- */ + +/* Like exec_scan_sql but keeps the step() return code, which the tests below need in + order to tell "rejected cleanly" from "produced a row it should not have". */ +static int run_scan(sqlite3 *db, const char *sql, scan_result *r) { + sqlite3_stmt *stmt = NULL; + int rc = sqlite3_prepare_v2(db, sql, -1, &stmt, NULL); + if (rc != SQLITE_OK) return rc; + + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) { + if (r->count < 64) { + r->ids[r->count] = sqlite3_column_int(stmt, 0); + r->distances[r->count] = sqlite3_column_double(stmt, 1); + } + r->count++; + } + + sqlite3_finalize(stmt); + return (rc == SQLITE_DONE) ? SQLITE_OK : rc; +} + +static void test_malformed_input(sqlite3 *db) { + printf("\n=== Malformed input and failure paths ===\n"); + char sql[1024]; + + /* + * A k whose byte count overflows an int. sqlite3_malloc() takes an int, so + * k=536870913 asked for 0x100000008 bytes, got 8, and the memset that follows + * overran the top-k arrays. It has to fail cleanly, and leave a usable cursor. + */ + { + const char *tbl = "tbad_bigk"; + if (setup_table(db, tbl, "f32", "L2", 4, float_vecs, float_nvecs) != 0) { + ASSERT(0, "huge k setup"); + } else { + scan_result r = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_full_scan('%s','v',vector_as_f32('%s'),536870913);", + tbl, float_query); + int rc = run_scan(db, sql, &r); + ASSERT(rc != SQLITE_OK, "huge k is rejected instead of overflowing the top-k arrays"); + + scan_result r2 = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_full_scan('%s','v',vector_as_f32('%s'),3);", + tbl, float_query); + rc = run_scan(db, sql, &r2); + ASSERT(rc == SQLITE_OK && r2.count == 3, "a sane k still works after a huge one"); + } + } + + /* + * _sqliteai_vector is an ordinary writable table and PRIMARY KEY on a rowid table + * does not imply NOT NULL, so a row with a NULL key used to reach strcmp(NULL). + */ + { + const char *tbl = "tbad_nullkey"; + snprintf(sql, sizeof(sql), "CREATE TABLE \"%s\" (id INTEGER PRIMARY KEY, v BLOB);", tbl); + exec_sql(db, sql); + snprintf(sql, sizeof(sql), + "INSERT INTO _sqliteai_vector (tblname,colname,key,value) VALUES ('%s','v',NULL,1);", tbl); + int rc = exec_sql(db, sql); + ASSERT(rc == SQLITE_OK, "a NULL metadata key can be stored"); + + snprintf(sql, sizeof(sql), + "SELECT vector_init('%s','v','type=f32,dimension=4,distance=L2');", tbl); + rc = exec_sql(db, sql); + ASSERT(rc == SQLITE_OK, "vector_init skips a NULL metadata key instead of crashing"); + } + + /* + * A quant chunk that claims zero vectors. Zero is the streaming scans' "load the + * next chunk" sentinel, so such a chunk slipped past the length check and one row + * was read from past the end of its blob. + */ + { + const char *tbl = "tbad_zerocount"; + if (setup_table(db, tbl, "f32", "L2", 4, float_vecs, float_nvecs) != 0) { + ASSERT(0, "zero-count chunk setup"); + } else { + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=UINT8');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "zero-count chunk: quantize succeeds"); + + scan_result before = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'));", + tbl, float_query); + ASSERT(run_scan(db, sql, &before) == SQLITE_OK, "zero-count chunk: scan works before tampering"); + + snprintf(sql, sizeof(sql), + "INSERT INTO \"vector0_%s_v\" (rowid1,rowid2,counter,data) VALUES (0,0,0,x'00');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "zero-count chunk can be stored"); + + scan_result streaming = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'));", + tbl, float_query); + int rc = run_scan(db, sql, &streaming); + ASSERT(rc != SQLITE_OK, "streaming scan rejects a chunk claiming zero vectors"); + ASSERT(streaming.count <= before.count, "streaming scan invents no extra row"); + + scan_result topk = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'),5);", + tbl, float_query); + rc = run_scan(db, sql, &topk); + ASSERT(rc != SQLITE_OK, "top-k scan rejects a chunk claiming zero vectors"); + } + } + + /* + * A vector_quantize that fails part way through. The savepoint rolls the shadow + * table back, so the in-memory q_type/scale/offset have to go back too: otherwise + * the next scan reads the chunks at the wrong stride and reports made-up rowids. + * 1BIT skips the min/max pass, so the undersized blob is caught in the later one. + */ + { + const char *tbl = "tbad_rollback"; + if (setup_table(db, tbl, "f32", "L2", 4, float_vecs, float_nvecs) != 0) { + ASSERT(0, "quantize rollback setup"); + } else { + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=UINT8');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "quantize rollback: UINT8 quantize succeeds"); + + scan_result before = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'),5) ORDER BY distance;", + tbl, float_query); + ASSERT(run_scan(db, sql, &before) == SQLITE_OK, "quantize rollback: scan works before"); + + snprintf(sql, sizeof(sql), "INSERT INTO \"%s\" (id, v) VALUES (9999, x'0001');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "quantize rollback: undersized blob stored"); + + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=1BIT');", tbl); + ASSERT(exec_sql(db, sql) != SQLITE_OK, "quantize rollback: 1BIT quantize fails on the bad blob"); + + snprintf(sql, sizeof(sql), "DELETE FROM \"%s\" WHERE id = 9999;", tbl); + exec_sql(db, sql); + + scan_result after = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'),5) ORDER BY distance;", + tbl, float_query); + ASSERT(run_scan(db, sql, &after) == SQLITE_OK, "quantize rollback: scan still works after the failure"); + ASSERT(after.count == before.count, "quantize rollback: same number of rows as before"); + + int same = (after.count == before.count); + for (int i = 0; same && i < after.count; i++) { + if (after.ids[i] != before.ids[i]) same = 0; + if (after.distances[i] != before.distances[i]) same = 0; + } + ASSERT(same, "quantize rollback: same rowids and distances as before the failed call"); + } + } + + /* + * The optional dimension argument has to hold a BLOB to the size it names, as it + * already did for TEXT and for BIT: a wrong-sized vector is otherwise stored and + * then skipped in silence by every scan. + */ + { + sqlite3_stmt *stmt = NULL; + int rc = sqlite3_prepare_v2(db, "SELECT length(vector_as_f32(x'0000000000000000', 4));", -1, &stmt, NULL); + if (rc == SQLITE_OK) rc = sqlite3_step(stmt); + ASSERT(rc != SQLITE_ROW, "vector_as_f32 rejects a BLOB that does not match the given dimension"); + sqlite3_finalize(stmt); + + stmt = NULL; + rc = sqlite3_prepare_v2(db, "SELECT length(vector_as_f32(x'0000000000000000', 2));", -1, &stmt, NULL); + if (rc == SQLITE_OK) rc = sqlite3_step(stmt); + ASSERT(rc == SQLITE_ROW && sqlite3_column_int(stmt, 0) == 8, "vector_as_f32 accepts a BLOB of the right dimension"); + sqlite3_finalize(stmt); + + /* + * A dimension whose byte count does not fit in an int. The size is a size_t and + * the message has to report it whole: with a %d it printed "expected 0 bytes". + * context_result_error() carries no printf format attribute, so nothing but a + * runtime check catches this. + */ + stmt = NULL; + rc = sqlite3_prepare_v2(db, "SELECT vector_as_f32(x'00000000', 1073741824);", -1, &stmt, NULL); + if (rc == SQLITE_OK) rc = sqlite3_step(stmt); + ASSERT(rc != SQLITE_ROW, "vector_as_f32 rejects a dimension whose byte count overflows an int"); + { + const char *msg = sqlite3_errmsg(db); + ASSERT(msg && strstr(msg, "4294967296") != NULL, + "the rejection reports the untruncated expected size"); + if (!msg || !strstr(msg, "4294967296")) printf(" msg: %s\n", msg ? msg : "(none)"); + } + sqlite3_finalize(stmt); + + stmt = NULL; + rc = sqlite3_prepare_v2(db, "SELECT length(vector_as_f32(x'0000000000000000'));", -1, &stmt, NULL); + if (rc == SQLITE_OK) rc = sqlite3_step(stmt); + ASSERT(rc == SQLITE_ROW && sqlite3_column_int(stmt, 0) == 8, "vector_as_f32 still accepts a BLOB with no dimension given"); + sqlite3_finalize(stmt); + } + + /* + * max_memory: 0 is a real setting ("use whatever the index needs") and used to be + * indistinguishable from a parse failure, which was itself silently ignored. + */ + { + const char *tbl = "tbad_maxmemory"; + if (setup_table(db, tbl, "f32", "L2", 4, float_vecs, float_nvecs) != 0) { + ASSERT(0, "max_memory setup"); + } else { + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=UINT8,max_memory=0');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "max_memory=0 is accepted"); + + scan_result r = {0}; + snprintf(sql, sizeof(sql), + "SELECT id, distance FROM vector_quantize_scan('%s','v',vector_as_f32('%s'),5);", + tbl, float_query); + ASSERT(run_scan(db, sql, &r) == SQLITE_OK && r.count == 5, "max_memory=0 quantizes a usable index"); + + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=UINT8,max_memory=banana');", tbl); + ASSERT(exec_sql(db, sql) != SQLITE_OK, "an unparseable max_memory is reported, not ignored"); + + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','v','qtype=UINT8,max_memory=1MB');", tbl); + ASSERT(exec_sql(db, sql) == SQLITE_OK, "max_memory with a unit suffix is accepted"); + } + } +} + /* ---------- Main ---------- */ int main(void) { @@ -1218,6 +1443,9 @@ int main(void) { test_distance_functions_hamming(db); } + /* 6. malformed input and failure paths */ + test_malformed_input(db); + sqlite3_close(db); From 40bc5b53a11c3807de6b7b2d051453279379314c Mon Sep 17 00:00:00 2001 From: Marco Bambini Date: Sun, 1 Nov 2026 08:15:09 +0100 Subject: [PATCH 2/3] fix: two memory-safety bugs found reviewing the existing code A security review of all of src/, not only this branch's own changes, turned up a heap buffer overflow and a null write. Both are reachable from ordinary SQL and both were reproduced under AddressSanitizer before being fixed. vector_from_json() sized its buffer and bounded its writes from two different numbers. The buffer came from sqlite3_malloc((int)alloc), which truncated a size_t to that function's int parameter, while max_count was still derived from the untruncated alloc. A JSON array with 2^30 commas therefore got an 8-byte buffer and a bound of 1073741825 elements, and the parse loop wrote floats of the caller's choosing straight past it; the comma count sizes the buffer to the byte, so this was a precise write primitive rather than a crash. There is now a single int64_t capacity: the allocation goes through sqlite3_malloc64() and the loop checks against that same capacity instead of a figure recovered from the allocation, so the two cannot drift apart however the arithmetic behaves. Reaching it took a TEXT value of about a gigabyte, so a host built with SQLITE_MAX_LENGTH >= 1073741824 - the amalgamation bundled here uses the 1e9 default, but this ships as a loadable extension and runs against the host's limits, not against these. The declared dimension was only checked for being positive, so it reached INT_MAX and overflowed the size arithmetic downstream. In turbo_rotation_plan_init() the expression (dim/2)*12 wraps negative past 357913941, which skipped the allocation of the rotation plan's coordinate arrays - they sit behind if (pair_count > 0), and so does the test that would have caught their absence - and the loop then wrote through the resulting null pointer. A vector_init with dimension=400000000 followed by a TurboQuant vector_quantize crashed the process on an empty table, since the plan is built before any row is read. The dimension is now range-checked where it is parsed, upstream of every one of those computations, against a MAX_VECTOR_DIMENSION of 2^24: three orders of magnitude above any real embedding, and small enough that (dim/2)*12 stays near a hundred million. It is also held in a long until it has been checked, because casting strtol()'s result to int first wrapped and let "4294967300" through as the dimension 4. That same bound now applies to the optional dimension argument of vector_as_*(), whose size check goes through vector_bytes_for_dim(): dim * item_size wraps where size_t is 32 bits, and a wrapped product would let a wrong-sized blob match. That check was added two commits ago, so this closes a gap of its own making. One consequence worth recording: with the dimension bounded, no accepted value yields a byte count above INT_MAX, so the %lld conversion in those rejection messages is no longer observable from SQL. Its test now pins the message on an in-range mismatch and checks separately that a dimension past the bound is refused before any size is computed at all. The gigabyte case is covered by a standalone probe rather than by the suite, since running it needs that much memory and a different SQLITE_MAX_LENGTH than the project builds with. Everything else is in test_malformed_input(). make unittest: 1482 passed, 0 failed, and the same under ASan and UBSan. Co-Authored-By: Claude Opus 5 --- src/sqlite-vector.c | 64 +++++++++++++++++++++++++++++++++---------- test/test_vector.c | 67 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 107 insertions(+), 24 deletions(-) diff --git a/src/sqlite-vector.c b/src/sqlite-vector.c index 883b0a5..773b27d 100644 --- a/src/sqlite-vector.c +++ b/src/sqlite-vector.c @@ -77,6 +77,14 @@ SQLITE_EXTENSION_INIT1 #define MAX_TABLES 128 #define STATIC_SQL_SIZE 2048 +// A sanity bound on the declared vector dimension, which otherwise reached INT_MAX and +// overflowed the size arithmetic downstream: (dim/2)*12 in turbo_rotation_plan_init() +// wrapped negative past 357913941, which left its coordinate arrays unallocated and the +// plan reported as valid, and the (dim + 7) expressions in the packing helpers are signed +// overflow near the top of the range. 16M is some three orders of magnitude above any +// real embedding and keeps every one of those computations far inside int. +#define MAX_VECTOR_DIMENSION (1 << 24) + #define INT64_TO_INT8PTR(_val, _ptr) do { \ (_ptr)[0] = (int8_t)(((_val) >> 0) & 0xFF); \ (_ptr)[1] = (int8_t)(((_val) >> 8) & 0xFF); \ @@ -1636,9 +1644,13 @@ bool vector_keyvalue_callback (sqlite3_context *context, void *xdata, const char } if (KEY_MATCH(OPTION_KEY_DIMENSION)) { - int dimension = (int)strtol(buffer, NULL, 0); - if (dimension <= 0) return context_result_error(context, SQLITE_ERROR, "Invalid vector dimension: expected a positive integer, got '%s'", buffer); - options->v_dim = dimension; + // kept as a long until it has been range-checked: casting strtol()'s result to + // int first silently wrapped, so "4294967300" came through as the dimension 4 + long dimension = strtol(buffer, NULL, 0); + if (dimension <= 0 || dimension > MAX_VECTOR_DIMENSION) { + return context_result_error(context, SQLITE_ERROR, "Invalid vector dimension: expected an integer between 1 and %d, got '%s'", MAX_VECTOR_DIMENSION, buffer); + } + options->v_dim = (int)dimension; return true; } @@ -2424,22 +2436,33 @@ static void *vector_from_json (sqlite3_context *context, sqlite3_vtab *vtab, vec json++; - // count number of commas to estimate (over-estimate) the number of entries in the json - int estimated_count = 0; + // count number of commas to estimate (over-estimate) the number of entries in the + // json: a well-formed array of n elements carries n-1 commas, and a malformed one + // only ever parses into fewer. 64-bit, because a comma per byte of a maximum-length + // TEXT value overflows an int counter. + int64_t capacity = 1; for (const char *p = json; *p; ++p) { - if (*p == ',') estimated_count++; + if (*p == ',') ++capacity; } - + // allocate blob // For BIT type, each JSON element is a single bit, pack 8 per byte size_t item_size = vector_type_to_size(type); - size_t alloc = (type == VECTOR_TYPE_BIT) ? ((estimated_count + 1) + 7) / 8 : (estimated_count + 1) * item_size; - blob = sqlite3_malloc((int)alloc); + // sqlite3_malloc() takes an int, so this byte count used to be truncated from a + // size_t while the write bound below was still derived from the untruncated value: a + // JSON array with 2^30 commas got an 8-byte buffer and a billion-element bound, and + // the parse loop wrote straight past it. The bound is now this same capacity rather + // than a figure recovered from the allocation, and sqlite3_malloc64() refuses + // anything near 2GB instead of wrapping. + sqlite3_uint64 alloc = (type == VECTOR_TYPE_BIT) + ? (sqlite3_uint64)((capacity + 7) / 8) + : (sqlite3_uint64)capacity * (sqlite3_uint64)item_size; + blob = sqlite3_malloc64(alloc); if (!blob) { return sqlite_common_set_error(context, vtab, SQLITE_NOMEM, "Out of memory: unable to allocate %lld bytes for BLOB buffer", (long long)alloc); } if (type == VECTOR_TYPE_BIT) { - memset(blob, 0, alloc); // Initialize to zero for bit packing + memset(blob, 0, (size_t)alloc); // Initialize to zero for bit packing } // typed pointers @@ -2469,9 +2492,9 @@ static void *vector_from_json (sqlite3_context *context, sqlite3_vtab *vtab, vec return sqlite_common_set_error(context, vtab, SQLITE_ERROR, "Malformed JSON: expected a number at position %d (found '%c')", (int)(p - json) + 1, *p ? *p : '?'); } - // check bounds - int max_count = (type == VECTOR_TYPE_BIT) ? (int)(alloc * 8) : (int)(alloc / item_size); - if (count >= max_count) { + // check bounds against the capacity the buffer was sized for, never against a + // count recovered from the allocation + if ((int64_t)count >= capacity) { sqlite3_free(blob); return sqlite_common_set_error(context, vtab, SQLITE_ERROR, "Too many elements in JSON array"); } @@ -2551,7 +2574,10 @@ static void *vector_from_json (sqlite3_context *context, sqlite3_vtab *vtab, vec return sqlite_common_set_error(context, vtab, SQLITE_ERROR, "Invalid JSON vector dimension: expected %d but found %d", dimension, count); } - if (size) *size = (type == VECTOR_TYPE_BIT) ? (int)((count + 7) / 8) : (int)(count * item_size); + // 64-bit arithmetic here too: the allocation above already caps count low enough that + // neither expression can overflow, but computing it in int left that guarantee resting + // on the allocator's limit rather than on anything stated locally + if (size) *size = (type == VECTOR_TYPE_BIT) ? (int)(((int64_t)count + 7) / 8) : (int)((int64_t)count * (int64_t)item_size); return blob; } @@ -2562,7 +2588,15 @@ static void vector_as_type (sqlite3_context *context, vector_type type, int argc // dimension is an optional argument int dimension = (argc == 2) ? sqlite3_value_int(argv[1]) : 0; - + + // held to the same bound as the dimension option, for the same reason: the size + // checks below go through vector_bytes_for_dim(), whose dim * item_size wraps where + // size_t is 32 bits, and a wrapped product would let a wrong-sized blob match + if (dimension > MAX_VECTOR_DIMENSION) { + context_result_error(context, SQLITE_ERROR, "Invalid dimension: expected an integer between 1 and %d, got %d", MAX_VECTOR_DIMENSION, dimension); + return; + } + if (value_type == SQLITE_BLOB) { if (type == VECTOR_TYPE_BIT) { // For bit vectors, any size is valid (dimensions = size * 8, minus padding) diff --git a/test/test_vector.c b/test/test_vector.c index 300c405..93ba354 100644 --- a/test/test_vector.c +++ b/test/test_vector.c @@ -1181,23 +1181,28 @@ static void test_malformed_input(sqlite3 *db) { sqlite3_finalize(stmt); /* - * A dimension whose byte count does not fit in an int. The size is a size_t and - * the message has to report it whole: with a %d it printed "expected 0 bytes". - * context_result_error() carries no printf format attribute, so nothing but a - * runtime check catches this. + * The size in the rejection message is a size_t behind a %lld, so it has to be + * reported whole. context_result_error() carries no printf format attribute, so + * nothing but a runtime check covers the conversion. */ stmt = NULL; - rc = sqlite3_prepare_v2(db, "SELECT vector_as_f32(x'00000000', 1073741824);", -1, &stmt, NULL); + rc = sqlite3_prepare_v2(db, "SELECT vector_as_f32(x'00000000', 16777216);", -1, &stmt, NULL); if (rc == SQLITE_OK) rc = sqlite3_step(stmt); - ASSERT(rc != SQLITE_ROW, "vector_as_f32 rejects a dimension whose byte count overflows an int"); + ASSERT(rc != SQLITE_ROW, "vector_as_f32 rejects a 4-byte BLOB offered as 16777216 dimensions"); { const char *msg = sqlite3_errmsg(db); - ASSERT(msg && strstr(msg, "4294967296") != NULL, - "the rejection reports the untruncated expected size"); - if (!msg || !strstr(msg, "4294967296")) printf(" msg: %s\n", msg ? msg : "(none)"); + ASSERT(msg && strstr(msg, "67108864") != NULL, "the rejection reports the expected size"); + if (!msg || !strstr(msg, "67108864")) printf(" msg: %s\n", msg ? msg : "(none)"); } sqlite3_finalize(stmt); + /* past the dimension bound it is refused before any size is computed at all */ + stmt = NULL; + rc = sqlite3_prepare_v2(db, "SELECT vector_as_f32(x'00000000', 1073741824);", -1, &stmt, NULL); + if (rc == SQLITE_OK) rc = sqlite3_step(stmt); + ASSERT(rc != SQLITE_ROW, "vector_as_f32 rejects a dimension past the bound"); + sqlite3_finalize(stmt); + stmt = NULL; rc = sqlite3_prepare_v2(db, "SELECT length(vector_as_f32(x'0000000000000000'));", -1, &stmt, NULL); if (rc == SQLITE_OK) rc = sqlite3_step(stmt); @@ -1205,6 +1210,50 @@ static void test_malformed_input(sqlite3 *db) { sqlite3_finalize(stmt); } + /* + * An unbounded dimension overflowed the size arithmetic downstream. The one that + * crashed outright: (dim/2)*12 in turbo_rotation_plan_init() wraps negative past + * 357913941, which skipped the allocation of the coordinate arrays and still + * reported the plan as valid, so building a TurboQuant index on an EMPTY table + * wrote through a NULL pointer. The dimension is now range-checked where it is + * parsed, which is upstream of every one of those computations. + */ + { + const char *tbl = "tbad_dim"; + snprintf(sql, sizeof(sql), "CREATE TABLE \"%s\" (id INTEGER PRIMARY KEY, v BLOB);", tbl); + exec_sql(db, sql); + + struct { const char *dim; int ok; const char *what; } cases[] = { + {"4", 1, "a small dimension is accepted"}, + {"16777216", 1, "the largest allowed dimension is accepted"}, + {"16777217", 0, "one past the bound is rejected"}, + {"400000000", 0, "the dimension that overflowed the TurboQuant plan is rejected"}, + {"2147483647", 0, "INT_MAX is rejected"}, + /* (int)strtol() used to wrap this to 4 and accept it as a valid dimension */ + {"4294967300", 0, "a value above INT_MAX is rejected, not wrapped"}, + }; + + for (unsigned i = 0; i < sizeof(cases) / sizeof(*cases); i++) { + char msg[160]; + /* a fresh column name each time: vector_init keeps the first dimension it + is given for a table/column pair and rejects later disagreements */ + snprintf(sql, sizeof(sql), "ALTER TABLE \"%s\" ADD COLUMN c%u BLOB;", tbl, i); + exec_sql(db, sql); + snprintf(sql, sizeof(sql), "SELECT vector_init('%s','c%u','type=f32,dimension=%s');", tbl, i, cases[i].dim); + int rc = exec_sql(db, sql); + snprintf(msg, sizeof(msg), "dimension=%s: %s", cases[i].dim, cases[i].what); + ASSERT(cases[i].ok ? (rc == SQLITE_OK) : (rc != SQLITE_OK), msg); + } + + /* and the quantize that used to crash is now unreachable: init refuses first */ + snprintf(sql, sizeof(sql), "ALTER TABLE \"%s\" ADD COLUMN turbo BLOB;", tbl); + exec_sql(db, sql); + snprintf(sql, sizeof(sql), "SELECT vector_init('%s','turbo','type=f32,dimension=400000000,distance=COSINE');", tbl); + ASSERT(exec_sql(db, sql) != SQLITE_OK, "the TurboQuant crash case is refused at vector_init"); + snprintf(sql, sizeof(sql), "SELECT vector_quantize('%s','turbo','qtype=TURBO');", tbl); + ASSERT(exec_sql(db, sql) != SQLITE_OK, "and vector_quantize on it reports no context instead of crashing"); + } + /* * max_memory: 0 is a real setting ("use whatever the index needs") and used to be * indistinguishable from a parse failure, which was itself silently ignored. From 62daad762e7d9d689ccba05651bbc42aa19f7aa2 Mon Sep 17 00:00:00 2001 From: Marco Bambini Date: Sun, 1 Nov 2026 08:19:22 +0100 Subject: [PATCH 3/3] Bump version to 1.2.0 Promotes the Unreleased section to 1.2.0 and records this branch's ten fixes under it. That section's lead paragraph had to go: it said the extension was unchanged from 1.1.0 and that a build produced the same binaries, which was true while Unreleased held only benchmark and README work, and is not true of a release carrying three out-of-bounds writes. Those three are called out under Security with how each is reached, because this file is what the release tag publishes to the website and what somebody reads to decide whether to upgrade. Package.swift is deliberately untouched. Its URL and checksum name a release artifact that does not exist yet, and CI rewrites both once the tag publishes one, in the commit it marks as an automated update. Bumping the URL by hand now would leave Swift consumers pointing at 1.2.0 with 1.1.2's checksum. (Deliberately not quoting CI's sentinel string here: main.yml gates every job on the head commit message not containing it, and the whole build skipped when it did.) Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 65 ++++++++++++++++++++++++++++++++++++++++++--- src/sqlite-vector.h | 2 +- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 462ef4c..360981c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). -## [Unreleased] +## [1.2.0] - 2026-11-01 -Documentation and tooling only — the extension itself is unchanged from 1.1.0, so a build -from this revision produces the same binaries. +A read-through of the sources, followed by a security review of all of `src/`, turned up +ten bugs. Three of them write outside their buffers and are reachable from ordinary SQL, +so upgrading is recommended for anyone whose application lets untrusted input reach a +connection with the extension loaded. No API changed and no index needs rebuilding. ### Changed @@ -32,6 +34,63 @@ from this revision produces the same binaries. faster** than the exact scan — it does beat brute force. What holds is the comparison that mattered: `INT8` is 4x faster again at twice the index size, so TurboQuant's argument is memory rather than throughput. +- **A `NULL` key in `_sqliteai_vector` crashed the next `vector_init()`.** That table is + an ordinary writable one, and `PRIMARY KEY` on a rowid table does not imply `NOT NULL`, + so the key went to `strcmp()` unchecked. Such rows are now skipped. +- **A failed `vector_quantize()` left the extension describing an index that no longer + existed.** The quantization type, bit width, scale and offset are written into the table + context before the pass that can still fail, and the savepoint rolls the shadow table + back without them. Scans then read the chunks at the wrong stride and reported made-up + rowids and distances with no error at all. Those fields, and the TurboQuant cache, are + now restored along with the rollback. +- **A quant chunk claiming zero vectors was read past the end of its blob.** Zero is the + streaming scans' "load the next chunk" sentinel, so such a chunk slipped past the length + check beside it, which degenerates to `bytes < 0`, and one fabricated row came back. + Every count test now rejects it, and `vector_quantize_preload()` catches it at load time + rather than leaving it to query time. +- **`vector_as_*()` ignored its optional dimension argument for BLOB input**, though it is + documented as a stricter check and the `BIT` and JSON paths already enforced it. A + wrong-sized vector was stored and then skipped in silence by every scan, so it never + appeared in a result. +- **`max_memory=0` could not be set, and an unparseable value was dropped silently.** Zero + is the one value the quantizer reads as "use whatever memory the index needs", but the + parser used a `0` return to signal failure, so the two were indistinguishable. A bad + `max_memory` is now reported like every other invalid option. +- **The `INT8` and `UINT8` cosine kernels accumulated in 32 bits** in the scalar and SSE2 + backends, wrapping around past roughly 133k and 66k dimensions; NEON, AVX2 and AVX-512 + already used 64-bit accumulators. SSE2 now also computes `sqrt(a)*sqrt(b)` like every + other backend instead of `sqrtf(a*b)`, which could overflow in `float`, so all five + compute the same cosine. + +### Security + +- **Out-of-bounds write from a large `k`.** The top-k arrays were allocated with + `sqlite3_malloc()`, whose parameter is an `int`, from a `size_t` byte count. `k` comes + straight from the query, so `k=536870913` asked for `0x100000008` bytes, got 8, and the + `memset` that follows overran them. Both allocations now go through + `sqlite3_malloc64()`, so an absurd `k` is a clean `SQLITE_NOMEM`. Manifests as a crash: + the length is not controllable and the value written is zero. +- **Heap buffer overflow in JSON vector parsing.** `vector_from_json()` sized its buffer + and bounded its writes from two different numbers — the buffer from + `sqlite3_malloc((int)alloc)`, which truncated a `size_t`, and the bound from the + untruncated `alloc`. A JSON array with 2^30 commas got an 8-byte buffer and a bound of + 1073741825 elements, and the parse loop wrote `float`s of the caller's choosing straight + past it; the comma count sizes the buffer to the byte, which makes this a precise write + primitive rather than a crash. Reachable through any `vector_as_*()` and through the TEXT + query-vector argument of both scans. It took a TEXT value of about a gigabyte, so a host + built with `SQLITE_MAX_LENGTH >= 1073741824` — the limit that applies is the host's, + since this ships as a loadable extension. There is now a single capacity in 64 bits: the + allocation uses `sqlite3_malloc64()` and the loop checks against that same number. +- **Null write from an unbounded `dimension`.** The declared dimension was only checked + for being positive, so it reached `INT_MAX` and overflowed the size arithmetic + downstream. `(dim/2)*12` in the TurboQuant rotation plan wraps negative past 357913941, + which skipped the allocation of the plan's coordinate arrays and still reported the plan + as valid; `vector_init` with `dimension=400000000` followed by a TurboQuant + `vector_quantize` then crashed the process on an empty table, since the plan is built + before any row is read. Dimensions are now bounded at 2^24 where they are parsed, which + is upstream of every one of those computations — and of the signed-overflow `dim + 7` + expressions in the packing helpers. A dimension above `INT_MAX` is also rejected rather + than wrapped: `4294967300` used to come through as the dimension 4. ## [1.1.0] - 2026-08-24 diff --git a/src/sqlite-vector.h b/src/sqlite-vector.h index 2a6777f..028f815 100644 --- a/src/sqlite-vector.h +++ b/src/sqlite-vector.h @@ -24,7 +24,7 @@ extern "C" { #endif -#define SQLITE_VECTOR_VERSION "1.1.2" +#define SQLITE_VECTOR_VERSION "1.2.0" SQLITE_VECTOR_API int sqlite3_vector_init (sqlite3 *db, char **pzErrMsg, const sqlite3_api_routines *pApi);