fix: stale cloudsync_table_settings crash on reopen#40
Merged
Conversation
Dropping a table and its <table>_cloudsync meta-table without calling cloudsync_cleanup left stale rows in cloudsync_table_settings. On the next connection, loading the extension crashed with a double-free. Two bugs: - cloudsync_dbversion_rebuild returned DBRES_NOMEM when the build query yielded NULL (no *_cloudsync tables in sqlite_master), failing init. Treat NULL the same as count == 0: no prepared stmt, rebuilt later. - On init failure, dbsync_register_functions manually freed the context that SQLite already owns via the cloudsync_version destructor, causing a double-free on sqlite3_close. Let SQLite release it instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rsion_rebuild Previously cloudsync_dbversion_build_query collapsed all failures into a NULL return, and the stale-settings fix then treated every NULL as "no meta-tables" and returned DBRES_OK. A genuine failure from database_select_text (SQLITE_NOMEM materializing the generated SQL, a prepare/step error, etc.) would leave data->db_version_stmt unset and silently succeed. Subsequent writes would fall back to CLOUDSYNC_MIN_DB_VERSION via cloudsync_dbversion_next, potentially emitting duplicate or rewound db_version values on a database that still has synced tables. Change cloudsync_dbversion_build_query to return rc with the SQL via an out-param. cloudsync_dbversion_rebuild now surfaces non-OK results through cloudsync_set_dberror, and only the rc == OK && sql == NULL case (generator produced a NULL row because sqlite_master has no *_cloudsync tables) is treated like count == 0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Install an sqlite3 authorizer that denies reads of sqlite_master, so sqlite3_prepare_v2 of SQL_DBVERSION_BUILD_QUERY fails with SQLITE_AUTH. On a context with a non-empty cloudsync_table_settings, this must now return a non-OK rc and set cloudsync_errcode/errmsg — before the review fix it was silently collapsed into DBRES_OK, leaving db_version_stmt unset. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eopen Mirrors the SQLite unit test do_test_stale_table_settings_dropped_meta. Drops the base table and its <table>_cloudsync meta-table without calling cloudsync_cleanup, reconnects to force a fresh backend, and verifies that cloudsync_init on a new table still succeeds. Before the shared fix in cloudsync_dbversion_rebuild this aborted with "An error occurred while trying to initialize context" because SQL_DBVERSION_BUILD_QUERY's string_agg over zero rows produced a NULL SQL string that was misreported as DBRES_NOMEM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<table>_cloudsyncmeta-table were dropped without callingcloudsync_cleanup.cloudsync_dbversion_rebuild, so init errors are surfaced instead of silently leavingdb_version_stmtunset.Details
Two independent bugs were involved:
src/cloudsync.c—cloudsync_dbversion_rebuildreturnedDBRES_NOMEMwhencloudsync_dbversion_build_queryyielded a NULL SQL string. That NULL happens benignly whencloudsync_table_settingsstill has a row butsqlite_masterno longer has any matching*_cloudsynctable (e.g. after a manualDROP TABLE). The build helper now returnsrcvia an out-param so the caller can distinguish "no meta-tables" (rc == OK && sql == NULL, treated likecount == 0) from a realdatabase_select_textfailure (propagated viacloudsync_set_dberror).src/sqlite/cloudsync_sqlite.c— on the init-failure branch,dbsync_register_functionsmanually calledcloudsync_context_free(data), but the pointer is already owned by SQLite via thecloudsync_versionfunction's destructor (registered earlier in the same function).sqlite3_closethen freed it a second time →pointer being freed was not allocatedSIGABRT. Removed the manual free; SQLite cleans up on close.Test plan
do_test_stale_table_settings_dropped_meta— reproduces the original crash (drop base table + meta-table, reopen, load extension, create new tracked table).do_test_dbversion_rebuild_error— installs ansqlite3_set_authorizerdenying reads ofsqlite_mastersoSQL_DBVERSION_BUILD_QUERYprepare fails withSQLITE_AUTH; assertscloudsync_dbversion_rebuildreturns non-OK and records an error on the context.make unittestsuite passes, including the memory-leak check.