From 0240b2497c1bc13fadb2d725357bcdb73a880a57 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Wed, 12 Aug 2026 12:30:29 +0100 Subject: [PATCH 1/3] fix: give the config lock a context and require it to tolerate blocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pool cleanup runs inside SolidSyslog_LockConfig, and adapter cleanup can block — closing a transport or a file. The documented primitive was a critical section, which forbids blocking, so a shipped adapter's Destroy could hang the target. The lock is now specified as a host mutex that may be held while blocking, and never a SolidSyslog Mutex, whose Create walks a pool under this lock. SolidSyslogConfigLockFunction takes a void* context and SolidSyslog_SetConfigLock installs one alongside the pair, so the handle no longer needs a file-scope static in integrator code. SolidSyslog_LockConfig and SolidSyslog_UnlockConfig keep their signatures, so no pool class changes. --- CLAUDE.md | 4 +-- Core/Interface/SolidSyslogConfigLock.h | 39 +++++++++++++--------- Core/Source/SolidSyslogConfigLock.c | 15 ++++++--- Tests/SolidSyslogConfigLockTest.cpp | 46 ++++++++++++++++++++++---- Tests/Support/ConfigLockFake.c | 10 +++--- docs/porting.md | 22 +++++------- 6 files changed, 91 insertions(+), 45 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dcbeb1af..441da985 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -727,8 +727,8 @@ role, not platform*, for the rule and the two-implementations-in-one-build cavea exhaustion site if a handler is installed. - **Slot-walk synchronisation.** Every pool's Create / Destroy wraps its slot probe in the `SolidSyslog_LockConfig` / `SolidSyslog_UnlockConfig` injection pair. Single-task targets get the - no-op default; multi-task targets wire `taskENTER_CRITICAL` (FreeRTOS), `pthread_mutex_lock` - (POSIX), `EnterCriticalSection` (Windows), etc. + no-op default; `SolidSyslogConfigLock.h` states what a multi-task target installs. Cleanup + runs inside the lock, so it must tolerate blocking and must not be a SolidSyslog Mutex. - **Shared helper.** `Core/Source/SolidSyslogPoolAllocator.{h,c}` (TU-internal) owns the three-operation contract (`AcquireFirstFree`, `FreeIfInUse`, `IndexIsValid`) every pool class reuses. No class re-implements the slot walk. diff --git a/Core/Interface/SolidSyslogConfigLock.h b/Core/Interface/SolidSyslogConfigLock.h index fa6f2eb0..97dd2989 100644 --- a/Core/Interface/SolidSyslogConfigLock.h +++ b/Core/Interface/SolidSyslogConfigLock.h @@ -1,6 +1,6 @@ /** @file - * The config-time critical-section injection pair guarding every pool - * Create/Destroy slot-walk; the no-op default suits single-task setup. */ + * The config-time lock injection pair guarding every pool Create/Destroy + * slot-walk; the no-op default suits single-task setup. */ #ifndef SOLIDSYSLOGCONFIGLOCK_H #define SOLIDSYSLOGCONFIGLOCK_H @@ -8,22 +8,31 @@ SOLIDSYSLOG_EXTERN_C_BEGIN - /** Critical-section enter/leave callback wrapping every pool Create/Destroy - * slot-walk. Single-task targets need none (the default is a no-op); - * multi-task targets wire taskENTER_CRITICAL (FreeRTOS), - * pthread_mutex_lock on a static mutex (POSIX), EnterCriticalSection - * (Windows), or a spinlock pair. Because this guards the pool walks, it is - * the one synchronisation primitive the Mutex and AtomicCounter pools can - * use for their own walks without a chicken-and-egg dependency on - * themselves. */ - typedef void (*SolidSyslogConfigLockFunction)(void); + /** Lock/unlock callback pair wrapping every pool Create/Destroy slot-walk, + * each passed the context installed with it. Single-task setup needs none; + * the default is a no-op. + * + * Where Create and Destroy can run concurrently, install a host mutex that + * may be held while blocking: Destroy releases the instance's resources + * inside this lock, and closing a transport or a file can block. A + * primitive that disables interrupts or spins deadlocks there. + * + * Never a SolidSyslog Mutex — creating one walks a pool under this lock. + * Using the host's own primitive is also what lets the Mutex and + * AtomicCounter pools use this pair for their own walks. */ + typedef void (*SolidSyslogConfigLockFunction)(void* context); /** Install the config-lock pair, applied setup-time before any Create. - * Single global slot, not synchronised against concurrent installs. Both - * handlers are set together; NULL on either side restores that side's - * no-op default. */ + * Single global slot, not synchronised against concurrent installs. All + * three are set together; @p context is passed back to both callbacks + * unchanged, and NULL on either function restores that side's no-op + * default. */ /* NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape */ - void SolidSyslog_SetConfigLock(SolidSyslogConfigLockFunction lockFn, SolidSyslogConfigLockFunction unlockFn); + void SolidSyslog_SetConfigLock( + SolidSyslogConfigLockFunction lockFn, + SolidSyslogConfigLockFunction unlockFn, + void* context + ); void SolidSyslog_LockConfig(void); void SolidSyslog_UnlockConfig(void); diff --git a/Core/Source/SolidSyslogConfigLock.c b/Core/Source/SolidSyslogConfigLock.c index 0698bf5f..c46f99a3 100644 --- a/Core/Source/SolidSyslogConfigLock.c +++ b/Core/Source/SolidSyslogConfigLock.c @@ -2,16 +2,23 @@ #include -static void ConfigLock_NoOp(void) +static void ConfigLock_NoOp(void* context) { + (void) context; } static SolidSyslogConfigLockFunction ConfigLock_Lock = ConfigLock_NoOp; static SolidSyslogConfigLockFunction ConfigLock_Unlock = ConfigLock_NoOp; +static void* ConfigLock_Context = NULL; // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape -void SolidSyslog_SetConfigLock(SolidSyslogConfigLockFunction lockFn, SolidSyslogConfigLockFunction unlockFn) +void SolidSyslog_SetConfigLock( + SolidSyslogConfigLockFunction lockFn, + SolidSyslogConfigLockFunction unlockFn, + void* context +) { + ConfigLock_Context = context; if (lockFn == NULL) { ConfigLock_Lock = ConfigLock_NoOp; @@ -32,10 +39,10 @@ void SolidSyslog_SetConfigLock(SolidSyslogConfigLockFunction lockFn, SolidSyslog void SolidSyslog_LockConfig(void) { - ConfigLock_Lock(); + ConfigLock_Lock(ConfigLock_Context); } void SolidSyslog_UnlockConfig(void) { - ConfigLock_Unlock(); + ConfigLock_Unlock(ConfigLock_Context); } diff --git a/Tests/SolidSyslogConfigLockTest.cpp b/Tests/SolidSyslogConfigLockTest.cpp index 17678c26..b6cf4e58 100644 --- a/Tests/SolidSyslogConfigLockTest.cpp +++ b/Tests/SolidSyslogConfigLockTest.cpp @@ -8,14 +8,28 @@ using namespace CososoTesting; static int testLockCallCount; static int testUnlockCallCount; +static void* testLockContext; +static void* testUnlockContext; -static void TestLock() +static void TestLockRecordingContext(void* context) { + testLockContext = context; +} + +static void TestUnlockRecordingContext(void* context) +{ + testUnlockContext = context; +} + +static void TestLock(void* context) +{ + (void) context; testLockCallCount++; } -static void TestUnlock() +static void TestUnlock(void* context) { + (void) context; testUnlockCallCount++; } @@ -64,11 +78,31 @@ TEST(SolidSyslogConfigLock, InstalledUnlockFunctionIsCalledByUnlockConfig) CALLED_FAKE(ConfigLockFake_Unlock, ONCE); } +TEST(SolidSyslogConfigLock, LockFunctionReceivesInstalledContext) +{ + int context = 0; + + SolidSyslog_SetConfigLock(TestLockRecordingContext, nullptr, &context); + SolidSyslog_LockConfig(); + + POINTERS_EQUAL(&context, testLockContext); +} + +TEST(SolidSyslogConfigLock, UnlockFunctionReceivesInstalledContext) +{ + int context = 0; + + SolidSyslog_SetConfigLock(nullptr, TestUnlockRecordingContext, &context); + SolidSyslog_UnlockConfig(); + + POINTERS_EQUAL(&context, testUnlockContext); +} + TEST(SolidSyslogConfigLock, SetConfigLockWithNullLockRestoresDefault) { - SolidSyslog_SetConfigLock(TestLock, TestUnlock); + SolidSyslog_SetConfigLock(TestLock, TestUnlock, nullptr); - SolidSyslog_SetConfigLock(nullptr, TestUnlock); + SolidSyslog_SetConfigLock(nullptr, TestUnlock, nullptr); SolidSyslog_LockConfig(); CALLED_FUNCTION(testLock, NEVER); @@ -76,9 +110,9 @@ TEST(SolidSyslogConfigLock, SetConfigLockWithNullLockRestoresDefault) TEST(SolidSyslogConfigLock, SetConfigLockWithNullUnlockRestoresDefault) { - SolidSyslog_SetConfigLock(TestLock, TestUnlock); + SolidSyslog_SetConfigLock(TestLock, TestUnlock, nullptr); - SolidSyslog_SetConfigLock(TestLock, nullptr); + SolidSyslog_SetConfigLock(TestLock, nullptr, nullptr); SolidSyslog_UnlockConfig(); CALLED_FUNCTION(testUnlock, NEVER); diff --git a/Tests/Support/ConfigLockFake.c b/Tests/Support/ConfigLockFake.c index 7f8d6886..69789794 100644 --- a/Tests/Support/ConfigLockFake.c +++ b/Tests/Support/ConfigLockFake.c @@ -7,13 +7,15 @@ static int lockCallCount; static int unlockCallCount; -static void Lock(void) +static void Lock(void* context) { + (void) context; lockCallCount++; } -static void Unlock(void) +static void Unlock(void* context) { + (void) context; unlockCallCount++; } @@ -21,12 +23,12 @@ void ConfigLockFake_Install(void) { lockCallCount = 0; unlockCallCount = 0; - SolidSyslog_SetConfigLock(Lock, Unlock); + SolidSyslog_SetConfigLock(Lock, Unlock, NULL); } void ConfigLockFake_Uninstall(void) { - SolidSyslog_SetConfigLock(NULL, NULL); + SolidSyslog_SetConfigLock(NULL, NULL, NULL); } int ConfigLockFake_LockCallCount(void) diff --git a/docs/porting.md b/docs/porting.md index 66863391..d0812f74 100644 --- a/docs/porting.md +++ b/docs/porting.md @@ -133,26 +133,20 @@ The pool allocator wraps each slot claim and release in the `AcquireFirstFree` locks per-slot around the claim, `FreeIfInUse` locks around the release, so an adapter's `_Create` / `_Destroy` inherit the synchronisation for free and never lock themselves (which is why the example above has no lock call). -Single-task setup gets the no-op default and pays nothing. On a multi-task or multi-core target -where setup races, install the pair once with `SolidSyslog_SetConfigLock(...)`: -`taskENTER_CRITICAL` / `taskEXIT_CRITICAL` (FreeRTOS), a static `pthread_mutex_t` -(POSIX), `EnterCriticalSection` / `LeaveCriticalSection` (Windows), or a spinlock. -This is the only synchronisation primitive the pools use for their own walks. +Single-task setup gets the no-op default and pays nothing. Where setup races, the +integrator installs the pair once with `SolidSyslog_SetConfigLock(...)`; +[`SolidSyslogConfigLock.h`](api/SolidSyslogConfigLock_8h.md) states what that lock +must be. This is the only synchronisation primitive the pools use for their own +walks. + +Your `Destroy` runs inside it, so cleanup may block and the installed lock must +allow it. ## Invariants every adapter must honour - Idempotent `Close` / `Destroy`. No leak on a partial `Open` failure, no double-free if `Close` and `Destroy` are both called. Release each resource exactly once and null the handle. -- Cleanup runs under the config lock. `FreeIfInUse` holds - `SolidSyslog_LockConfig()` across your cleanup callback, so whatever `Destroy` - does to release a resource happens inside whichever primitive the integrator - installed — a FreeRTOS critical section, at the recommendation above. Blocking - there is not safe: a mutex may not be taken inside `taskENTER_CRITICAL`, and - work that needs another task to run cannot complete while interrupts are off. - The shipped lwIP TCP stream does block this way, tracked as - [#754](https://github.com/cososo-ltd/solid-syslog/issues/754); until that is - resolved, keep teardown in your own adapter non-blocking. - Never free injected handles. An adapter frees only what it created. Handles the integrator passed in (a certificate, an RNG, a caller's socket) are borrowed; the owner frees them. The same applies to an upstream library's From 4303b07b4d68a55d58211caf086f18d5ddf8e141 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Wed, 12 Aug 2026 12:53:15 +0100 Subject: [PATCH 2/3] fix: suppress the swappable-parameter warning across the wrapped signature --- Core/Interface/SolidSyslogConfigLock.h | 23 ++++++++++++----------- Core/Source/SolidSyslogConfigLock.c | 3 ++- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/Core/Interface/SolidSyslogConfigLock.h b/Core/Interface/SolidSyslogConfigLock.h index 97dd2989..b4cb8477 100644 --- a/Core/Interface/SolidSyslogConfigLock.h +++ b/Core/Interface/SolidSyslogConfigLock.h @@ -9,30 +9,31 @@ SOLIDSYSLOG_EXTERN_C_BEGIN /** Lock/unlock callback pair wrapping every pool Create/Destroy slot-walk, - * each passed the context installed with it. Single-task setup needs none; - * the default is a no-op. + * each passed the context installed with it. The default is a no-op, which + * suits single-task setup. * - * Where Create and Destroy can run concurrently, install a host mutex that - * may be held while blocking: Destroy releases the instance's resources - * inside this lock, and closing a transport or a file can block. A - * primitive that disables interrupts or spins deadlocks there. + * Where Create and Destroy can run concurrently, install a mutex supplied + * by the host system. Destroy releases the instance's resources inside + * this lock and may block, so the primitive must permit blocking while + * held; one that disables interrupts or spins will deadlock. * - * Never a SolidSyslog Mutex — creating one walks a pool under this lock. - * Using the host's own primitive is also what lets the Mutex and - * AtomicCounter pools use this pair for their own walks. */ + * A SolidSyslog Mutex is not a valid choice: its Create walks a pool under + * this lock. Requiring a host primitive is also what allows the Mutex and + * AtomicCounter pools to use this pair for their own walks. */ typedef void (*SolidSyslogConfigLockFunction)(void* context); /** Install the config-lock pair, applied setup-time before any Create. * Single global slot, not synchronised against concurrent installs. All - * three are set together; @p context is passed back to both callbacks + * three are set together; @p context is passed to both callbacks * unchanged, and NULL on either function restores that side's no-op * default. */ - /* NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape */ + /* NOLINTBEGIN(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape */ void SolidSyslog_SetConfigLock( SolidSyslogConfigLockFunction lockFn, SolidSyslogConfigLockFunction unlockFn, void* context ); + /* NOLINTEND(bugprone-easily-swappable-parameters) */ void SolidSyslog_LockConfig(void); void SolidSyslog_UnlockConfig(void); diff --git a/Core/Source/SolidSyslogConfigLock.c b/Core/Source/SolidSyslogConfigLock.c index c46f99a3..d684a15e 100644 --- a/Core/Source/SolidSyslogConfigLock.c +++ b/Core/Source/SolidSyslogConfigLock.c @@ -11,12 +11,13 @@ static SolidSyslogConfigLockFunction ConfigLock_Lock = ConfigLock_NoOp; static SolidSyslogConfigLockFunction ConfigLock_Unlock = ConfigLock_NoOp; static void* ConfigLock_Context = NULL; -// NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape +// NOLINTBEGIN(bugprone-easily-swappable-parameters) -- deliberate pair API: lock and unlock are installed together and conceptually inseparable; matches SolidSyslog_SetErrorHandler's pair shape void SolidSyslog_SetConfigLock( SolidSyslogConfigLockFunction lockFn, SolidSyslogConfigLockFunction unlockFn, void* context ) +// NOLINTEND(bugprone-easily-swappable-parameters) { ConfigLock_Context = context; if (lockFn == NULL) From d10bc0ec7df14d2815a7c7b294b92a81766fed06 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Wed, 12 Aug 2026 14:10:00 +0100 Subject: [PATCH 3/3] docs: name the lock as the subject in the slot-walk note --- CLAUDE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 441da985..895e6fec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -727,8 +727,8 @@ role, not platform*, for the rule and the two-implementations-in-one-build cavea exhaustion site if a handler is installed. - **Slot-walk synchronisation.** Every pool's Create / Destroy wraps its slot probe in the `SolidSyslog_LockConfig` / `SolidSyslog_UnlockConfig` injection pair. Single-task targets get the - no-op default; `SolidSyslogConfigLock.h` states what a multi-task target installs. Cleanup - runs inside the lock, so it must tolerate blocking and must not be a SolidSyslog Mutex. + no-op default. Cleanup runs inside the lock, so a multi-task target must install one that + tolerates blocking and is not a SolidSyslog Mutex; `SolidSyslogConfigLock.h` states the contract. - **Shared helper.** `Core/Source/SolidSyslogPoolAllocator.{h,c}` (TU-internal) owns the three-operation contract (`AcquireFirstFree`, `FreeIfInUse`, `IndexIsValid`) every pool class reuses. No class re-implements the slot walk.