From fcb0a5c3b0814b695bb5279bd303dee4db824b99 Mon Sep 17 00:00:00 2001 From: Knutwurst <36196269+knutwurst@users.noreply.github.com> Date: Wed, 24 Jun 2026 20:35:31 +0200 Subject: [PATCH] Concurrency: atomic g_stage/g_err, snapshot pkg_diag, safe open, SQLite mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit g_stage and g_err are now _Atomic. The backend init thread publishes stage transitions and function-pointer assignments; HTTP request handlers read g_stage to decide whether to call the Sony API. With the old `volatile int` reads, nothing in the C memory model ordered the function-pointer loads against the stage check — a stage==5 sighting could (in theory) come before the pointer stores were visible. Default seq_cst on _Atomic gives us the acquire/release pairing for free. /api/pkgdiag returned g_pkg_diag_json directly with RESPMEM_PERSISTENT, so MHD's writer thread could read the buffer while record_pkg_diag was mid-snprintf — torn JSON or a missing NUL terminator. Snapshot under g_mutex into a heap copy and queue with RESPMEM_MUST_FREE instead. patchdl_net.c gets fopen_safe(): open() with O_NOFOLLOW|O_CLOEXEC and mode 0600, then fdopen. The old fopen("wb") follows symlinks (a malicious symlink at dest_path could redirect the write) and creates mode 0666 (libc default). Both download paths now use it. patchdl_appdb opens SQLite with SQLITE_OPEN_FULLMUTEX. Today the scan runs on the startup thread only, but a future rescan triggered from the HTTP thread would otherwise race the handle. --- src/patchdl_appdb.c | 6 +++++- src/patchdl_install.c | 10 +++++++--- src/patchdl_net.c | 23 ++++++++++++++++++++--- src/patchdl_websrv.c | 13 +++++++++++-- 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/patchdl_appdb.c b/src/patchdl_appdb.c index fd244ec..7320cee 100644 --- a/src/patchdl_appdb.c +++ b/src/patchdl_appdb.c @@ -61,8 +61,12 @@ patchdl_appdb_load(patchdl_appinfo_t **out, size_t *count) { *out = NULL; *count = 0; + /* FULLMUTEX: today only the startup thread calls this, but future code + paths (a manual rescan triggered from the HTTP thread) would otherwise + race the SQLite handle. Cost is one mutex per call. */ if (sqlite3_open_v2(APP_DB_URI, &db, - SQLITE_OPEN_READONLY | SQLITE_OPEN_URI, NULL) != SQLITE_OK) { + SQLITE_OPEN_READONLY | SQLITE_OPEN_URI | + SQLITE_OPEN_FULLMUTEX, NULL) != SQLITE_OK) { if (db) sqlite3_close(db); return -1; } diff --git a/src/patchdl_install.c b/src/patchdl_install.c index 41f27de..872e86d 100644 --- a/src/patchdl_install.c +++ b/src/patchdl_install.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -112,9 +113,12 @@ static ai_get_status_fn ai_get_status; symbols via the kernel dynlib helpers. Runs in a detached thread; the HTTP handler reports the stage and never blocks. stage: 0 idle, 1 resolve loader, 2 load modules, 3 resolve symbols, - 4 initialize, 5 ready, negative = failure at that step. */ -static volatile int g_stage; -static int g_err; + 4 initialize, 5 ready, negative = failure at that step. + Stored as _Atomic so the worker's release-store and the request handlers' + acquire-loads pair properly — the function pointers they read after + stage==5 must not be reordered ahead of the stage check. */ +static _Atomic int g_stage; +static _Atomic int g_err; static pthread_mutex_t g_mtx = PTHREAD_MUTEX_INITIALIZER; static char g_probe_json[2048]; /* filled by the backend thread */ static char g_last_content_id[AI_CONTENTID_SIZE]; diff --git a/src/patchdl_net.c b/src/patchdl_net.c index 9009132..2bd2e07 100644 --- a/src/patchdl_net.c +++ b/src/patchdl_net.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -9,6 +10,7 @@ #include #include #include +#include #include #include @@ -33,6 +35,21 @@ #define PATCHDL_BUF_MAX_VERXML (16 * 1024 * 1024) #define PATCHDL_BUF_MAX_MANIFEST (64 * 1024 * 1024) +#ifdef PATCHDL_HAVE_CURL +/* Replacement for fopen("wb"/"r+b") that refuses to follow a symlink at the + destination (would let a malicious symlink redirect the download) and pins + the new file's mode to 0600. Returns NULL on any open error. */ +static FILE * +fopen_safe(const char *path, int rw_existing) { + int flags = O_CLOEXEC | O_NOFOLLOW; + int fd; + flags |= rw_existing ? O_RDWR : (O_WRONLY | O_CREAT | O_TRUNC); + fd = open(path, flags, 0600); + if (fd < 0) return NULL; + return fdopen(fd, rw_existing ? "r+b" : "wb"); +} +#endif + #ifdef PATCHDL_HAVE_CURL static const char *ALLOWED_HOSTS[] = { @@ -486,7 +503,7 @@ int patchdl_http_download_progress(const char *url, const char *dest_path, long long *bytes_out, patchdl_download_progress_cb cb, void *ctx) { - FILE *fp = fopen(dest_path, "wb"); + FILE *fp = fopen_safe(dest_path, 0); progress_state_t progress = { cb, ctx, 0, 0 }; int rc; @@ -628,10 +645,10 @@ patchdl_http_download_manifest_progress(const char *manifest_url, written continues mid-piece via an HTTP byte range (with a fall back to re-fetching it whole if the CDN ignores the range). */ if (resume) { - fp = fopen(dest_path, "r+b"); + fp = fopen_safe(dest_path, 1); if (fp) { fseek(fp, 0, SEEK_END); have = ftell(fp); if (have < 0) have = 0; } } - if (!fp) { fp = fopen(dest_path, "wb"); have = 0; } + if (!fp) { fp = fopen_safe(dest_path, 0); have = 0; } if (!fp) { free(manifest.data); return -1; } started = (have <= 0); diff --git a/src/patchdl_websrv.c b/src/patchdl_websrv.c index b36662c..f2528f7 100644 --- a/src/patchdl_websrv.c +++ b/src/patchdl_websrv.c @@ -2013,8 +2013,17 @@ on_request(void *cls, struct MHD_Connection *conn, const char *url, return queue_json_owned(conn, MHD_HTTP_OK, strdup(p)); } - if (!strcmp(url, "/api/pkgdiag")) - return queue_json(conn, MHD_HTTP_OK, g_pkg_diag_json); + if (!strcmp(url, "/api/pkgdiag")) { + /* Snapshot under the lock — otherwise MHD would read g_pkg_diag_json + in-place (RESPMEM_PERSISTENT) while record_pkg_diag is mid-snprintf, + producing a torn read or a missing NUL terminator. */ + char snap[sizeof(g_pkg_diag_json)]; + pthread_mutex_lock(&g_mutex); + memcpy(snap, g_pkg_diag_json, sizeof(snap)); + pthread_mutex_unlock(&g_mutex); + snap[sizeof(snap) - 1] = '\0'; + return queue_json_owned(conn, MHD_HTTP_OK, strdup(snap)); + } /* Read-only diagnostic: re-fetch the patch manifest for a title (PatchDL can bypass the DNS block) and dump each piece's offset/size/SHA-256 so the