From 90945b8817c58647f80035c4f8fa7546c0e710af Mon Sep 17 00:00:00 2001 From: Levi Neely <141506390+lneely@users.noreply.github.com> Date: Mon, 9 Mar 2026 20:49:09 +0100 Subject: [PATCH] Fix debug build: compile error, false-positive abort, crash DB lock (#372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix debug build: compile error, false-positive abort, and crash DB lock - psql_debug.c: add forward declaration for psql_do_prepare to fix conflicting-types compile error (BUILD=debug was broken entirely) - pfs_debug.c: change pfs_debug_check_lock_order from abort to log-only; write paths legitimately take file lock before SQL and handle ordering via psql_trylock()+relock in pfs_reopen_file_for_writing — no actual deadlock risk, the check was a false positive - psignal.c/h: add psignal_register_cleanup() hook mechanism; change panic() to use _exit(1) instead of abort() so all file descriptors are closed on crash, releasing SQLite WAL POSIX advisory locks immediately and preventing ASan from hanging the process as a zombie - psql.c: register psql_panic_cleanup() hook to close the DB on panic (belt-and-suspenders alongside _exit fd cleanup) Co-Authored-By: Claude Sonnet 4.6 * Fix debug psql_trylock: missing strong override left lockctr unupdated The weak psql_trylock() stub in psql.c called plocks_trywrlock() directly without updating lockctr. In the debug build, psql_unlock() asserts lockctr > 0, so when trylock succeeded (lock acquired, lockctr still 0) the assert fired with SIGABRT on write ops via pfs_inc_writeid_locked. Add a strong psql_trylock() override in psql_debug.c that delegates to psql_do_trylock(), which properly acquires the rwlock and updates lockctr. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Levi Neely Co-authored-by: Claude Sonnet 4.6 --- pclsync/debug/pfs_debug.c | 11 +++++++---- pclsync/debug/psql_debug.c | 8 ++++++++ pclsync/psignal.c | 21 +++++++++++++++++++-- pclsync/psignal.h | 1 + pclsync/psql.c | 8 ++++++++ 5 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pclsync/debug/pfs_debug.c b/pclsync/debug/pfs_debug.c index 868b598..a05fbf5 100644 --- a/pclsync/debug/pfs_debug.c +++ b/pclsync/debug/pfs_debug.c @@ -47,8 +47,11 @@ void pfs_debug_register_signal_handlers() { } void pfs_debug_check_lock_order(const char *file, unsigned long line) { - if (!psql_locked()) { - pdbg_logf(D_ERROR, "lock ordering violation: pfs_lock_file called without psql_lock at %s:%lu", file, line); - abort(); - } + /* Lock order is sql -> file, but write paths legitimately take file lock + * first and handle sql acquisition via psql_trylock()+relock in + * pfs_reopen_file_for_writing. No actual deadlock risk; just log. */ + if (!psql_locked()) + pdbg_logf(D_NOTICE, + "pfs_lock_file called without psql_lock at %s:%lu (expected for write path)", + file, line); } diff --git a/pclsync/debug/psql_debug.c b/pclsync/debug/psql_debug.c index a18ba85..2abfd35 100644 --- a/pclsync/debug/psql_debug.c +++ b/pclsync/debug/psql_debug.c @@ -39,6 +39,10 @@ extern PSYNC_THREAD const char *psync_thread_name; +/* Forward declaration */ +psync_sql_res *psql_do_prepare(const char *sql, const char *file, + unsigned line); + // -------------------------------------------------------------------------- // Debug-only state // -------------------------------------------------------------------------- @@ -223,6 +227,10 @@ void psql_do_rdlock(const char *file, unsigned line) { // Strong overrides for same-named functions (weak in psql.c) // -------------------------------------------------------------------------- +int psql_trylock() { + return psql_do_trylock(__FILE__, __LINE__); +} + void psql_lock() { psql_do_lock(__FILE__, __LINE__); } diff --git a/pclsync/psignal.c b/pclsync/psignal.c index 4ce71bf..b23795d 100644 --- a/pclsync/psignal.c +++ b/pclsync/psignal.c @@ -4,6 +4,16 @@ #include #include #include +#include + +#define PSIGNAL_MAX_CLEANUPS 16 +static void (*cleanup_fns[PSIGNAL_MAX_CLEANUPS])(void); +static int cleanup_count = 0; + +void psignal_register_cleanup(void (*fn)(void)) { + if (cleanup_count < PSIGNAL_MAX_CLEANUPS) + cleanup_fns[cleanup_count++] = fn; +} static volatile sig_atomic_t sigint_flag = 0; static volatile sig_atomic_t sigterm_flag = 0; @@ -42,8 +52,15 @@ void panic(const char *msg) { signal(SIGSEGV, SIG_DFL); signal(SIGABRT, SIG_DFL); signal(SIGBUS, SIG_DFL); - - abort(); + + for (int i = 0; i < cleanup_count; i++) + cleanup_fns[i](); + + /* Use _exit() instead of abort(): _exit closes all file descriptors + * immediately, releasing POSIX advisory locks (including SQLite's WAL + * locks). abort() is intercepted by ASan which can hang indefinitely, + * leaving the process as a zombie and the DB locked. */ + _exit(1); } static void panic_handler(int sig) { diff --git a/pclsync/psignal.h b/pclsync/psignal.h index cc50c23..2bbb681 100644 --- a/pclsync/psignal.h +++ b/pclsync/psignal.h @@ -10,6 +10,7 @@ extern "C" { void psignal_register(int signum); int psignal_check_pending(void); void psignal_set_custom_handler(int sig, void (*handler)(int)); +void psignal_register_cleanup(void (*fn)(void)); void panic(const char *msg) __attribute__((noreturn)); #ifdef __cplusplus diff --git a/pclsync/psql.c b/pclsync/psql.c index e7ea9a0..3966420 100644 --- a/pclsync/psql.c +++ b/pclsync/psql.c @@ -12,6 +12,7 @@ #include "psettings.h" #include "psql.h" #include "psql_internal.h" +#include "psignal.h" #include "psys.h" // psql.h defines function-like macros (e.g. psql_lock() -> psql_do_lock()) @@ -82,6 +83,12 @@ static void on_error(void *ptr, int code, const char *msg) { pdbg_logf(D_WARNING, "database warning %d: %s", code, msg); } +static void psql_panic_cleanup(void) { + if (psync_db) + sqlite3_close_v2(psync_db); + psync_db = NULL; +} + static void psync_sql_free_cache(void *ptr) { psync_sql_res *res = (psync_sql_res *)ptr; sqlite3_finalize(res->stmt); @@ -199,6 +206,7 @@ int psql_connect(const char *db) { pthread_mutex_init(&cpmutex, &mattr); pthread_mutexattr_destroy(&mattr); initmutex = 0; + psignal_register_cleanup(psql_panic_cleanup); } if (IS_DEBUG) { sqlite3_config(SQLITE_CONFIG_LOG, on_error, NULL);