Open
Conversation
Comprehensive audit identified and fixed 35 bugs (1 CRITICAL, 7 HIGH, 18 MEDIUM, 9 LOW) across the entire codebase. All 84 SQLite tests and 26 PostgreSQL tests pass with 0 failures and 0 memory leaks. ## src/cloudsync.c (13 fixes) - [HIGH] Guard NULL db_version_stmt in cloudsync_dbversion_rerun — set db_version = CLOUDSYNC_MIN_DB_VERSION and return 0 when stmt is NULL, preventing NULL dereference after schema rebuild failure - [MEDIUM] Add early return for NULL stmt in dbvm_execute to prevent crash when called with uninitialized statement pointer - [MEDIUM] Change (bool)dbvm_count() to (dbvm_count() > 0) in table_pk_exists — prevents negative return values being cast to true, giving false positive "pk exists" results - [MEDIUM] Add NULL check on database_column_text result in cloudsync_refill_metatable before calling strlen — prevents crash on corrupted or empty column data - [MEDIUM] Route early returns in cloudsync_payload_apply through goto cleanup so CLEANUP callback and vm finalize always run — prevents resource leaks and callback contract violation - [MEDIUM] Change return false to goto abort_add_table when ROWIDONLY rejected — ensures table_free runs on the partially allocated table, preventing memory leak - [MEDIUM] Initialize *persistent = false at top of cloudsync_colvalue_stmt — prevents use of uninitialized value when table_lookup returns NULL - [LOW] Add NULL check on database_column_blob in merge_did_cid_win — prevents memcmp with NULL pointer on corrupted cloudsync table - [LOW] Handle partial failure in table_add_to_context_cb — clean up col_name, col_merge_stmt, col_value_stmt at index on error instead of leaving dangling pointers - [LOW] Remove unused pragma_checked field from cloudsync_context - [LOW] Change pointer comparison to strcmp in cloudsync_set_schema — pointer equality missed cases where different string pointers had identical content - [LOW] Fix cloudsync_payload_get NULL check: blob == NULL (always false for char** arg) changed to *blob == NULL - [LOW] Pass extra meta_ref args to SQL_CLOUDSYNC_UPSERT_COL_INIT_OR_BUMP_VERSION mprintf call to match updated PostgreSQL format string ## src/sqlite/cloudsync_sqlite.c (5 fixes) - [HIGH] Split DEFAULT_FLAGS into FLAGS_PURE (SQLITE_UTF8 | SQLITE_INNOCUOUS | SQLITE_DETERMINISTIC) and FLAGS_VOLATILE (SQLITE_UTF8). Pure functions: cloudsync_version, cloudsync_pk_encode, cloudsync_pk_decode. All others volatile — fixes cloudsync_uuid() returning identical values within the same query when SQLite cached deterministic results - [HIGH] Fix realloc inconsistency in dbsync_update_payload_append: on second realloc failure, state was inconsistent (new_values resized, old_values not, capacity not updated). Both reallocs now checked before updating pointers and capacity - [MEDIUM] Move payload->count++ after all database_value_dup NULL checks in dbsync_update_payload_append — prevents count increment when allocation failed, which would cause use-after-free on cleanup - [MEDIUM] Add dbsync_update_payload_free(payload) before 3 early returns in dbsync_update_final — prevents memory leak of entire aggregate payload on error paths - [MEDIUM] Clean up partial database_value_dup allocations on OOM in dbsync_update_payload_append — free dup'd values at current index when count is not incremented to prevent leak ## src/sqlite/database_sqlite.c (6 fixes) - [MEDIUM] Replace fixed 4096-byte trigger WHEN clause buffers with dynamic cloudsync_memory_mprintf — prevents silent truncation for tables with long filter expressions - [MEDIUM] Check cloudsync_memory_mprintf return for NULL before storing in col_names[] in database_build_trigger_when — prevents strlen(NULL) crash in filter_is_column under OOM - [LOW] Use consistent PRId64 format with (int64_t) cast for schema hash in database_check_schema_hash and database_update_schema_hash — prevents format string mismatch on platforms where uint64_t and int64_t have different printf specifiers - [LOW] Fix DEBUG_DBFUNCTION using undeclared variable 'table' instead of 'table_name' in database_create_metatable (line 568) and database_create_triggers (line 782) — compile error when debug macros enabled - [LOW] Remove dead else branch in database_pk_rowid — unreachable code after sqlite3_prepare_v2 success check ## src/sqlite/sql_sqlite.c (1 fix) - [MEDIUM] Change %s to %q in SQL_INSERT_SETTINGS_STR_FORMAT — prevents SQL injection via malformed setting key/value strings ## src/dbutils.c (1 fix) - [MEDIUM] Change snprintf to cloudsync_memory_mprintf for settings insert using the new %q format — ensures proper SQL escaping ## src/utils.c (1 fix) - [MEDIUM] Fix integer overflow in cloudsync_blob_compare: (int)(size1 - size2) overflows for large size_t values, changed to (size1 > size2) ? 1 : -1 ## src/network.c (1 fix) - [MEDIUM] Remove trailing semicolon from savepoint name "cloudsync_logout_savepoint;" — semicolon in name caused savepoint/release mismatch ## src/postgresql/sql_postgresql.c (1 fix) - [CRITICAL] Replace EXCLUDED.col_version with %s.col_version (table reference) in SQL_CLOUDSYNC_UPSERT_COL_INIT_OR_BUMP_VERSION — PostgreSQL EXCLUDED refers to the proposed INSERT row (always col_version=1), not the existing row. This caused col_version to never increment correctly on conflict, breaking CRDT merge logic ## src/postgresql/cloudsync_postgresql.c (4 fixes) - [HIGH] Change PG_RETURN_INT32(rc) to PG_RETURN_BOOL(rc == DBRES_OK) in pg_cloudsync_terminate — SQL declaration returns BOOLEAN but code returned raw integer, causing protocol mismatch - [HIGH] Copy blob data with palloc+memcpy before databasevm_reset in cloudsync_col_value, and fix PG_RETURN_CSTRING to PG_RETURN_BYTEA_P — reset invalidates SPI tuple memory, causing use-after-free; wrong return type caused type mismatch with SQL declaration - [MEDIUM] Use palloc0 instead of cloudsync_memory_alloc+memset in aggregate context — palloc0 is lifetime-safe in PG aggregate memory context; cloudsync_memory_alloc uses wrong allocator - [MEDIUM] Free SPI_tuptable in all paths of get_column_oid — prevents SPI tuple table leak on early return ## src/postgresql/database_postgresql.c (3 fixes) - [MEDIUM] Use sql_escape_identifier for table_name/schema in CREATE INDEX — prevents SQL injection via specially crafted table names - [MEDIUM] Use sql_escape_literal for table_name in trigger WHEN clause — prevents SQL injection in trigger condition - [LOW] Use SPI_getvalue instead of DatumGetName for type safety in database_pk_names — DatumGetName assumes Name type which may not match the actual column type from information_schema ## test/unit.c (3 new tests) - do_test_blob_compare_large_sizes: verifies overflow fix for large size_t values in cloudsync_blob_compare - do_test_deterministic_flags: verifies cloudsync_uuid() returns different values in same query (non-deterministic flag working) - do_test_schema_hash_consistency: verifies int64 hash format roundtrip through cloudsync_schema_versions table Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <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
Critical & High severity fixes
sql_postgresql.cEXCLUDED.col_version→ table reference in UPSERT — broke CRDT merge logic in PostgreSQLcloudsync.cdb_version_stmtincloudsync_dbversion_reruncloudsync_sqlite.cDEFAULT_FLAGSinto pure/volatile —cloudsync_uuid()returned identical values in same querycloudsync_sqlite.ccloudsync_postgresql.cPG_RETURN_INT32(rc)→PG_RETURN_BOOL(rc == DBRES_OK)type mismatchcloudsync_postgresql.cdatabasevm_reset+ fixPG_RETURN_CSTRING→PG_RETURN_BYTEA_P(use-after-free)Medium severity fixes
dbvm_execute,(bool)dbvm_count→> 0, NULL column text check in refill_metatable, early returns routed throughgoto cleanupin payload_apply,return false→goto abort_add_table, uninitialized*persistentincloudsync_colvalue_stmt, wrong NULL check inpayload_getcount++after NULL checks, addpayload_freebefore early returns, clean up partialdatabase_value_dupon OOMcol_names[]%s→%qfor SQL escaping in settings insertblob_comparesize subtractionsql_escape_identifierfor CREATE INDEX,sql_escape_literalin trigger WHENpalloc0in aggregate context,SPI_freetuptablein all pathsLow severity fixes
database_column_blobinmerge_did_cid_wintable_add_to_context_cbpragma_checkedfieldstrcmpinstead of pointer comparison incloudsync_set_schemaPRId64format for schema hashSPI_getvalueinstead ofDatumGetNamefor type safetydatabase_pk_rowidDEBUG_DBFUNCTIONwrong variable name (2 instances)New tests
do_test_blob_compare_large_sizes— verifies overflow fixdo_test_deterministic_flags— verifies UUID non-determinismdo_test_schema_hash_consistency— verifies int64 hash roundtripTest plan
make clean && make unittest— 84/84 SQLite tests pass, 0 memory leaksmake postgres-docker-run-test— 26/26 PostgreSQL tests pass, 0 failures🤖 Generated with Claude Code