From 2461e01c6e97468df1afebfa4ba568e19480a712 Mon Sep 17 00:00:00 2001 From: datagutt Date: Sun, 16 Aug 2026 15:36:50 +0200 Subject: [PATCH 1/4] fix(audio): stop the pump re-taking the lock its caller already holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the audio/video freeze reported on Linux in #11. irl_audio_thread takes audio_state_lock around the whole of irl_pump_audio_once (receiver.c), but two places inside the pump took it again: the offset re-anchor in irl_audio_maybe_reanchor_offset, and the audio_fill_peak_ms publish in the pump itself. Both are reached only once playback has primed, which is why it took a live stream to hit. A POSIX mutex is not recursive, so the second acquire hung the audio thread outright. The video thread then blocked on the same lock, nothing drained the demuxer, and OBS wound up waiting on both ("No room to store incoming packet" in the log). Windows never showed it because CRITICAL_SECTION is recursive, so the re-acquire there was a no-op. Neither inner lock was buying anything — the caller's hold already covers those writes — so they are simply removed. The comments claiming "nothing is nested here" were describing the callee in isolation and were wrong about the caller; they now state the real contract, as do the declaration in receiver-internal.h and the threading section of CLAUDE.md. Making the mutex recursive would also stop the hang, but it would relax every lock in the plugin to hide one bug in one call path, and video_queue_lock is paired with a condition variable, where a recursively-held mutex releases only one level inside the wait. Co-authored-by: Conor Wilson --- CLAUDE.md | 2 +- src/receiver-audio.c | 23 ++++++++++++----------- src/receiver-internal.h | 5 +++++ src/receiver.c | 5 +++++ 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b5987e1..becd5b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -104,7 +104,7 @@ Buffer regulation happens through playback speed only, asymmetric like IRLToolki - **Main/OBS thread**: calls create, destroy, update, tick, get_properties, and the activate/deactivate/show/hide callbacks (used only when "Close Stream When Inactive" is on) - **Receiver thread**: owns demux/decode FFmpeg state. Writes to the audio buffer (mutex protected) and pushes decoded video frames (PTS pre-converted to nanoseconds) onto the video queue. Never blocks on GPU or OBS video delivery. - **Video thread**: pops the video queue, does the HW frame transfer, paces each frame to its due time, then converts (owns sws_ctx) and calls `obs_source_output_video`. Queue overflow drops the oldest frame (`video_queue_drops`). The pacing queue it holds those frames in needs no lock — the receiver thread never touches it, and a clear is routed through `video_clear_pending` — but its counters are mirrored under `video_queue_lock` for the stats line. -- **Audio thread**: drains the jitter buffer and submits audio to OBS via `obs_source_output_audio`, paced against the sample counter output clock. Shared timing state is protected by `audio_state_lock` (lock order: `audio_state_lock` before the buffer mutex). +- **Audio thread**: drains the jitter buffer and submits audio to OBS via `obs_source_output_audio`, paced against the sample counter output clock. Shared timing state is protected by `audio_state_lock` (lock order: `audio_state_lock` before the buffer mutex). The thread takes `audio_state_lock` once around the whole of `irl_pump_audio_once`, so nothing reachable from the pump may take it again: the mutex is a plain non-recursive one, and a nested acquire hangs the audio thread and then the video thread queued behind it. Buffer-mutex calls (`audio_buffer_peek_state`, `audio_buffer_fill_ms_locked`, the reads) nest underneath it, which is the documented order. Config fields marked `/* hot */` in `struct irl_config` are written by `irl_source_update` while the worker threads run, so every cross-thread read goes through `os_atomic_load_long` / `os_atomic_load_bool` (not C11 `_Atomic`, which MSVC does not support without an experimental flag). The remaining fields are only written while the threads are stopped, where `irl_thread_create` and `irl_thread_join` supply the happens-before edge. diff --git a/src/receiver-audio.c b/src/receiver-audio.c index 270a7a8..e782ac5 100644 --- a/src/receiver-audio.c +++ b/src/receiver-audio.c @@ -637,16 +637,15 @@ static void irl_audio_maybe_reanchor_offset(struct irl_source *ctx, ctx->audio_buf.target_ms) return; - /* Lock order note: fill query above takes and releases the buffer - * mutex on its own; the state lock below is never held across it. */ - irl_mutex_lock(&ctx->audio_state_lock); + /* audio_state_lock is already held (see irl_pump_audio_once). The + * fill query above takes and releases the buffer mutex underneath it, + * which is the documented order (state lock, then buffer mutex). */ ctx->audio_out_anchor_ns = now + chunk_ns; ctx->audio_out_samples = 0; ctx->latest_audio_obs_end_ts_ns = 0; ctx->latest_audio_buffered_end_pts_ns = 0; ctx->audio_playout_offset_baseline_set = false; ctx->audio_conceal_fade_pending = true; - irl_mutex_unlock(&ctx->audio_state_lock); ctx->audio_offset_reanchors++; ctx->audio_quality_events++; @@ -704,6 +703,10 @@ static void audio_check_drain_progress(struct irl_source *ctx, int fill_ms, /* ── Pump ─────────────────────────────────────────────────── */ +/* Caller holds audio_state_lock for the whole call (irl_audio_thread). See + * the declaration in receiver-internal.h: nothing on this path may re-take + * it. Buffer-mutex calls (peek/read/fill) nest underneath it, which is the + * documented lock order. */ bool irl_pump_audio_once(struct irl_source *ctx) { bool low_latency = ctx->config.low_latency_audio; @@ -758,15 +761,13 @@ bool irl_pump_audio_once(struct irl_source *ctx) int chunk_count = 0; bool has_audio = audio_buffer_peek_state(&ctx->audio_buf, &peek, &fill_ms, &chunk_count); - /* The receiver thread reads this for the stats line, so publish it - * under the shared timing lock like the rest of the cross-thread - * state. peek_state released the buffer mutex before returning, so - * nothing is nested here and the documented order (audio_state_lock - * before the buffer mutex) still holds. */ - irl_mutex_lock(&ctx->audio_state_lock); + /* The receiver thread reads this for the stats line; audio_state_lock + * is already held for the whole pump, so the publish is covered. + * peek_state took and released the buffer mutex underneath it, which + * is the documented order (audio_state_lock before the buffer + * mutex). */ if (fill_ms > ctx->audio_fill_peak_ms) ctx->audio_fill_peak_ms = fill_ms; - irl_mutex_unlock(&ctx->audio_state_lock); if (has_audio && maybe_trim_hidden_audio_backlog(ctx, fill_ms, chunk_count)) diff --git a/src/receiver-internal.h b/src/receiver-internal.h index 482a754..ca2dad8 100644 --- a/src/receiver-internal.h +++ b/src/receiver-internal.h @@ -13,6 +13,11 @@ void irl_close_ffmpeg(struct irl_source *ctx); void irl_prepare_new_connection(struct irl_source *ctx); bool irl_wait_for_reconnect(struct irl_source *ctx); void irl_handle_stream_read_error(struct irl_source *ctx, int read_ret); +/* Caller must hold audio_state_lock: the pump owns the output clock and the + * playout mapping outright, so it reads and writes them without re-taking the + * lock anywhere below this call. The lock is NOT recursive — an inner + * irl_mutex_lock(&ctx->audio_state_lock) on this path self-deadlocks the + * audio thread. */ bool irl_pump_audio_once(struct irl_source *ctx); void irl_handle_audio_packet(struct irl_source *ctx, AVPacket *pkt, AVFrame *frame); diff --git a/src/receiver.c b/src/receiver.c index 96258a8..18e60df 100644 --- a/src/receiver.c +++ b/src/receiver.c @@ -29,6 +29,11 @@ void *irl_audio_thread(void *data) bool pumped = false; for (int i = 0; i < 16 && os_atomic_load_bool(&ctx->thread_active); i++) { + /* The whole pump runs under audio_state_lock, so + * nothing it calls may take that lock again — the + * mutex is not recursive and a nested acquire hangs + * this thread, and with it the video thread waiting + * behind it. */ irl_mutex_lock(&ctx->audio_state_lock); bool ok = irl_pump_audio_once(ctx); irl_mutex_unlock(&ctx->audio_state_lock); From dbd9a995a5e11a20c3c7f2fbb5cd182ccca1a296 Mon Sep 17 00:00:00 2001 From: datagutt Date: Sun, 16 Aug 2026 15:47:43 +0200 Subject: [PATCH 2/4] fix(threading): handle mutex init failure instead of ignoring it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit irl_mutex_init() returns an error code that nothing looked at, so a failed init left the caller locking a mutex that was never created. Unlikely in practice today, but it becomes a real path once the checked lock build sets a mutex attribute, which can fail on its own. irl_source_create() now frees its context and returns NULL; libobs logs that and never calls irl_source_destroy for a create that failed, so the bail-out is the whole cleanup. audio_buffer_init() returns bool and initialises the lock before setting sample_rate, which audio_buffer_free() reads as its "init ran" marker — leaving the struct zeroed on failure keeps free() from destroying a mutex that does not exist. Its one caller already had the plumbing to handle a failed buffer setup, alongside audio_buffer_reconfigure(). Co-authored-by: Conor Wilson --- include/audio-buffer.h | 6 +++++- src/audio-buffer.c | 23 +++++++++++++++++------ src/irl-source.c | 29 ++++++++++++++++++++++++++--- src/receiver-audio.c | 6 +++--- 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/include/audio-buffer.h b/include/audio-buffer.h index 8e2d792..e26d991 100644 --- a/include/audio-buffer.h +++ b/include/audio-buffer.h @@ -73,8 +73,12 @@ struct audio_buffer { * Initialise the buffer. Allocates storage with headroom above `max_ms` so * the configured max is not an audible old-audio drop point. * Call after the first decoded audio frame reveals the stream parameters. + * + * Returns false only if the lock could not be created, in which case the + * buffer is left zeroed and must not be used (a storage allocation failure + * still returns true and leaves an empty, usable ring). */ -void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, +bool audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, int bytes_per_sample, int target_ms, int min_ms, int max_ms); diff --git a/src/audio-buffer.c b/src/audio-buffer.c index 1e8a4a7..c63bc6d 100644 --- a/src/audio-buffer.c +++ b/src/audio-buffer.c @@ -16,6 +16,7 @@ #include #include +#include #include #include "../include/audio-buffer.h" @@ -160,12 +161,26 @@ static void pts_consume(struct audio_buffer *buf, size_t bytes_consumed) /* ── Public API ───────────────────────────────────────────── */ -void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, +bool audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, int bytes_per_sample, int target_ms, int min_ms, int max_ms) { memset(buf, 0, sizeof(*buf)); + /* Initialise the lock before publishing buf->data: stats readers + * (e.g. proc_handler) gate locking on buf->data != NULL, so the + * mutex must already be live when data becomes visible. + * + * It also has to happen before sample_rate is set, which is what + * audio_buffer_free() reads as its "init ran" marker: leaving the + * struct zeroed here keeps free() from destroying a mutex that was + * never created. */ + if (irl_mutex_init(&buf->lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create audio buffer lock"); + return false; + } + buf->sample_rate = sample_rate; buf->channels = channels; buf->bytes_per_sample = bytes_per_sample; @@ -174,11 +189,6 @@ void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, buf->min_ms = min_ms; buf->max_ms = max_ms; - /* Initialise the lock before publishing buf->data: stats readers - * (e.g. proc_handler) gate locking on buf->data != NULL, so the - * mutex must already be live when data becomes visible. */ - irl_mutex_init(&buf->lock); - /* Allocate enough headroom that Max Buffer is not an audible hard * trim point. Old audio is dropped only by explicit recovery paths. */ buf->capacity = ms_to_bytes(buf, max_ms * 4); @@ -187,6 +197,7 @@ void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, buf->data = bzalloc(buf->capacity); if (!buf->data) buf->capacity = 0; + return true; } bool audio_buffer_reconfigure(struct audio_buffer *buf, int sample_rate, diff --git a/src/irl-source.c b/src/irl-source.c index b00eba2..ca7d6ad 100644 --- a/src/irl-source.c +++ b/src/irl-source.c @@ -402,9 +402,32 @@ void *irl_source_create(obs_data_t *settings, obs_source_t *source) struct irl_source *ctx = bzalloc(sizeof(*ctx)); ctx->source = source; ctx->current_speed = 1.0f; - irl_mutex_init(&ctx->audio_state_lock); - irl_mutex_init(&ctx->video_queue_lock); - irl_cond_init(&ctx->video_queue_cond); + /* Bail out rather than run on primitives that were never created: + * every lock/unlock below this point would be undefined behaviour. + * Nothing is registered with libobs yet, so freeing ctx and returning + * NULL is the whole cleanup — libobs logs the failure and never calls + * irl_source_destroy for a create that returned NULL. */ + if (irl_mutex_init(&ctx->audio_state_lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create audio state lock"); + bfree(ctx); + return NULL; + } + if (irl_mutex_init(&ctx->video_queue_lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create video queue lock"); + irl_mutex_destroy(&ctx->audio_state_lock); + bfree(ctx); + return NULL; + } + if (irl_cond_init(&ctx->video_queue_cond) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create video queue condition variable"); + irl_mutex_destroy(&ctx->video_queue_lock); + irl_mutex_destroy(&ctx->audio_state_lock); + bfree(ctx); + return NULL; + } config_load(&ctx->config, settings); apply_async_audio_mode(ctx); diff --git a/src/receiver-audio.c b/src/receiver-audio.c index e782ac5..3a58566 100644 --- a/src/receiver-audio.c +++ b/src/receiver-audio.c @@ -928,9 +928,9 @@ void irl_handle_audio_frame(struct irl_source *ctx, AVFrame *frame) &ctx->audio_buf, out_rate, out_channels, bytes_per_sample, target_ms, min_ms, max_ms); } else { - audio_buffer_init(&ctx->audio_buf, out_rate, - out_channels, bytes_per_sample, - target_ms, min_ms, max_ms); + reconfigured = audio_buffer_init( + &ctx->audio_buf, out_rate, out_channels, + bytes_per_sample, target_ms, min_ms, max_ms); } ctx->audio_out_primed = false; ctx->audio_out_anchor_ns = 0; From f797d368267f3f17580cbb9626b30a6861793934 Mon Sep 17 00:00:00 2001 From: datagutt Date: Sun, 16 Aug 2026 15:51:30 +0200 Subject: [PATCH 3/4] feat(threading): catch nested lock acquires instead of hanging on them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audio pump's double lock hung OBS with no diagnostic beyond a stalled stream, and only on POSIX: CRITICAL_SECTION is recursive, so the nested acquire was a no-op on Windows and a deadlock everywhere else. It reached a release that way. -DIRL_CHECKED_LOCKS=ON, and Debug builds, now turn that into an immediate abort naming the file and line of the offending lock. POSIX mutexes become PTHREAD_MUTEX_ERRORCHECK, so a re-acquire returns EDEADLK rather than blocking forever and an unlock by a thread that does not hold the mutex returns EPERM; the Win32 backend reads CRITICAL_SECTION's recursion count for the nested-acquire half, which is the half Windows otherwise cannot see at all. irl_mutex_lock/unlock become function-like macros in a checked build so the abort reports the caller's location — the header's own line number would say nothing useful. Default builds are untouched: plain pthread_mutex_init, no branch on the lock path. There is no recovery once a lock call has failed (the caller would run on unprotected state and its unlock would fail in turn), so this is a development aid, not a shipping mode. Verified both directions on Linux: a nested acquire aborts with the caller's line under -DIRL_CHECKED_LOCKS, and hangs without it. --- CLAUDE.md | 2 + CMakeLists.txt | 24 ++++++++ include/irl-threading.h | 124 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index becd5b5..126282d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,6 +42,8 @@ cmake --build build --parallel Output: `build/obs-irl-source.so` (Linux/macOS) or `build/RelWithDebInfo/obs-irl-source.dll` (Windows). +`-DIRL_CHECKED_LOCKS=ON` (also automatic in Debug builds) makes a lock-contract violation abort on the spot, naming the offending file and line, instead of hanging the stream. Worth using whenever you touch the threading model: it is the difference between "OBS froze" and "src/receiver-audio.c:766 took a lock its caller already held". See the header comment in `include/irl-threading.h`. Development only — the check has no recovery path, so it stops the process. + `-DIRL_BUNDLED_FFMPEG=OFF` falls back to linking a system or obs-deps FFmpeg. That path still works for a quick compile check, but it reintroduces the per OBS line binding the bundled stack exists to remove, so it is not what releases use. `scripts/verify-plugin.sh` is not optional polish. It asserts the two properties that make the bundled stack correct and that a successful compile does not prove: that the binary carries no `libav*` dependency, and that it exports nothing but `obs_module_*`. CI runs it (and a `dumpbin` equivalent on Windows) on every build. diff --git a/CMakeLists.txt b/CMakeLists.txt index 9c171fb..812d35b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -396,6 +396,30 @@ endif() # ── C standard ─────────────────────────────────────────────── target_compile_features(obs-irl-source PRIVATE c_std_11) +# glibc hides PTHREAD_MUTEX_ERRORCHECK and pthread_mutexattr_settype() behind +# __USE_UNIX98, which a strict -std=c11 build does not define. CMake asks for +# gnu11 by default, so this only bites when C extensions are turned off — pin +# the visibility rather than depend on that default. macOS and the BSDs +# declare both unconditionally, so they need nothing. +if(UNIX AND NOT APPLE) + target_compile_definitions(obs-irl-source PRIVATE _GNU_SOURCE) +endif() + +# ── Checked locks ──────────────────────────────────────────── +# Turn a lock-contract violation (a nested acquire, or an unlock by a thread +# that does not hold the mutex) into an immediate abort naming the offending +# line, instead of a hung stream that only reproduces on POSIX. See the +# header comment in include/irl-threading.h. Costs a mutex type change and +# a branch per lock, so it is a development aid rather than a shipping mode. +option(IRL_CHECKED_LOCKS "Abort on lock-contract violations (dev builds)" OFF) +target_compile_definitions(obs-irl-source PRIVATE + $<$:IRL_CHECKED_LOCKS> +) +if(IRL_CHECKED_LOCKS) + target_compile_definitions(obs-irl-source PRIVATE IRL_CHECKED_LOCKS) + message(STATUS "obs-irl-source: checked locks enabled") +endif() + # ── Compiler warnings ──────────────────────────────────────── if(MSVC) target_compile_options(obs-irl-source PRIVATE /W4 /wd4100) diff --git a/include/irl-threading.h b/include/irl-threading.h index 367fe4d..39af150 100644 --- a/include/irl-threading.h +++ b/include/irl-threading.h @@ -33,6 +33,47 @@ #pragma once +/* ── Checked locks ──────────────────────────────────────────── + * + * Two of the plugin's locks carry contracts the compiler cannot see. The + * audio thread holds audio_state_lock across the whole of + * irl_pump_audio_once(), so nothing the pump reaches may take it again; the + * video thread waits on a condition variable under video_queue_lock, which + * needs that lock held exactly once. Breaking either hangs OBS with no + * diagnostic beyond a stalled stream. + * + * Worse, it does not hang everywhere. CRITICAL_SECTION is recursive, so a + * nested acquire is a no-op on Windows and a deadlock on Linux and macOS — + * the bug ships from a Windows desk and detonates on someone else's machine. + * That is exactly how the audio pump's double lock reached a release. + * + * A checked build turns the hang into an immediate, located abort: POSIX + * mutexes become PTHREAD_MUTEX_ERRORCHECK, where a re-acquire returns + * EDEADLK rather than blocking forever and an unlock by a thread that does + * not hold the mutex returns EPERM; Win32 reads CRITICAL_SECTION's own + * recursion count for the nested-acquire half of the same signal. + * + * Enabled by -DIRL_CHECKED_LOCKS=ON and in Debug builds (see CMakeLists.txt). + * It is a development aid, not a shipping mode: once a lock call has failed + * there is nothing to recover to, because the caller would run on + * unprotected state and its matching unlock would fail in turn. */ +#ifdef IRL_CHECKED_LOCKS + +#include +#include + +static inline void irl_lock_abort(const char *what, int code, const char *file, + int line) +{ + blog(LOG_ERROR, + "[irl-source] Lock contract violated at %s:%d: %s (code %d). " + "See the threading model in CLAUDE.md", + file, line, what, code); + abort(); +} + +#endif /* IRL_CHECKED_LOCKS */ + #ifdef _WIN32 #ifndef WIN32_LEAN_AND_MEAN @@ -71,6 +112,32 @@ static inline void irl_mutex_unlock(irl_mutex_t *m) LeaveCriticalSection(m); } +#ifdef IRL_CHECKED_LOCKS +/* RecursionCount is part of the public RTL_CRITICAL_SECTION layout and is + * incremented by the Enter above, so anything past 1 means this thread was + * already inside. There is no matching unlock check: LeaveCriticalSection on + * a section this thread does not own is undefined rather than reported, and + * inferring ownership from the undocumented OwningThread encoding would risk + * aborting a correct build. POSIX covers that half. */ +static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, + int line) +{ + EnterCriticalSection(m); + if (m->RecursionCount > 1) { + irl_lock_abort("this thread already holds this lock", + (int)m->RecursionCount, file, line); + } +} + +static inline void irl_mutex_unlock_checked(irl_mutex_t *m, const char *file, + int line) +{ + (void)file; + (void)line; + LeaveCriticalSection(m); +} +#endif /* IRL_CHECKED_LOCKS */ + static inline int irl_cond_init(irl_cond_t *c) { InitializeConditionVariable(c); @@ -159,6 +226,7 @@ static inline void irl_thread_join(irl_thread_t *t) #else /* !_WIN32 */ +#include #include #include #include @@ -169,7 +237,28 @@ typedef pthread_t irl_thread_t; static inline int irl_mutex_init(irl_mutex_t *m) { +#ifdef IRL_CHECKED_LOCKS + /* No fallback to a default mutex if the attribute cannot be set: a + * checked build that silently produced unchecked mutexes would report + * a clean run while catching nothing. + * + * glibc gates PTHREAD_MUTEX_ERRORCHECK and pthread_mutexattr_settype() + * on __USE_UNIX98, which a strict -std=c11 build does not define. The + * build defines _GNU_SOURCE there (see CMakeLists.txt). */ + pthread_mutexattr_t attr; + int ret = pthread_mutexattr_init(&attr); + if (ret != 0) + return ret; + + ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK); + if (ret == 0) + ret = pthread_mutex_init(m, &attr); + + pthread_mutexattr_destroy(&attr); + return ret; +#else return pthread_mutex_init(m, NULL); +#endif } static inline void irl_mutex_destroy(irl_mutex_t *m) @@ -187,6 +276,32 @@ static inline void irl_mutex_unlock(irl_mutex_t *m) pthread_mutex_unlock(m); } +#ifdef IRL_CHECKED_LOCKS +static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, + int line) +{ + int ret = pthread_mutex_lock(m); + if (ret == EDEADLK) { + irl_lock_abort("this thread already holds this lock", ret, + file, line); + } else if (ret != 0) { + irl_lock_abort("pthread_mutex_lock failed", ret, file, line); + } +} + +static inline void irl_mutex_unlock_checked(irl_mutex_t *m, const char *file, + int line) +{ + int ret = pthread_mutex_unlock(m); + if (ret == EPERM) { + irl_lock_abort("this thread does not hold this lock", ret, + file, line); + } else if (ret != 0) { + irl_lock_abort("pthread_mutex_unlock failed", ret, file, line); + } +} +#endif /* IRL_CHECKED_LOCKS */ + static inline int irl_cond_init(irl_cond_t *c) { #if defined(__APPLE__) @@ -266,3 +381,12 @@ static inline void irl_thread_join(irl_thread_t *t) } #endif /* _WIN32 */ + +#ifdef IRL_CHECKED_LOCKS +/* Function-like macros rather than checks inside irl_mutex_lock() itself, so + * the abort names the *caller's* file and line — the offending lock/unlock is + * the one thing a contributor needs to see, and it is never in this header. + * Both backends define the _checked helpers above. */ +#define irl_mutex_lock(m) irl_mutex_lock_checked((m), __FILE__, __LINE__) +#define irl_mutex_unlock(m) irl_mutex_unlock_checked((m), __FILE__, __LINE__) +#endif From ba6a72628b83f431bb26cf100f2eae5e35fe97d5 Mon Sep 17 00:00:00 2001 From: datagutt Date: Sun, 16 Aug 2026 16:05:12 +0200 Subject: [PATCH 4/4] feat(threading): catch unlocking an unheld lock on Windows too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The Win32 checked backend only looked for nested acquires; a stray unlock went through silently, where POSIX reports EPERM. RecursionCount == 0 means no thread is inside, so unlocking there is a contract violation and is now reported. The read is unsynchronised, but it can only ever produce a false negative — it cannot observe 0 while this thread holds the section, so a correct build is never aborted. The other half, unlocking a section a *different* thread holds, stays uncaught on Windows. Detecting it needs an owner identity, and the options are the undocumented OwningThread encoding or a field of our own; the latter would make sizeof(irl_mutex_t) depend on IRL_CHECKED_LOCKS while the type is embedded by value in struct irl_source and struct audio_buffer. A mutex whose layout varies between translation units is the bug documented at the top of this header, and it is not worth re-creating for a development aid. No condition-variable interaction: SleepConditionVariableCS releases and reacquires the section internally, so a waiting thread is never inside an unlock, and RecursionCount is back to 1 before the caller's next one. --- include/irl-threading.h | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/include/irl-threading.h b/include/irl-threading.h index 39af150..5e853ab 100644 --- a/include/irl-threading.h +++ b/include/irl-threading.h @@ -114,11 +114,8 @@ static inline void irl_mutex_unlock(irl_mutex_t *m) #ifdef IRL_CHECKED_LOCKS /* RecursionCount is part of the public RTL_CRITICAL_SECTION layout and is - * incremented by the Enter above, so anything past 1 means this thread was - * already inside. There is no matching unlock check: LeaveCriticalSection on - * a section this thread does not own is undefined rather than reported, and - * inferring ownership from the undocumented OwningThread encoding would risk - * aborting a correct build. POSIX covers that half. */ + * incremented by the Enter below, so anything past 1 means this thread was + * already inside. */ static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, int line) { @@ -129,11 +126,25 @@ static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, } } +/* Catches unlocking a section nobody holds. It does not catch unlocking one + * that a *different* thread holds: that needs an owner identity, and the only + * ones available are the undocumented OwningThread encoding or a field of our + * own — which would make sizeof(irl_mutex_t) depend on IRL_CHECKED_LOCKS, + * while the type is embedded by value in struct irl_source and struct + * audio_buffer. A mutex whose layout varies between translation units is the + * bug described at the top of this file; it is not worth re-creating for a + * debug aid. POSIX reports that case as EPERM. + * + * Reading RecursionCount unsynchronised races with other threads, but only + * ever toward a false negative: it cannot read 0 while this thread holds the + * section, so a correct build is never aborted. */ static inline void irl_mutex_unlock_checked(irl_mutex_t *m, const char *file, int line) { - (void)file; - (void)line; + if (m->RecursionCount == 0) { + irl_lock_abort("unlock of a lock no thread holds", 0, file, + line); + } LeaveCriticalSection(m); } #endif /* IRL_CHECKED_LOCKS */