diff --git a/CHANGELOG.md b/CHANGELOG.md index eb19b52..efa1bc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ 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/). +## [1.0.14] - 2026-04-15 + +### Fixed + +- **Stale `cloudsync_table_settings` crash**: Reopening a database that had its base table and `_cloudsync` meta-table dropped without calling `cloudsync_cleanup` crashed with a double-free on `sqlite3_close`. Two bugs were involved: (1) `cloudsync_dbversion_rebuild` returned `DBRES_NOMEM` when `cloudsync_dbversion_build_query` yielded a NULL SQL string (stale row in `cloudsync_table_settings` but no matching `*_cloudsync` table in `sqlite_master`), failing extension init; (2) on init failure `dbsync_register_functions` manually freed the context that SQLite already owned via the `cloudsync_version` destructor, causing a double-free when the connection was later closed. `cloudsync_dbversion_rebuild` now treats a NULL build query the same as `count == 0` (no prepared statement, db_version stays at the minimum and is rebuilt on the next `cloudsync_init`), and the manual free in the error path has been removed. + +### Added + +- Unit test `do_test_stale_table_settings_dropped_meta` (Stale Table Settings Dropped Meta) covering the drop-base-table + drop-meta-table + reopen scenario. + ## [1.0.13] - 2026-04-14 ### Fixed diff --git a/src/cloudsync.c b/src/cloudsync.c index cb23368..d23c29d 100644 --- a/src/cloudsync.c +++ b/src/cloudsync.c @@ -363,11 +363,11 @@ void dbvm_reset (dbvm_t *stmt) { // MARK: - Database Version - -char *cloudsync_dbversion_build_query (cloudsync_context *data) { +int cloudsync_dbversion_build_query (cloudsync_context *data, char **sql_out) { // this function must be manually called each time tables changes // because the query plan changes too and it must be re-prepared // unfortunately there is no other way - + // we need to execute a query like: /* SELECT max(version) as version FROM ( @@ -380,12 +380,11 @@ char *cloudsync_dbversion_build_query (cloudsync_context *data) { SELECT value as version FROM cloudsync_settings WHERE key = 'pre_alter_dbversion' ) */ - + // the good news is that the query can be computed in SQLite without the need to do any extra computation from the host language - - char *value = NULL; - int rc = database_select_text(data, SQL_DBVERSION_BUILD_QUERY, &value); - return (rc == DBRES_OK) ? value : NULL; + + *sql_out = NULL; + return database_select_text(data, SQL_DBVERSION_BUILD_QUERY, sql_out); } int cloudsync_dbversion_rebuild (cloudsync_context *data) { @@ -393,16 +392,26 @@ int cloudsync_dbversion_rebuild (cloudsync_context *data) { databasevm_finalize(data->db_version_stmt); data->db_version_stmt = NULL; } - + int64_t count = dbutils_table_settings_count_tables(data); if (count == 0) return DBRES_OK; else if (count == -1) return cloudsync_set_dberror(data); - - char *sql = cloudsync_dbversion_build_query(data); - if (!sql) return DBRES_NOMEM; + + char *sql = NULL; + int rc = cloudsync_dbversion_build_query(data, &sql); + if (rc != DBRES_OK) return cloudsync_set_dberror(data); + + // A NULL SQL with rc == OK means the generator produced a NULL row: + // sqlite_master has no *_cloudsync meta-tables (for example, the user + // dropped the base table and its meta-table without calling + // cloudsync_cleanup, leaving stale cloudsync_table_settings rows). + // Treat this the same as count == 0: no prepared statement, db_version + // stays at the minimum and will be rebuilt on the next cloudsync_init. + // Genuine errors from database_select_text are handled above. + if (!sql) return DBRES_OK; DEBUG_SQL("db_version_stmt: %s", sql); - - int rc = databasevm_prepare(data, sql, (void **)&data->db_version_stmt, DBFLAG_PERSISTENT); + + rc = databasevm_prepare(data, sql, (void **)&data->db_version_stmt, DBFLAG_PERSISTENT); DEBUG_STMT("db_version_stmt %p", data->db_version_stmt); cloudsync_memory_free(sql); return rc; diff --git a/src/cloudsync.h b/src/cloudsync.h index 4be7a9f..d4f935e 100644 --- a/src/cloudsync.h +++ b/src/cloudsync.h @@ -18,7 +18,7 @@ extern "C" { #endif -#define CLOUDSYNC_VERSION "1.0.13" +#define CLOUDSYNC_VERSION "1.0.14" #define CLOUDSYNC_MAX_TABLENAME_LEN 512 #define CLOUDSYNC_VALUE_NOTSET -1 diff --git a/src/sqlite/cloudsync_sqlite.c b/src/sqlite/cloudsync_sqlite.c index 619650c..86c1e83 100644 --- a/src/sqlite/cloudsync_sqlite.c +++ b/src/sqlite/cloudsync_sqlite.c @@ -1470,7 +1470,10 @@ int dbsync_register_functions (sqlite3 *db, char **pzErrMsg) { // load config, if exists if (cloudsync_config_exists(data)) { if (cloudsync_context_init(data) == NULL) { - cloudsync_context_free(data); + // Do not free ctx here: it is already owned by the cloudsync_version + // function (registered above with cloudsync_context_free as its + // destructor). SQLite will release it when the connection is closed. + // Freeing it manually would cause a double-free on sqlite3_close. if (pzErrMsg) *pzErrMsg = sqlite3_mprintf("An error occurred while trying to initialize context"); return SQLITE_ERROR; } diff --git a/test/postgresql/51_stale_table_settings_dropped_meta.sql b/test/postgresql/51_stale_table_settings_dropped_meta.sql new file mode 100644 index 0000000..6d4880a --- /dev/null +++ b/test/postgresql/51_stale_table_settings_dropped_meta.sql @@ -0,0 +1,123 @@ +-- 'Stale cloudsync_table_settings with dropped meta-table' +-- Mirrors the SQLite unit test: Stale Table Settings Dropped Meta +-- +-- When a user drops a tracked table and its
_cloudsync meta-table +-- manually (without calling cloudsync_cleanup), cloudsync_table_settings is +-- left with stale rows while pg_tables no longer has any matching +-- *_cloudsync table. Before the fix in cloudsync_dbversion_rebuild, opening a +-- new backend and calling any cloudsync function caused +-- cloudsync_dbversion_build_query to produce a NULL SQL string (string_agg +-- over zero rows), which was misreported as DBRES_NOMEM, making +-- cloudsync_context_init fail and every cloudsync_* call ereport ERROR. + +\set testid '51' +\ir helper_test_init.sql + +\connect postgres +\ir helper_psql_conn_setup.sql + +DROP DATABASE IF EXISTS cloudsync_test_51; +CREATE DATABASE cloudsync_test_51; + +\connect cloudsync_test_51 +\ir helper_psql_conn_setup.sql + +CREATE EXTENSION IF NOT EXISTS cloudsync; + +-- Phase 1: create a tracked table and initialize cloudsync on it. +DROP TABLE IF EXISTS stale_doc CASCADE; +CREATE TABLE stale_doc (id TEXT PRIMARY KEY NOT NULL, body TEXT); +SELECT cloudsync_init('stale_doc', 'CLS', 1) AS _init \gset + +-- Sanity: the meta-table exists and cloudsync_table_settings has a row for it. +SELECT count(*) AS meta_exists FROM pg_tables WHERE tablename = 'stale_doc_cloudsync' \gset +SELECT (:meta_exists::int = 1) AS meta_exists_ok \gset +\if :meta_exists_ok +\echo [PASS] (:testid) stale_doc_cloudsync meta-table created +\else +\echo [FAIL] (:testid) expected stale_doc_cloudsync to exist +SELECT (:fail::int + 1) AS fail \gset +\endif + +SELECT count(*) AS settings_rows FROM cloudsync_table_settings WHERE tbl_name = 'stale_doc' \gset +SELECT (:settings_rows::int > 0) AS settings_rows_ok \gset +\if :settings_rows_ok +\echo [PASS] (:testid) cloudsync_table_settings has row(s) for stale_doc +\else +\echo [FAIL] (:testid) expected cloudsync_table_settings row for stale_doc +SELECT (:fail::int + 1) AS fail \gset +\endif + +-- Phase 2: drop BOTH the base table and the meta-table without calling +-- cloudsync_cleanup. cloudsync_table_settings still references stale_doc, +-- but pg_tables has no *_cloudsync tables at all now. +DROP TABLE stale_doc; +DROP TABLE stale_doc_cloudsync; + +SELECT count(*) AS cloudsync_meta_tables FROM pg_tables WHERE tablename LIKE '%_cloudsync' \gset +SELECT (:cloudsync_meta_tables::int = 0) AS no_meta_ok \gset +\if :no_meta_ok +\echo [PASS] (:testid) no *_cloudsync meta-tables remain in pg_tables +\else +\echo [FAIL] (:testid) expected zero *_cloudsync tables, got :cloudsync_meta_tables +SELECT (:fail::int + 1) AS fail \gset +\endif + +-- Phase 3: reconnect to force a fresh backend. pg_cloudsync_context is a +-- static per-process pointer, so a new backend means +-- cloudsync_pg_context_init runs again on the next cloudsync call — which +-- is exactly what used to fail under this bug. +\connect cloudsync_test_51 +\ir helper_psql_conn_setup.sql + +-- cloudsync_version is a pure function that does not touch the context, so +-- this call cannot fail even with the bug present. It's here only as a +-- trivial smoke check that the extension is still loadable. +SELECT cloudsync_version() IS NOT NULL AS version_ok \gset +\if :version_ok +\echo [PASS] (:testid) cloudsync_version() reachable after reopen +\else +\echo [FAIL] (:testid) cloudsync_version() failed after reopen +SELECT (:fail::int + 1) AS fail \gset +\endif + +-- The real check: calling a function that goes through get_cloudsync_context() +-- must succeed. Before the fix, cloudsync_dbversion_rebuild returned +-- DBRES_NOMEM here because SQL_DBVERSION_BUILD_QUERY's string_agg over zero +-- rows produced a NULL SQL string, and the whole init path would ereport +-- ERROR — any cloudsync_* call below would abort the script. +CREATE TABLE stale_doc2 (id TEXT PRIMARY KEY NOT NULL, body TEXT); +SELECT cloudsync_init('stale_doc2', 'CLS', 1) AS _init2 \gset + +-- The new table's meta-table exists. If cloudsync_init failed (pre-fix +-- behavior) this count will be 0, covering both the init-rc check and the +-- meta-table creation in a single assertion. +SELECT count(*) AS meta2_exists FROM pg_tables WHERE tablename = 'stale_doc2_cloudsync' \gset +SELECT (:meta2_exists::int = 1) AS meta2_exists_ok \gset +\if :meta2_exists_ok +\echo [PASS] (:testid) cloudsync_init succeeded and stale_doc2_cloudsync was created +\else +\echo [FAIL] (:testid) expected stale_doc2_cloudsync to exist after cloudsync_init +SELECT (:fail::int + 1) AS fail \gset +\endif + +-- An insert via the new cloudsync_init'd table should produce a cloudsync +-- metadata entry — confirming the context is fully functional. +INSERT INTO stale_doc2 (id, body) VALUES ('a', 'hello'); + +SELECT count(*) AS meta_rows FROM stale_doc2_cloudsync \gset +SELECT (:meta_rows::int > 0) AS meta_rows_ok \gset +\if :meta_rows_ok +\echo [PASS] (:testid) stale_doc2_cloudsync has metadata after insert +\else +\echo [FAIL] (:testid) expected metadata rows in stale_doc2_cloudsync +SELECT (:fail::int + 1) AS fail \gset +\endif + +-- Cleanup +\ir helper_test_cleanup.sql +\if :should_cleanup +DROP DATABASE IF EXISTS cloudsync_test_51; +\else +\echo [INFO] !!!!! +\endif diff --git a/test/postgresql/full_test.sql b/test/postgresql/full_test.sql index 6c1d01f..9ff000a 100644 --- a/test/postgresql/full_test.sql +++ b/test/postgresql/full_test.sql @@ -58,6 +58,7 @@ \ir 48_row_filter_multi_table.sql \ir 49_row_filter_prefill.sql \ir 50_block_lww_existing_data.sql +\ir 51_stale_table_settings_dropped_meta.sql -- 'Test summary' \echo '\nTest summary:' diff --git a/test/unit.c b/test/unit.c index 8164e15..30c4f69 100644 --- a/test/unit.c +++ b/test/unit.c @@ -46,6 +46,7 @@ sqlite3 *do_create_database (void); int cloudsync_table_sanity_check (cloudsync_context *data, const char *name, CLOUDSYNC_INIT_FLAG init_flags); bool database_system_exists (cloudsync_context *data, const char *name, const char *type); +int cloudsync_dbversion_rebuild (cloudsync_context *data); static int stdout_backup = -1; // Backup file descriptor for stdout static int dev_null_fd = -1; // File descriptor for /dev/null @@ -2401,6 +2402,160 @@ bool do_test_stale_table_settings(bool cleanup_databases) { return result; } +// Same as do_test_stale_table_settings, but also drops the
_cloudsync +// meta-table before reopening. With a stale cloudsync_table_settings row and +// no matching *_cloudsync meta-table in sqlite_master, the dbversion query +// builder produces an empty (NULL) SQL string, causing sqlite3_cloudsync_init +// to fail on reopen — previously crashing in some environments. +bool do_test_stale_table_settings_dropped_meta(bool cleanup_databases) { + bool result = false; + char dbpath[256]; + time_t timestamp = time(NULL); + + #ifdef __ANDROID__ + snprintf(dbpath, sizeof(dbpath), "%s/cloudsync-test-stale-meta-%ld.sqlite", ".", timestamp); + #else + snprintf(dbpath, sizeof(dbpath), "%s/cloudsync-test-stale-meta-%ld.sqlite", getenv("HOME"), timestamp); + #endif + + // Phase 1: create database, table, and init cloudsync + sqlite3 *db = NULL; + int rc = sqlite3_open(dbpath, &db); + if (rc != SQLITE_OK) return false; + sqlite3_exec(db, "PRAGMA journal_mode=WAL;", NULL, NULL, NULL); + sqlite3_cloudsync_init(db, NULL, NULL); + + rc = sqlite3_exec(db, "CREATE TABLE cloud (id TEXT PRIMARY KEY NOT NULL, value TEXT, extra INTEGER);", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + rc = sqlite3_exec(db, "SELECT cloudsync_init('cloud');", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + // Phase 2: drop both the base table AND the meta-table without calling + // cloudsync_cleanup. This leaves stale entries in cloudsync_table_settings + // with no matching *_cloudsync table in sqlite_master. + sqlite3_exec(db, "SELECT cloudsync_terminate();", NULL, NULL, NULL); + sqlite3_close(db); + db = NULL; + + rc = sqlite3_open(dbpath, &db); + if (rc != SQLITE_OK) goto cleanup; + rc = sqlite3_exec(db, "DROP TABLE IF EXISTS cloud;", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + rc = sqlite3_exec(db, "DROP TABLE IF EXISTS cloud_cloudsync;", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + sqlite3_close(db); + db = NULL; + + // Phase 3: reopen the database and load the extension — must succeed. + rc = sqlite3_open(dbpath, &db); + if (rc != SQLITE_OK) goto cleanup; + rc = sqlite3_cloudsync_init(db, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + // Sanity check: we can still call cloudsync_version and create a new table. + rc = sqlite3_exec(db, "SELECT cloudsync_version();", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + rc = sqlite3_exec(db, "CREATE TABLE cloud2 (id TEXT PRIMARY KEY NOT NULL, v TEXT);", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + rc = sqlite3_exec(db, "SELECT cloudsync_init('cloud2');", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + result = true; + +cleanup: + if (db) { + sqlite3_exec(db, "SELECT cloudsync_terminate();", NULL, NULL, NULL); + sqlite3_close(db); + } + if (cleanup_databases) { + file_delete_internal(dbpath); + char walpath[280]; + snprintf(walpath, sizeof(walpath), "%s-wal", dbpath); + file_delete_internal(walpath); + snprintf(walpath, sizeof(walpath), "%s-shm", dbpath); + file_delete_internal(walpath); + } + return result; +} + +// Authorizer that denies SELECT reads of sqlite_master. Used to force +// sqlite3_prepare_v2 of SQL_DBVERSION_BUILD_QUERY (which scans sqlite_master) +// to fail with SQLITE_AUTH, exercising the real error path in +// cloudsync_dbversion_rebuild introduced after 1.0.14. +static int deny_sqlite_master_authorizer(void *pUserData, int action, const char *zArg1, + const char *zArg2, const char *zDbName, const char *zTrigger) { + (void)pUserData; (void)zArg2; (void)zDbName; (void)zTrigger; + if (action == SQLITE_READ && zArg1 && strcmp(zArg1, "sqlite_master") == 0) { + return SQLITE_DENY; + } + return SQLITE_OK; +} + +// Verify that cloudsync_dbversion_rebuild surfaces a real failure from +// database_select_text(SQL_DBVERSION_BUILD_QUERY, ...) instead of silently +// treating it as "no *_cloudsync meta-tables present" — which would leave +// db_version_stmt unset and cause writes to fall back to CLOUDSYNC_MIN_DB_VERSION. +bool do_test_dbversion_rebuild_error(void) { + sqlite3 *db = NULL; + cloudsync_context *ctx = NULL; + bool result = false; + + int rc = sqlite3_open(":memory:", &db); + if (rc != SQLITE_OK) return false; + rc = sqlite3_cloudsync_init(db, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + // Create a real cloudsync table so cloudsync_table_settings has a row + // (count_tables > 0 — the early-return-OK path is not taken). + rc = sqlite3_exec(db, "CREATE TABLE t (id TEXT PRIMARY KEY NOT NULL, v TEXT);", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + rc = sqlite3_exec(db, "SELECT cloudsync_init('t');", NULL, NULL, NULL); + if (rc != SQLITE_OK) goto cleanup; + + // Create a secondary context on the same db and initialize it. This + // context is independent from the one registered by sqlite3_cloudsync_init, + // so we can call cloudsync_dbversion_rebuild on it directly without + // disturbing the registered functions. + ctx = cloudsync_context_create(db); + if (!ctx) goto cleanup; + if (cloudsync_context_init(ctx) == NULL) goto cleanup; + + // Install an authorizer that denies reads of sqlite_master. New prepares + // (including the one SQL_DBVERSION_BUILD_QUERY triggers inside + // database_select_text) will fail with SQLITE_AUTH. Already-prepared + // statements are unaffected, so the registered cloudsync_* functions + // still work for cleanup. + sqlite3_set_authorizer(db, deny_sqlite_master_authorizer, NULL); + + // Expect a non-OK result now that the build query cannot be prepared. + // Before the review fix this would incorrectly return DBRES_OK and leave + // db_version_stmt == NULL, silently masking the failure. + int rebuild_rc = cloudsync_dbversion_rebuild(ctx); + + // Remove authorizer before any further work so cleanup can run normally. + sqlite3_set_authorizer(db, NULL, NULL); + + if (rebuild_rc == DBRES_OK) goto cleanup; + + // The error must have been recorded on the context via cloudsync_set_dberror. + if (cloudsync_errcode(ctx) == DBRES_OK) goto cleanup; + const char *msg = cloudsync_errmsg(ctx); + if (!msg || msg[0] == 0) goto cleanup; + + result = true; + +cleanup: + sqlite3_set_authorizer(db, NULL, NULL); + if (ctx) cloudsync_context_free(ctx); + if (db) { + sqlite3_exec(db, "SELECT cloudsync_terminate();", NULL, NULL, NULL); + sqlite3_close(db); + } + return result; +} + // Authorizer state for do_test_context_cb_error_cleanup. // Denies INSERT on a specific table after allowing a set number of INSERTs. static const char *g_deny_insert_table = NULL; @@ -12268,6 +12423,8 @@ int main (int argc, const char * argv[]) { result += test_report("Large Composite PK Test:", do_test_large_composite_pk(2, print_result, cleanup_databases)); result += test_report("Schema Hash Mismatch:", do_test_schema_hash_mismatch(2, print_result, cleanup_databases)); result += test_report("Stale Table Settings:", do_test_stale_table_settings(cleanup_databases)); + result += test_report("Stale Table Settings Dropped Meta:", do_test_stale_table_settings_dropped_meta(cleanup_databases)); + result += test_report("DBVersion Rebuild Error:", do_test_dbversion_rebuild_error()); result += test_report("Block LWW Existing Data:", do_test_block_lww_existing_data(cleanup_databases)); result += test_report("Block Column Reload:", do_test_block_column_reload(cleanup_databases)); result += test_report("CB Error Cleanup:", do_test_context_cb_error_cleanup());