Scott Duensing ac2d587aa4 Maybe base features complete?

2026-07-04 18:40:32 -05:00

216 KiB

Raw Blame History

calog full audit -- 114 confirmed findings

Original audit: 15 scoped reviewers + 1 adversarial verifier per finding (118 raw, 4 refuted, 114 confirmed).

Remediation status

110 FIXED, 2 PARTIAL, 1 DECLINED, 1 KEPT. Verified: make all clean (-Werror -Wconversion, ASan/UBSan), make test 28/28 binaries 0 failed, TSan (actor + mybasic) clean, 31 example scripts + multi-file pass.

FIXED -- applied and verified.
PARTIAL -- addressed in part; the residual is declined with a reason on the finding.
DECLINED -- not applied; the finding was a false-dedup or invalid.
KEPT -- intentionally unchanged.

Shared helpers now carry the de-duplicated logic (in value.c / calogInternal.h): calogGrow, calogValueFromDouble, calogAggIsKeyed, calogMapSet*, calogRegistryRetain/Release, calogHexNibble, calogHandleGetAny/RemoveAny, plus the new src/adapterBinding.h.

HIGH (7)

H1. [bug] libs/calogHttp.c:449 --- FIXED

Status: FIXED

TLS write path can raise SIGPIPE and kill the whole broker process; plain path is protected but SSL_write/SSL_shutdown are not.

The plaintext path deliberately uses send(conn->fd, bytes + sent, length - sent, MSG_NOSIGNAL) (line 456), but the https path calls w = SSL_write(conn->ssl, bytes + sent, chunk) (line 449) and SSL_shutdown(conn->ssl) (line 355). The vendored OpenSSL socket BIO writes via writesocket(s,b,n) which is send((s),(b),(n),0) / write() (vendor/openssl/include/internal/sockets.h:178-195) -- no MSG_NOSIGNAL, and Linux has no SO_NOSIGPIPE. Nothing in the process ignores SIGPIPE (src/calogMain.c only installs handlers for SIGINT/SIGTERM at lines 455-456; grep -rn SIGPIPE src/ libs/ finds nothing). Concrete failure: an https POST to a server that resets/closes the connection before the request is fully written (e.g. early 413 + close on a large body, or any mid-upload RST) makes the second SSL_write hit a broken pipe, deliver SIGPIPE, and terminate the entire multi-engine broker with the default disposition. Fix: signal(SIGPIPE, SIG_IGN) at startup, or block SIGPIPE per-thread, or use a BIO that passes MSG_NOSIGNAL.

Verifier: Verified in code: libs/calogHttp.c:449 (SSL_write) and :355 (SSL_shutdown) run over OpenSSL's default socket BIO (SSL_set_fd at :1002), whose writesocket macro in vendor/openssl/include/internal/sockets.h:178-195 is send(...,0)/write() with no MSG_NOSIGNAL; no setsockopt on the fd. Grep of src/ and libs/ finds zero SIGPIPE handling -- calogMain.c:455-456 only installs SIGINT/SIGTERM handlers, and no linked-library init (libssh2_init, libpq) ignores SIGPIPE process-wide. The plaintext path (calogHttp.c:456) and calogNet.c:447 deliberately use MSG_NOSIGNAL, confirming the TLS path is an unintended gap, and no comment documents it as a tradeoff. The full request is written in one httpConnWriteAll call (:855), so a >16KB body spans multiple internal send()s within one SSL_write; a peer RST mid-write (e.g. early 413+close on a large upload) makes the next send() hit EPIPE, raising SIGPIPE with default disposition and terminating the entire multi-engine broker.

H2. [bug] libs/calogSsh.c:738 --- FIXED

Status: FIXED

sshExec drains stdout fully to EOF before touching stderr, so a stderr-heavy command buffers unbounded data inside libssh2, bypassing SSH_MAX_TRANSFER and hanging/OOMing exactly as the cap is documented to prevent.

Lines 737-743: // Blocking reads: drain stdout to EOF, then stderr (libssh2 buffers the other stream). / status = sshDrain(channel, 0, &outBytes, &outLength); then sshDrain(channel, SSH_EXTENDED_DATA_STDERR, ...). The comment is right that libssh2 buffers the other stream -- and that is the problem: the vendored _libssh2_channel_read (vendor/libssh2/src/channel.c, 'expand the receiving window first if it has become too narrow') re-extends the 2MB receive window on every read call WITHOUT regard to how much unread data is already queued, so while sshDrain blocks on stream 0, stderr packets accumulate in session->packets with no limit. SSH_MAX_TRANSFER (line 35, whose comment says 'without it a huge/endless remote source would grow an unbounded buffer and OOM-kill the shared host process') is only enforced on the app-side buffer of the stream currently being drained. Failure scenario: sshExec(h, "yes spam 1>&2") -- stdout never EOFs, stderr floods, libssh2's internal queue grows forever, the native never returns and the host process is OOM-killed. Even for finite output, a command writing 10GB to stderr buffers all 10GB in libssh2 before the second sshDrain runs and finally fails with calogErrRangeE. Fix: interleave reads of stream 0 and SSH_EXTENDED_DATA_STDERR in one loop (break when both hit EOF), enforcing the cap on the running total.

Verifier: The core defect is confirmed by the code, though the claimed OOM mechanism is refuted. Confirmed: libs/calogSsh.c:738-743 drains stdout to EOF before stderr, in blocking mode with no libssh2 session timeout. In the vendored libssh2, stderr packets queue into the shared per-channel session->packets/read_avail while stream 0 is being read, and _libssh2_channel_read's window top-up only fires when remote.window_size drops -- which happens only when the app reads bytes out. During a stderr-only flood on stream 0, bytes_read stays 0, so after one top-up no further WINDOW_ADJUST is sent; a compliant sshd stops sending after the ~2MB advertised window (LIBSSH2_CHANNEL_WINDOW_DEFAULT), the remote command blocks writing stderr, never exits, stdout never EOFs, and sshExec hangs forever. Any command writing more than ~2MB to stderr before completing (verbose build, tar -v) permanently wedges the calling context thread and the owner-scoped connection handle; SSH_MAX_TRANSFER never triggers because no stderr byte reaches the app buffer. The line-737 comment's premise that 'libssh2 buffers the other stream' is false beyond the window. Refuted: no unbounded growth/OOM -- packet.c lines 1047-1074 drop/truncate arrivals once read_avail >= remote.window_size, capping internal buffering at ~2MB+buflen, and the '10GB buffered then calogErrRangeE' scenario never happens (it hangs in the first sshDrain instead). The proposed fix (interleave both streams in one loop with a shared cap) is correct.

H3. [race] libs/calogTask.c:198 --- FIXED

Status: FIXED

Non-owner taskEval/taskClose can dereference a freed TaskT: calogHandleGet returns a raw pointer, then task->ownerId is read while the owner may concurrently taskClose (calogHandleRemove + free).

taskEval does task = (TaskT *)calogHandleGet(lib->handles, args[0].as.i, TASK_TYPE_CONTEXT); (line 194) then if (task->ownerId != calogCurrentId()) (line 198); taskClose has the same Get-then-deref at lines 138/142 before its Remove at 145 and free(task) at line 151. calogHandleGet (libs/calogHandle.c:83) returns the stored pointer with no pinning -- the table mutex is released before return. The ownership check is an ENFORCED cross-context rejection path (calogTask.h:16: "ONLY that owner may taskEval or taskClose it (others get an error)"), so a non-owner calling with a live handle is a supported input. Concrete failure: context A spawns a task and shares the int64 handle via kvSet; context B calls taskEval(h, ...) while A calls taskClose(h). B's Get returns the TaskT*, A's taskClose removes the entry and frees the TaskT, then B reads task->ownerId from freed memory (use-after-free; with malloc reuse the check can even spuriously pass and B then uses a dangling task->context). The comment at lines 133-136 ("the owner is a single thread ... no other thread can claim this handle") only covers the owner's own peek/remove pair, not non-owner readers. Fix: make the owner check atomic with the table operation, e.g. store ownerId in the handle entry, add a calogHandleGetChecked/RemoveIfOwner that compares under the table mutex, or refcount the wrapper.

Verifier: Verified in code: calogHandleGet (libs/calogHandle.c:83-102) copies the raw resource pointer and releases the table mutex before returning; taskEval (libs/calogTask.c:194,198) and taskClose (:138,142) then dereference task->ownerId outside any lock, while the owner's taskClose frees the TaskT (:145,150). Non-owner calls with a live handle are a supported input per the enforced rejection contract in calogTask.h:15-16, contexts run natives concurrently on their own threads (calogTask.c:1-3), and handles are guessable monotonic int64s starting at 1 (calogHandle.c:57-58,136). The comment at calogTask.c:135-137 only justifies the owner's own peek/remove serialization and does not cover non-owner readers, so this is not a documented tradeoff. Result is a real use-after-free; with malloc reuse the stale ownerId can spuriously match and the caller then uses a dangling task->context.

H4. [race] src/context.c:151 --- FIXED

Status: FIXED

calogActorInit unconditionally overwrites the thread-local currentContext, so with two runtimes on one thread any calogCall into the older runtime deadlocks forever on a reply box nobody can service.

Line 151: currentContext = calog->hostContext;. currentContext is a single slot per thread, so creating runtime B repoints the thread's identity away from runtime A's host. Deterministic failure: calogCreate() -> A, calogCreate() -> B on the same thread, then calogCall(A, "someNative", ...) for a non-inline native. actorRoute (line 210) sees currentContext (B's host) != A->hostContext and marshals to A's host queue; contextSendBlocking (line 804) sees currentContext->broker != A, takes the foreign-caller reply-box path, and blocks in while (!box.done) pthread_cond_wait(...) (line 837-839) with no timeout -- but the only thread that ever pumps A's host queue is the one now blocked. Permanent hang. The file's own header (lines 20-22) advertises one thread hosting several runtimes via calogPump, but any direct host-thread call between pumps into a non-most-recent runtime deadlocks. Fix: record the host pthread_t in CalogT at calogActorInit and have actorRoute/contextSendBlocking treat pthread_equal(pthread_self(), calog->hostThread) as being on the host (running inline / pumping the host queue), instead of relying on the single-slot currentContext.

Verifier: Verified end-to-end in src/context.c: line 151 unconditionally overwrites the thread-local currentContext on every calogCreate, and only calogPump (which save/restores) ever changes it back, so after creating runtime B the thread permanently presents as B's host. calogCall(A, nonInlineNative) then fails actorRoute's inline check (line 210, pointer equality with A->hostContext), marshals to A's host queue, and contextSendBlocking (line 804) classifies the caller as foreign (currentContext->broker == B != A), taking the reply-box path that blocks in an untimed pthread_cond_wait (lines 837-839). A's host queue is drained only by calogPump(A) via tryDequeue/hostDispatch, and the only thread that would call it is the one now blocked -- deterministic permanent deadlock. Not a documented tradeoff: calog.h lines 5-8 explicitly advertise one thread driving several runtimes, testEngineLua.c:326 tests that mode (but only pump-driven script->native calls, never a direct host calogCall into the older runtime), and the comment at lines 800-803 addresses reply misrouting, not the unserviceable-wait case. The single-runtime path proves direct host-thread calogCall outside calogPump is intended usage (line 151 exists precisely to make it run inline).

H5. [bug] src/s7/s7Adapter.c:378 --- FIXED

Status: FIXED

s7FromValue builds hash-tables and lists without gc-protecting the container while recursive calls allocate, so s7's GC can reclaim the partially built aggregate.

In the calogAggE branch: table = s7_make_hash_table(sc, size > 0 ? size : 1); then a loop of s7_hash_table_set(sc, table, s7_make_integer(...), s7FromValue(context, ..., depth + 1)); (lines 378-396), and the list branch list = s7_cons(sc, element, list); after element = s7FromValue(...) (lines 401-404). Neither table nor the growing list chain is referenced from any GC root during construction. In vendored s7, every allocation goes through new_cell which calls try_to_call_gc when the free-heap trigger is hit (vendor/s7/s7.c:4192), and the GC only marks the most recent gc_temps_size (256) allocations (vendor/s7/s7.c:8186-8198). Failure scenario: marshalling a large aggregate (e.g. a DB result set of a few hundred string rows, exactly what the in-progress calogDb work will produce) exceeds the 256-cell temp window, GC fires mid-build, and the container head or earlier elements are swept -- use-after-free / silently corrupted values. Small test payloads never trigger it, which is why ASan tests pass. The code demonstrably knows the rule: s7ToValue protects iter at line 531. Fix: s7_gc_protect the table (and for the list branch, a protected holder cell updated with s7_set_car after each cons, since the head moves) and unprotect before returning.

Verifier: Verified against both the adapter and vendored s7: src/s7/s7Adapter.c:378-397 creates the hash table and then loops recursive s7FromValue calls with the table held only in a C local; s7's GC is precise (no C-stack scan), new_cell can invoke try_to_call_gc on any allocation (vendor/s7/s7.c:4192), and the mark phase protects only the last gc_temps_size=256 allocations plus rooted objects (s7.c:171, 8191-8198). s7_make_hash_table (s7.c:47238) and s7_hash_table_set (s7.c:47915) root nothing, so after ~128 string key/value entries (2 cells each) the table exits the temp window and a triggered GC sweeps it mid-build, leaving subsequent s7_hash_table_set calls writing into a reallocated cell. The list branch at 399-406 is exposed only when a nested element's marshal allocates >256 cells, since the head cons is otherwise always recent. No compensating guard exists (no gc_on(false), no call-site protection), and the codebase follows the protect rule elsewhere (s7ToValue protects its iterator at lines 531/556). Only mitigating factor: requires a large payload coinciding with the free-heap trigger, which is why small ASan tests pass.

H6. [bug] src/wren/wrenAdapter.c:230 --- FIXED

Status: FIXED

wrenDispatch and wrenForeignInvoke read slots without type validation; a script passing wrong types gets undefined behavior instead of a fiber abort because Wren is built without -DDEBUG.

Line 230-231: name = wrenGetSlotString(vm, 1); argCount = wrenGetListCount(vm, 2); and line 370: argCount = wrenGetListCount(vm, 1);. The preamble signature call(name, args) fixes arity but not types. wrenGetSlotString/wrenGetListCount only guard types with ASSERT (vendor/wren/wren.c:4482, 4563), and ASSERT is a no-op without DEBUG (vendor/wren/wren.c:780); the Makefile builds wren with WRENFLAGS = -std=c99 -w -g -O1 (Makefile:118), so AS_STRING/AS_LIST reinterpret raw NaN-boxed bits as an object pointer. Failure scenario: Calog.call(42, 7) or f.call(nil) in a script dereferences a garbage pointer and crashes or corrupts the broker, contradicting the documented error model ('a native failure sets the error string in slot 0 and wrenAbortFiber'). Fix: check wrenGetSlotType(vm, 1) == WREN_TYPE_STRING && wrenGetSlotType(vm, 2) == WREN_TYPE_LIST (and WREN_TYPE_LIST in wrenForeignInvoke) and abort the fiber with a message otherwise.

Verifier: Empirically reproduced: a one-line Wren script Calog.call(42, 7) segfaults the broker (ASan SEGV in wrenGetListCount at vendor/wren/wren.c:4566, called from wrenDispatch at src/wren/wrenAdapter.c:231). The preamble call(name, args) fixes arity only; slots 1/2 are read via wrenGetSlotString/wrenGetListCount whose type guards are ASSERTs compiled out because Makefile:118 (WRENFLAGS = -std=c99 -w -g -O1) lacks -DDEBUG. Same hole in wrenForeignInvoke at line 370 for f.call(nil). No comment documents a tradeoff; the file header (line 11) documents the opposite contract (errors should wrenAbortFiber). The natural typo Calog.call("report", 42) -- forgetting the list brackets the API forces -- crashes the whole multi-script broker with a wild-pointer read, so this is memory-unsafe and reachable in normal use, warranting high rather than medium.

H7. [bug] src/wren/wrenAdapter.c:369 --- FIXED

Status: FIXED

A script calling CalogFn.new().call([...]) passes NULL into calogFnInvoke and segfaults the broker.

wrenForeignInvoke does callable = *(CalogFnT **)wrenGetSlotForeign(vm, 0); and later calogFnInvoke(callable, cargs, argCount, &result); (line 395) with no NULL check. wrenForeignAllocate (line 342) deliberately stores *data = NULL; for script-constructed instances -- its own comment admits this path is 'only reached if a script does CalogFn.new()' -- and wrenForeignFinalize (line 351) does check NULL, but the invoke path does not. calogFnInvoke (src/value.c:235) immediately executes atomic_load_explicit(&callable->alive, ...), a NULL dereference. Failure scenario: the one-line Wren script CalogFn.new().call([]) crashes the whole broker process, violating the adapter's stated error model (set error in slot 0 + wrenAbortFiber). Fix: if (callable == NULL) { wrenSetSlotString(vm, 0, "empty CalogFn"); wrenAbortFiber(vm, 0); return; } right after fetching it.

Verifier: Verified end-to-end: WREN_PREAMBLE (wrenAdapter.c:64-67) declares 'construct new() {}' on foreign class CalogFn in module "main", the same module user scripts run in (lines 92, 498), so any script can call CalogFn.new(); wrenForeignAllocate (line 342) intentionally stores *data = NULL; wrenForeignInvoke (line 369) fetches that NULL and passes it unguarded to calogFnInvoke (line 395); calogFnInvoke (src/value.c:237) immediately does atomic_load_explicit(&callable->alive, ...) with no NULL check, a NULL dereference. The finalizer (line 351) guards NULL for this exact empty-instance state, proving the state was meant to be handled and the missing check in the invoke path is an oversight, not a documented tradeoff; the function's own error model (set string in slot 0 + wrenAbortFiber, lines 375-377/390-392/403-404) is what the one-line fix should follow.

MEDIUM (45)

M1. [duplication] Makefile:492 --- FIXED

Status: FIXED

The eight tsan targets repeat the flag group -std=c11 $(WARN) -g -O1 -fsanitize=thread -pthread $(INC) and the source group src/context.c src/value.c src/broker.c eleven times each.*

Every tsan recipe line (492, 495, 505, 516, 528, 538, 548, 558, 568, 572, 576) spells out the identical TSan compile flags, and each also relists the actor-core sources src/context.c src/value.c src/broker.c. Two variables (TSANFLAGS = -std=c11 $(WARN) -g -O1 -fsanitize=thread -pthread and TSANCORE = src/context.c src/value.c src/broker.c) would remove ~22 duplicated fragments that must currently be edited in lockstep (e.g. bumping -O1 or adding a core file means touching 11 lines). tsanlibs additionally repeats its own 3-line compile+setarch block three times differing only in the test/lib name, and is the one tsan target missing from the .PHONY list at line 587 (a stray file named tsanlibs would silently no-op it while all its siblings are protected).

Verifier: Verified in /home/scott/claude/calog/Makefile: the flag group -std=c11 $(WARN) -g -O1 -fsanitize=thread -pthread $(INC) appears verbatim on all 11 claimed lines (492, 495, 505, 516, 528, 538, 548, 558, 568, 572, 576) and src/context.c src/value.c src/broker.c is relisted on the 11 matching link lines. This Makefile already factors every other flag group into variables (COREFLAGS, ADPFLAGS, QJSFLAGS, RELFLAGS...) and even has CORE = obj/value.o obj/broker.o at line 33, so the inline tsan repetition is both inconsistent with the file's own style and a second source of truth for the actor core. Also confirmed: .PHONY at line 587 omits tsanlibs (all sibling tsan targets are listed; ssh-test has its own .PHONY at 474), so a stray file named tsanlibs newer than lib/liblua.a makes make tsanlibs silently no-op; and tsanlibs' compile+setarch block is triplicated at 568-579 differing only in Timer/Pubsub/Kv. Concrete payoff: any flag or core-file change requires 11 lockstep edits, and a missed edit yields a silently inconsistent TSan build. Cleanup with clear payoff, but no runtime failure, so medium not high.

M2. [error-path] Makefile:504 --- FIXED

Status: FIXED

The tsansq/tsanjs/tsanberry compile for-loops mask mid-loop compiler failures because the shell loop's exit status is that of the last iteration only.

Line 504: for f in $(SQSRC); do $(CXX) $(SQXXFLAGS) -fsanitize=thread -c $$f -o obj/$$(basename $${f%.cpp}).tsan.o; done (same pattern at 515 tsanjs and 537 tsanberry). If any file except the last fails to compile, the loop keeps going, the recipe reports success, and make proceeds to the link step. Worst case: the cleanup rm -f *.tsan.o at the end of each target only runs if the preceding setarch test PASSES, so a previous aborted run leaves stale .tsan.o files behind; the next run's masked compile failure then links the STALE object silently and the TSan result is for old code. Fix: || exit 1 inside each loop body (or set -e in the recipe line).

Verifier: Makefile lines 504/515/537 run multi-file compiles inside for f in ...; do $(CC) ...; done with no || exit 1; the Makefile sets no SHELL/.SHELLFLAGS/.POSIX, so the loop's exit status is the last iteration's only (verified empirically: a mid-loop failure in an identical recipe lets make exit 0). The cleanup rm -f *.tsan.o at lines 510/520/542 only runs after a passing setarch test, so a prior failed/aborted TSan run leaves stale .tsan.o files; the next run's masked compile failure then links the stale object and the TSan result silently covers old code. Sibling targets tsanmb/tsans7/tsanwren compile directly and fail correctly, confirming the loops are the anomaly, and no comment documents the error-tolerance as intentional.

M3. [bug] libs/calogCrypto.c:86 --- FIXED

Status: FIXED

cryptoBase64Decode accepts malformed base64 containing interior '=' and returns silent garbage instead of an error.

decoded = EVP_DecodeBlock(out, in, (int)length); -- EVP_DecodeBlock treats '=' anywhere in the input as a zero sextet rather than rejecting it, and the pad trim at lines 92-97 only inspects the final two bytes. So cryptoBase64Decode("YQ==YQ==") returns the 4 bytes "a\0\0a" with calogOkE instead of failing, and "A=B=" returns 2 garbage bytes. This file already documents and compensates for EVP_DecodeBlock's other quirks (the padding-byte comment at lines 84-85, the whitespace comment at 61-63) but this one is unhandled; a scan rejecting '=' anywhere except the last two positions would close it.

Verifier: Confirmed against the vendored OpenSSL: in vendor/openssl/crypto/evp/encode.c, data_ascii2bin[0x3D] ('=') is 0x00, which passes the only error check ((a|b|c|d) & 0x80) in evp_decodeblock_int, and EVP_DecodeBlock invokes it with eof=0 so even final-block '=' handling is skipped -- every '=' decodes silently as a zero sextet. Tracing libs/calogCrypto.c:76-98: "YQ==YQ==" passes the %4 length check, EVP_DecodeBlock returns 6 ('a',0,0,'a',0,0), the pad trim at lines 92-97 only inspects in[length-1] and in[length-2], so the function returns 4 bytes "a\0\0a" with calogOkE; "A=B=" returns 2 garbage bytes (0x00,0x00) with calogOkE. This is not a documented tradeoff: line 89's "invalid base64 data" error path shows malformed input is meant to be rejected, and the comments at lines 61-63/84-85 compensate for EVP_DecodeBlock's other quirks but not this one. Header and tests contain no caveat and no guard exists elsewhere. No memory unsafety (outCap exactly bounds the write) and all valid base64 decodes correctly, so impact is limited to silent garbage-for-garbage on malformed input.

M4. [duplication] libs/calogDb.c:796 --- FIXED

Status: FIXED

The rows-to-CalogValueT marshalling loop is triplicated almost verbatim across mysqlQuery, pgQuery, and sqliteQuery, and the copies have already drifted (see the uninitialized-value finding).

mysqlQuery lines 634-676, pgQuery lines 796-835, and sqliteQuery lines 1047-1088 are the same ~40-line block: calogAggCreate(rowMap) -> per column: calogValueString(&key, name, strlen(name)) -> backend column marshal -> calogAggSet -> 6-line error unwind (calogValueFree(&key); calogValueFree(&value); calogAggFree(rowMap); calogAggFree(rows); <backend cleanup>; return calogFail(...)) -> calogValueAgg + calogAggPush + its own 4-line unwind. Only the column-name getter and cell marshaller differ. A shared helper taking columnCount plus a columnName(ctx, i) / columnValue(ctx, i, &value) callback pair (or a per-row callback) would collapse ~80 duplicated lines, and would have prevented the mysql-vs-pg/sqlite drift where only mysqlQuery pre-nils value (line 646). The three open tails are also triplicated (lines 453-459, 767-773, 976-982: calogHandleAdd -> handle==0 -> close + "dbOpen: out of memory" -> calogValueInt), worth one small dbFinishOpen(lib, type, conn, closeFn, result) helper.

Verifier: Verified by reading libs/calogDb.c: the ~40-line row-marshalling block is triplicated nearly verbatim in mysqlQuery (634-676), pgQuery (796-835), and sqliteQuery (1047-1088), differing only in the column-name getter, cell marshaller, and backend cleanup call, so the proposed callback-based helper is concretely feasible. The claimed drift is real and consequential: mysqlQuery pre-nils value (line 646) but pgQuery/sqliteQuery do not, and src/value.c confirms calogValueString on key failure leaves value uninitialized, so their shared unwind calls calogValueFree on uninitialized stack memory (potential free of a garbage pointer if the garbage type equals calogStringE). The triplicated dbOpen tails (453-459, 767-773, 976-982) also check out. This is a duplication finding with clear payoff (~80 lines collapsed, drift-bug class eliminated), matching the project's explicit instruction to consolidate duplicated code.

M5. [error-path] libs/calogDb.c:820 --- FIXED

Status: FIXED

pgQuery and sqliteQuery call calogValueFree on an uninitialized CalogValueT when column-key allocation fails, which is undefined behavior (potential free of a garbage pointer).

In pgQuery (lines 807-820) CalogValueT value; is declared uninitialized; status = calogValueString(&key, name, ...) runs first and pgColumn (which nils value as its first act) is only reached if (status == calogOkE). When calogValueString fails (malloc failure -- src/value.c:469-472 returns calogErrOomE), the unwind block executes calogValueFree(&value); on stack garbage. calogValueFree (src/value.c:398) switches on value->type; garbage matching calogStringE/calogAggE/calogFnE leads to free()/calogAggFree()/calogFnRelease() on a wild pointer -> heap corruption or crash. The identical bug is in sqliteQuery at line 1073 (value declared at 1061, sqliteColumn skipped on key failure). mysqlQuery is the only correct copy: it calls calogValueNil(&value); at line 646 BEFORE building the key. Fix: add the same calogValueNil(&value); before the key creation in pgQuery and sqliteQuery (or share one loop, see the duplication finding). OOM-gated, so ASan tests never see it.

Verifier: Verified in libs/calogDb.c: pgQuery (line 808) and sqliteQuery (line 1061) declare CalogValueT value; uninitialized, and the only initializer (pgColumn/sqliteColumn, which call calogValueNil(out) first) is skipped when calogValueString(&key,...) fails (src/value.c:469-472 returns calogErrOomE on malloc failure, nil-ing only key). The shared unwind blocks (lines 818-825 and 1071-1078) then call calogValueFree(&value) on indeterminate stack memory; calogValueFree (src/value.c:398-419) switches on the garbage type and can free()/calogAggFree()/calogFnRelease() a wild pointer. mysqlQuery is the proof of intent: line 646 does calogValueNil(&value) before building the key, exactly the guard missing from the other two copies. No comment documents this as intentional. Severity stays medium: the UB is real memory-unsafety, but the sole trigger is malloc failure mid-row (OOM-gated edge case), not reachable in normal use.

M6. [bug] libs/calogDb.c:1014 --- FIXED

Status: FIXED

String parameter and SQL lengths are narrowed from int64_t to int without a range guard, silently corrupting bindings for data >= 2 GiB.

Line 1014: rc = sqlite3_bind_text(stmt, index + 1, param->as.s.bytes, (int)param->as.s.length, SQLITE_TRANSIENT); -- a length in [2^31, 2^32) wraps negative, which sqlite treats as "NUL-terminated", so a binary-safe blob with embedded NULs binds silently truncated at the first NUL; a length just over 2^32 truncates to a small positive and binds a silent prefix. Same casts at line 993 (sqlite3_prepare_v2(db, sqlValue->as.s.bytes, (int)sqlValue->as.s.length, ...)) and pgRun line 894 (lengths[index] = (int)param->as.s.length; fed to PQexecParams as a binary-format length). The MySQL path is immune ((unsigned long) is 64-bit here). Given the project deliberately removed size limits (commit 6c64406) and CalogValueT lengths are int64_t, these should fail with calogErrRangeE instead of silently truncating (sqlite/libpq would reject such sizes anyway, but with the wrong/no error, or after binding wrong data).

Verifier: Verified in libs/calogDb.c: lines 1014, 993, and 894 cast int64_t string lengths to int with no range guard anywhere (calogValueString in src/value.c:458 imposes no upper bound, and calogDb.c has no INT_MAX checks or calogErrRangeE uses). For lengths in [2^31, 2^32) the cast wraps negative, which sqlite3_bind_text treats as NUL-terminated -- silently truncating binary data at the first embedded NUL, directly violating the binary-safety contract documented in the comment above sqlitePrepare (lines 986-987). Lengths over 2^32 bind a silent prefix. The pg path (line 894, binary format) has the same defect; the MySQL path's (unsigned long) cast is 64-bit here and immune, as the claim stated. No comment documents this as an intentional tradeoff.

M7. [bug] libs/calogExport.c:109 --- FIXED

Status: FIXED

Export name matching uses strcmp/strdup on length-carrying calog strings, so names with embedded NULs silently truncate and alias, unlike kv and pubsub which correctly match on (length, memcmp).

exportFindLocked does if (strcmp(gEntries[index].name, name) == 0) (line 109) and exportPublish stores nameCopy = strdup(name); (line 150), both ignoring args[0].as.s.length. Calog strings are explicitly length + bytes and may contain embedded NULs (calogValueString, src/value.c:458, copies arbitrary bytes and only appends a terminator). Concrete failure: a script calls calogExport("foo\0bar", fn) -- strdup stores just "foo", and a later calogExport("foo\0baz", g) takes the replace path at line 130 and clobbers the first export; calogCall("foo") then invokes a function exported under a different name. The sibling registries get this right: kvFindLocked uses gEntries[index].keyLen == length && memcmp(...) (calogKv.c:143) and pubsubPublish matches topicLen == ... && memcmp(...) (calogPubsub.c:126). Export should match and store by (bytes, length) the same way -- which also removes the split policy between the three libraries.

Verifier: Confirmed in libs/calogExport.c: exportFindLocked uses strcmp (line 109) and exportPublish uses strdup (line 150), with args[0].as.s.length never consulted anywhere in the file. Calog strings genuinely carry embedded NULs -- calogValueString (src/value.c:458) memcpys arbitrary bytes and records length, and engine bridges pass true byte lengths (lua_tolstring at luaAdapter.c:470, JS_ToCStringLen at jsAdapter.c:535) -- so a script can reach this with calogExport("foo\0bar", f): strdup stores "foo", a later calogExport("foo\0baz", g) strcmp-matches and takes the replace path (line 130), clobbering f, and calogCall("foo") invokes the wrong function. Neither the file-head comment nor calogExport.h documents a NUL-free-name restriction, while siblings kvFindLocked (calogKv.c:143) and pubsub (calogPubsub.c:126) correctly match on (length, memcmp), making export the inconsistent outlier. Mitigations: no memory unsafety (buffers are always NUL-terminated), the bare-name resolve path is unaffected (identifiers cannot contain NULs), and embedded-NUL names are exotic -- so a real edge-case bug plus a clear consistency cleanup, medium not high.

M8. [bug] libs/calogFs.c:243 --- FIXED

Status: FIXED

fsRead trusts st_size as the exact content length, so zero-size-but-nonempty files (/proc, /sys) silently read as empty.

total = (st.st_size > 0) ? (size_t)st.st_size : (size_t)0; followed by while (offset < total) -- for procfs/sysfs files st_size is 0, so the read loop never executes and fsRead("/proc/self/status") returns "" with calogOkE instead of the file contents. For an ops-oriented script broker this is a likely real-world call. The loop also never reads past st_size, so any special file whose stat size understates content is silently truncated. Fix: after consuming st_size bytes, keep read()ing into a growing buffer until read returns 0 (st_size then becomes just the initial allocation hint).

Verifier: fsReadNative (libs/calogFs.c:243-265) bounds its read loop strictly by fstat's st_size with no post-loop read to EOF; verified /proc/self/status and /proc/cpuinfo stat as size 0, so total=0, the loop never runs, and line 267 returns an empty string with calogOkE. The header contract (calogFs.h:5, "the whole file, binary-safe") promises full contents, and unlike fsRemove/fsStat, which document their tradeoffs in comments, no comment marks this as intentional. Not memory-unsafe and regular files are handled correctly, so it stays medium: a real silent-wrong-result bug limited to procfs/sysfs/FIFO-style files whose st_size understates content.

M9. [bug] libs/calogHttp.c:394 --- FIXED

Status: FIXED

No connect, read, or write timeout anywhere, so one stalled server hangs the calling engine thread forever.

connect(fd, rp->ai_addr, rp->ai_addrlen) (line 394), recv(conn->fd, buf, length, 0) (line 431), SSL_read/SSL_write, and the SSL_connect handshake all run on a default blocking socket with no SO_RCVTIMEO/SO_SNDTIMEO and no deadline. These natives are registered INLINE (calogRegisterInline, lines 89-90), so they execute on the engine context's thread: a server that accepts the connection and never responds (or a blackholed IP where connect waits for the full kernel SYN timeout) blocks the script -- and any calogContextClose/shutdown waiting on that context -- indefinitely, with no recourse from the script. Neither calogHttp.h nor the .c banner documents an intentional no-timeout tradeoff. Fix: setsockopt SO_RCVTIMEO/SO_SNDTIMEO after socket(), and use a non-blocking connect with poll() for a bounded connect.

Verifier: Verified in libs/calogHttp.c: line 394 connect(), line 431 recv(), SSL_read/SSL_write/SSL_connect all operate on a default blocking socket, and a project-wide grep confirms zero uses of SO_RCVTIMEO/SO_SNDTIMEO/O_NONBLOCK/poll in libs/ or src/. The natives are calogRegisterInline (lines 89-90), which per src/calog.h:145 run on the calling script thread, and calogContextClose (src/context.c:476) pthread_joins that thread, so a stalled peer blocks both the script and shutdown. The read-to-EOF design (httpReadResponse relies on the server honoring Connection: close) means a server that responds but never closes hangs recv/SSL_read unboundedly with no kernel timeout. The banners document other tradeoffs (INLINE, TLS verify default) but say nothing about timeouts, so this is not a documented intentional choice. Minor correction to the claim: the connect path is bounded by the kernel SYN retry timeout (~130s) rather than infinite, but the read/handshake paths are genuinely unbounded.

M10. [bug] libs/calogHttp.c:706 --- FIXED

Status: FIXED

httpParseStatus accumulates unbounded digits into int32_t, so a hostile status line causes signed integer overflow (UB).

code = code * 10 + (int32_t)(line[i] - '0'); runs with no digit-count or magnitude bound, so a server replying HTTP/1.1 99999999999 OK overflows int32_t -- undefined behavior per C11 (UBSan would trip on it), and in practice a garbage/negative status value handed to the script. Its sibling httpParseInt explicitly guards this exact pattern (if (value > (int64_t)HTTP_MAX_RESPONSE) { break; }, line 679), so the omission here is an oversight, not a tradeoff. Fix: stop after 3 digits (RFC 7230 status codes are exactly 3 digits) or bound code before multiplying.

Verifier: Confirmed in libs/calogHttp.c: httpParseStatus (line 706) accumulates an unbounded run of digits into int32_t with no cap; the status line comes straight from server-controlled network bytes (httpReadResponse -> httpBuildResult line 228, statusLen bounded only by the 64MB response cap), so 11+ digits cause signed int32 overflow (C11 UB, UBSan trap, garbage/negative status in practice). No comment documents a tradeoff, and sibling httpParseInt guards the identical pattern at line 679 (value > HTTP_MAX_RESPONSE break), confirming the omission is an oversight.

M11. [error-path] libs/calogHttp.c:883 --- FIXED

Status: FIXED

Socket/TLS read errors are indistinguishable from EOF, so a truncated response is returned to the script as a successful result.

httpReadResponse loops n = httpConnRead(conn, chunk, sizeof(chunk)); if (n <= 0) { break; } ... return calogOkE; -- a recv() return of -1 (e.g. ECONNRESET mid-body) and an SSL_read error/unclean TLS shutdown (httpConnRead line 424: if (r <= 0) { return 0; }) all terminate the loop exactly like clean EOF, and calogOkE is returned. Downstream, httpBuildResult even has the information to detect the loss but clamps it away: finalLen = ((size_t)contentLength < bodyRawLen) ? (size_t)contentLength : bodyRawLen; (line 312), so a body cut short of Content-Length is silently delivered as status-200 success. For https with verify=true this also means a MITM can undetectably truncate responses (no close_notify required), defeating part of what certificate verification buys. Fix: propagate n < 0 (and SSL_get_error != ZERO_RETURN) as a failure, and error when haveContentLength && bodyRawLen < contentLength.

Verifier: Verified in libs/calogHttp.c: httpReadResponse (883-894) breaks on n <= 0 and returns calogOkE, so recv() == -1 (only EINTR retried) is treated as clean EOF; httpConnRead (424-426) maps every SSL_read error/unclean shutdown to 0, discarding TLS truncation signals (no SSL_get_error/ZERO_RETURN check); and httpBuildResult line 312 clamps finalLen = min(contentLength, bodyRawLen) with no error when the body is shorter than Content-Length, so a truncated body is delivered as a successful status-200 result. The line-423 comment documents the mechanism only, not a truncation tradeoff, and the chunked path (httpDechunk) DOES error on truncation, showing the Content-Length path's blindness is an inconsistency rather than intent. The MITM point also holds: httpTlsHandshake does full cert+hostname verification, yet a bare TCP FIN mid-body yields undetected truncation.

M12. [duplication] libs/calogHttp.c:921 --- FIXED

Status: FIXED

httpRequestNative repeats the same 9-line option-lookup block five times (url, method, headers, body, insecure).

Lines 920-977 repeat the identical dance five times: status = calogValueString(&keyValue, "url", 3); if (status != calogOkE) { return calogFail(result, status, "httpRequest: out of memory"); } urlValue = calogAggGet(opts, &keyValue); calogValueFree(&keyValue); -- only the key literal, its hardcoded length (3/6/7/4/8, each a second source of truth for strlen of the adjacent literal), and the destination pointer change. A single helper, e.g. static CalogValueT *httpOptGet(CalogAggT *opts, const char *name) that builds the key with strlen(name), does the get, frees the key, and returns the value (NULL on OOM/missing), collapses ~50 lines to 5 calls and removes the duplicated error handling. This is exactly the group-similar-functionality cleanup the codebase owner asks for.

Verifier: Verified: libs/calogHttp.c lines 920-979 repeat the identical build-key/OOM-check/calogAggGet/calogValueFree block five times, differing only in key literal, hardcoded length (3/6/7/4/8 -- each a second source of truth for the literal), and destination. No comment documents it as intentional. The same file's own helpers httpMapSetAgg/Int/Str (lines 605-663), plus netMapSet* in calogNet.c and sshMapSet* in calogSsh.c, already use (int64_t)strlen(key), so httpRequestNative is the lone deviation from the codebase's established name-based-helper pattern. One caveat: the claim's proposed helper (NULL on OOM/missing) would silently conflate OOM with absent-key; a behavior-preserving version needs a status out-param (e.g. CalogValueT *httpOptGet(CalogAggT *opts, const char *name, int32_t *status)). Not a runtime bug -- current code is functionally correct -- but a concrete cleanup collapsing ~50 lines to ~15 and removing five hardcoded lengths.

M13. [bug] libs/calogJson.c:166 --- FIXED

Status: FIXED

Hybrid aggregate serialization can emit duplicate JSON keys, so data IS silently dropped on round-trip despite the comment claiming otherwise.

In jsonEncodeAgg, array elements of a hybrid are emitted as n = snprintf(tmp, sizeof(tmp), "\"%lld\":", (long long)index); alongside the real pairs. If a hybrid also has a pair keyed "0" (or integer key 0, which jsonEncodeKey renders identically as ""0""), the output is {"0":elem,"0":pairValue} -- duplicate keys. jsonParse of that output calls calogAggSet twice with equal keys, and calogAggSet (src/value.c) replaces the first value, so the array element is lost. This contradicts the comment at lines 146-148 ('so nothing is silently dropped'). Cheap fix options: prefix index keys (or error) when a colliding pair key exists.

Verifier: Verified end-to-end: jsonEncodeAgg (libs/calogJson.c:166) emits hybrid array elements under synthesized keys "0","1",... with no collision check against real pair keys, and jsonEncodeKey (line 228) renders integer key 0 identically to index 0; Lua ingress (src/lua/luaAdapter.c:481-553) produces exactly such hybrids (e.g. t={10,20}; t[0]="zero" -> {"0":10,"1":20,"0":"zero"}); on re-parse, jsonParseObject feeds both members through calogAggSet, which replaces on calogValueEquals string match (src/value.c:158-164, 384-388), silently dropping the array element and contradicting the comment at libs/calogJson.c:146-148 that says nothing is silently dropped.

M14. [duplication] libs/calogNet.c:163 --- FIXED

Status: FIXED

netMapSetInt/netMapSetStr are the third verbatim copy of the same two map-helper functions, duplicated per-library instead of shared.

netMapSetInt (calogNet.c:163) and netMapSetStr (calogNet.c:182) are byte-for-byte identical in logic to httpMapSetInt/httpMapSetStr (libs/calogHttp.c:625/643) and sshMapSetInt/sshMapSetStr (libs/calogSsh.c:798/817) -- same signatures, same key-build/set/free-on-failure sequence, only the function-name prefix differs. Every new library that returns a map will grow a fourth copy, and a bug fix in the ownership-on-failure logic (which is subtle: key freed on scalar-set failure, key+value freed on string-set failure) must be applied in three places or they drift. These belong in one shared translation unit alongside calogHandle.c, e.g. calogMapSetInt/calogMapSetStr in a small calogLibUtil.c, with the three per-library copies deleted.

Verifier: Verified byte-for-byte: netMapSetInt/netMapSetStr (calogNet.c:163/182), httpMapSetInt/httpMapSetStr (calogHttp.c:625/643), and sshMapSetInt/sshMapSetStr (calogSsh.c:798/817) have identical bodies differing only in name prefix, and a grep of all libs confirms exactly three copies. This is ~120 lines of duplicated subtle ownership-on-failure logic (key freed on scalar-set failure; key+value freed on string-set failure), not trivial boilerplate, with no comment documenting it as intentional. The proposed shared-TU fix is proven feasible by the existing architecture: calogHandle.o is already a shared per-library object linked into testNet, testSsh, testDb, and the calog CLI, so calogMapSetInt/calogMapSetStr in a sibling shared TU drops in with no build changes beyond one object file.

M15. [error-path] libs/calogNet.c:781 --- FIXED

Status: FIXED

enetService consumes an ENet event before creating the result map; if calogAggCreate fails, a RECEIVE packet leaks and a DISCONNECT leaves a stale handle that will alias the next connection reusing that peer slot.

At line 777 serviced = enet_host_service(host, &event, ...) dequeues the event, and only then does line 781 status = calogAggCreate(&map, calogMapE); if (status != calogOkE) { return calogFail(result, status, "enetService: out of memory"); }. On that failure return: (1) for ENET_EVENT_TYPE_RECEIVE, event.packet is never passed to enet_packet_destroy -- a straight packet leak (the OOM branch at lines 800-802 handles exactly this for the later add-handle failure, proving the obligation is known); (2) for ENET_EVENT_TYPE_DISCONNECT, the cleanup at lines 825-826 (calogHandleRemove(lib->handles, peerHandle, NET_TYPE_ENET_PEER); event.peer->data = NULL;) is skipped. By the time ENet delivers a DISCONNECT event it has already run enet_peer_reset on the peer (verified in vendor/enet/protocol.c), and enet_peer_reset does NOT clear peer->data (verified in vendor/enet/peer.c line 385ff), so the slot is immediately reusable while still carrying the dead handle. The next incoming connection that reuses the slot is reported by enetService line 796 under the DEAD peer's handle (peerHandle = (int64_t)(intptr_t)event.peer->data; is nonzero), and the stale handle still resolves in the table to the reused ENetPeer -- the script ends up talking to a different remote under an old handle. Fix: create the map (or at least perform the packet-destroy/handle-remove/data-clear obligations) before returning on this failure, e.g. build the map before servicing or add the same cleanup done at lines 800-806 plus the disconnect cleanup to this branch.

Verifier: Verified in libs/calogNet.c: enet_host_service (line 777) dequeues the event before calogAggCreate (line 781), and the failure return at 782-783 performs none of the event-cleanup obligations the function itself demonstrates elsewhere (packet destroy at 800-802/820, disconnect handle-remove/data-clear at 825-826). calogAggCreate (src/value.c:90) fails only on calloc OOM. Vendored ENet confirms both consequences: protocol.c calls enet_peer_reset while delivering DISCONNECT (ZOMBIE dispatch case and notify_disconnect), making the slot reusable, and grep shows ENet never touches peer->data in peer.c/protocol.c/host.c, so the stale handle survives in both peer->data and the handle table. A reused slot's next connection is then reported under the dead handle (line 796-797 skips handle allocation for nonzero data) and the old handle resolves to a peer connected to a different remote. RECEIVE packets dequeued into event.packet leak outright on this path.

M16. [duplication] libs/calogSsh.c:164 --- FIXED

Status: FIXED

sftpGet's read-to-EOF grow loop (lines 161-199) is a near-verbatim copy of sshDrain (lines 644-679) and should be one shared drain routine.

Both loops are: if (length == cap) { if (cap >= SSH_MAX_TRANSFER) {fail range} want = (cap == 0) ? N : cap * CALOG_GROWTH_FACTOR; ... realloc ... } then read / n > 0 accumulate / n == 0 break / EAGAIN continue / else fail. The only real differences are the read call (libssh2_sftp_read vs libssh2_channel_read_ex) and the initial capacity (65536 vs 4096 -- which have already drifted). A shared helper taking a read callback (or the two concrete wrappers calling a common grow-and-read core) removes ~35 duplicated lines and gives the cap/growth policy a single home. Related nit it would also unify: sftpGet line 201 guards (buffer != NULL) ? buffer : "" while sshExec lines 758/760 pass possibly-NULL drain buffers straight to sshMapSetStr -- both are fine (src/value.c:465 calogValueString only rejects NULL when length > 0), so the ternary is an unnecessary conditional.

Verifier: Verified in libs/calogSsh.c: sftpGet lines 164-199 and sshDrain lines 647-679 are structurally identical grow-and-read loops (same SSH_MAX_TRANSFER range check, same (cap==0)?N:cap*CALOG_GROWTH_FACTOR growth with clamp, same realloc/OOM path, same n>0/n==0/EAGAIN/else dispatch), differing only in the read call (libssh2_sftp_read vs libssh2_channel_read_ex) and unnamed initial capacities (65536 vs 4096) that have already drifted with no comment documenting intent. A shared drain core with a read callback and initialCap parameter is feasible since sshExec already wraps sshDrain's bare status codes in calogFail, so sftpGet can do the same and close the file at its level. The related nit is also confirmed: src/value.c calogValueString rejects NULL only when length > 0, and sshExec passes possibly-NULL drain buffers through sshMapSetStr unguarded, making sftpGet line 201's (buffer != NULL) ? buffer : "" an unnecessary conditional. Cleanup with clear payoff, not a runtime bug.

M17. [multiple-truth] libs/calogSsh.c:778 --- FIXED

Status: FIXED

sshMapSetBool/sshMapSetInt/sshMapSetStr (lines 778-837) are the third per-library copy of the same map-building helpers, already duplicated as httpMapSetInt/Str and netMapSetInt/Str.

libs/calogSsh.c:778/798/817 vs libs/calogHttp.c:625/643 (httpMapSetInt, httpMapSetStr) vs libs/calogNet.c:163/182 (netMapSetInt, netMapSetStr) -- identical bodies (build key string, build value, calogAggSet, free on failure) maintained in three files that can and already do drift (only the ssh copy has a Bool variant; the http copy grew its own NULL-guard convention at calogHttp.c:332). These belong in one shared internal helper header/TU (e.g. next to calogHandle.c) per the stated goal of grouping similar functionality; every future lib returning a map will otherwise mint a fourth copy.

Verifier: Verified byte-identical bodies: sshMapSetInt/Str (calogSsh.c:798/817) == httpMapSetInt/Str (calogHttp.c:625/643) == netMapSetInt/Str (calogNet.c:163/182), all hand-maintaining the same subtle calogAggSet failure-path ownership rules. Drift already exists (Bool variant only in ssh, Agg variant with the only ownership-contract comment only in http), plus a fourth variant the claim missed: fsMapSet (calogFs.c:137) implements the same job with the OPPOSITE ownership convention (frees the caller's value on failure). A shared lib-internal TU precedent already exists (calogHandle.c, included by db/net/task/ssh), so consolidation is natural. No runtime bug today -- all copies' error paths are currently correct -- so this is a cleanup, not a defect, but one with concrete payoff since this diff itself mints the third copy and more map-returning libs are in progress.

M18. [bug] libs/calogTime.c:76 --- FIXED

Status: FIXED

timeSleep converts an unbounded double to int64_t, which is undefined behavior for large arguments.

nanos = (int64_t)(ms * 1000000.0); -- ms is validated only as !(ms >= 0.0) (catches NaN and negatives), but not against an upper bound. A script calling timeSleep(1e19) (or any ms > ~9.22e12, i.e. INT64_MAX nanoseconds) makes ms * 1000000.0 exceed INT64_MAX, and C11 6.3.1.4 makes the conversion undefined behavior -- UBSan (which this project runs, Makefile line 22 -fsanitize=address,undefined) aborts on it; in a plain build the typical x86 result is INT64_MIN, producing a negative tv_sec/tv_nsec and a misleading 'nanosleep failed' error. Fix: reject or clamp ms above (double)INT64_MAX / 1000000.0 before the multiply.

Verifier: libs/calogTime.c:73-76 confirms it: the only validation is !(ms >= 0.0) (rejects NaN/negatives only), then nanos = (int64_t)(ms * 1000000.0) with script-controlled ms. Any ms > ~9.22e12 (or +Infinity, which passes the check since Inf >= 0.0) makes the double-to-int64_t cast UB per C11 6.3.1.4p1. Makefile line 22 builds with -fsanitize=address,undefined, so UBSan reports float-cast-overflow; the unsanitized release build gets INT64_MIN, negative timespec fields, and a misleading 'nanosleep failed' (calogErrUnsupportedE) instead of a range error. No guard exists elsewhere and no comment documents it as intentional. Minor overstatement in the claim: UBSan reports and continues by default rather than aborting, unless recover is disabled.

M19. [race] libs/calogTimer.c:255 --- FIXED

Status: FIXED

timerAfter/timerEvery called after the last calogTimerShutdown silently restarts the background thread, which is never joined and can fire callbacks after calogDestroy, dereferencing the freed broker.

calogTimerShutdown resets gThreadStarted = false; gShutdown = false; (lines 121-122) with gRefCount now 0, but timerSchedule (line 242 onward) never checks gRefCount or a retired flag: it calls status = timerEnsureThreadLocked(); (line 255), which sees gThreadStarted == false and starts a brand-new thread + cond (lines 161-175). The whole static-state design exists precisely because natives stay callable between lib shutdown and calogDestroy -- kvKv/pubsub/export headers all promise "a post-shutdown native call simply sees an empty store" and behave that way, but timer instead re-arms. Concrete failure: a still-live script calls timerAfter(1, fn) after calogTimerShutdown (the contractually required order is shutdown BEFORE calogDestroy, per this file's own header lines 10-11); nobody will ever join the new thread (refCount is already 0), leaking the thread, the cond, and the retained callback -- and when the timer fires after calogDestroy, calogFnInvoke (src/value.c:242) dereferences callable->runtime->invokeHook on the freed CalogT (calogFnMarkDead is never called anywhere, so the alive early-out at value.c:237 does not save it). Fix: have timerSchedule fail (or no-op like the other libs) when gRefCount == 0 / after the final shutdown, instead of restarting the thread.

Verifier: Confirmed in code: calogTimerShutdown resets gThreadStarted/gShutdown to false at libs/calogTimer.c:121-122 with gRefCount at 0, and timerSchedule (line 242+) has no gRefCount/retired guard -- line 255 calls timerEnsureThreadLocked, which restarts the cond+thread (lines 161-175) on any post-shutdown timerAfter/timerEvery. No future shutdown call exists to join it (calogMain.c calls calogTimerShutdown exactly once, line 524), so the thread, cond, and retained callback leak. The UAF chain also verifies: calogFnMarkDead (src/value.c:249) has zero callers, so the alive early-out at value.c:237 never trips, and calogFnInvoke dereferences callable->runtime->invokeHook (value.c:242) on the CalogT freed by calogDestroy (src/context.c:317-323). The post-shutdown-callable window is contractual (calogTimer.h requires shutdown before calogDestroy while contexts are alive; calogTimer.c lines 8-13 and calogKv.c lines 10-11 document that natives stay callable and should degrade benignly -- timer is the one lib that re-arms instead). Reachable even in the shipped CLI via fire-and-forget task contexts, which run until calogDestroy (calogMain.c:526) and are not in the liveCount loop. Kept at medium: memory-unsafe outcome, but only in the narrow teardown-window ordering; the guaranteed effect is a process-end leak.

M20. [leak] src/berry/berryAdapter.c:183 --- FIXED

Status: FIXED

Every exported Berry function permanently consumes a new hidden-global slot (_calog_fn_%d with monotonically increasing nextRef); release only nils the value, so the global name table grows without bound in a long-lived context.

berryExportValue does snprintf(export->refName, sizeof(export->refName), "_calog_fn_%d", context->nextRef); context->nextRef++; then be_setglobal(vm, export->refName). Berry's be_global_new (vendor/berry/src/be_var.c:117) registers the name string + vtab entry + global slot permanently -- there is no removal API, and berryCallableRelease (lines 124-126) only assigns nil to the slot. A long-lived Berry context that repeatedly passes lambdas out to natives (each crossing creates a fresh export via berryToValue line 435) accumulates one never-reclaimed global entry per crossing. Secondary: nextRef is int32_t, so after 2^31 exports the name wraps and a released slot's name could alias a still-live pin, unpinning it. Fix: keep a free-list of released refNames and reuse them, which bounds the table at the peak number of simultaneously live exports and removes the wrap hazard.

Verifier: Verified in berryAdapter.c: berryExportValue (lines 183-187) mints a fresh calog_fn%d global per export from a monotonic nextRef with no reuse, berryToValue's be_isfunction branch creates a new export on every crossing, and berryCallableRelease (lines 124-126) only nils the slot. Vendored Berry's be_global_new (be_var.c:117) permanently registers the name in the vtab map plus a vlist slot with no removal API (be_global_undef is internal, keeps the vtab entry, and is unused by calog), so each function crossing permanently grows the global table in a long-lived context. The proposed refName free-list fix is sound since be_global_new reuses the slot for a known name. The secondary int32 wrap-alias hazard is effectively unreachable (memory exhausts long before 2^31 exports).

M21. [bug] src/berry/berryAdapter.c:317 --- FIXED

Status: FIXED

berryFromValue discards the status of every recursive marshal inside aggregates (lines 317, 331, 339), and berryForeignCall discards the result-marshal status (line 274), so depth-capped or unmarshalable values silently become nil with calogOkE reported.

berryFromValue(context, &aggregate->array[index], depth + 1); at line 317 (and the identical calls at 331 and 339) ignore the int32_t return. Every failure path in berryFromValue pushes nil, so the stack stays balanced, but a nested value deeper than CALOG_MAX_DEPTH (or a failed calogFnE track) is silently replaced by nil and the top-level call still returns calogOkE -- silent data corruption, whereas the JS adapter propagates the same failure as a RangeError (jsFromValue lines 381-397). Likewise berryForeignCall line 274 berryFromValue(context, &result, 0); ignores the status and be_return's whatever was pushed (nil on failure), while the sibling berryTrampoline DOES check it and raises type_error (lines 597-603). The aggregate loops should propagate the child status (freeing the container is unnecessary -- Berry GC owns it -- just unwind to base and return the error), and berryForeignCall should mirror berryTrampoline.

Verifier: Verified in src/berry/berryAdapter.c: the three recursive berryFromValue calls in the calogAggE case (lines 317, 331, 339) and the result marshal in berryForeignCall (line 274) all discard the int32_t status, while berryTrampoline (line 597) and berryCallableInvoke (line 97) check the same function and raise/propagate. Every berryFromValue failure path pushes nil, so a depth-capped (>CALOG_MAX_DEPTH=64), OOM-tracked-fn, or unmarshalable nested value is silently replaced by nil with calogOkE reported at line 353. Not an intentional tradeoff: the file header (lines 14-15) documents failures as raised exceptions, and unlike s7/wren (whose fromValue signatures return s7_pointer/void and cannot report errors by design), berryFromValue has a status return that is checked elsewhere in the same file; the JS adapter (jsAdapter.c lines 381-397) propagates the identical child failure. The berryForeignCall-vs-berryTrampoline inconsistency in the same file is the clincher.

M22. [duplication] src/calogMain.c:397 --- FIXED

Status: FIXED

main() repeats the same teardown block (free sources loop + free(engines) + free(sources) [+ calogDestroy] + return 1) five times.

Near-identical cleanup appears at lines 397-402 (malloc fail), 407-414 (resolveArg fail, with an undo partial loop), 418-426 (calogCreate fail), 444-452 (register fail, plus calogDestroy), and 462-471 (gLaunched calloc fail, plus calogDestroy). Three of the five are byte-for-byte the same free-loop/free/free/return sequence. A single goto fail; cleanup label (freeing sources[0..resolvedCount), engines, sources, and destroying calog when non-NULL) collapses all five and removes the risk of a future error path forgetting one of the frees -- exactly the grouped-cleanup pattern the rest of this codebase uses.

Verifier: Verified in src/calogMain.c: five teardown blocks at lines 397-402, 407-414, 418-426, 444-452, and 462-471, exactly as claimed. Blocks 3-5 share a byte-identical free-loop/free(engines)/free(sources)/return 1 sequence, blocks 4-5 additionally prepend calogDestroy, and block 2 differs only in loop bound (partial undo). A single goto-fail label with a resolvedCount and NULL-initialized calog collapses all five; the codebase already uses this pattern (goto cleanupArgs in src/mybasic/mybasicAdapter.c:569-607) and no comment documents the repetition as intentional. No current leak or bug, so it is a duplication cleanup, not a correctness defect.

M23. [error-path] src/calogMain.c:476 --- FIXED

Status: FIXED

A script that fails to launch (calogContextOpen NULL or calogContextEval error) prints a diagnostic but never sets gExitCode, so calog can exit 0 having run nothing.

In the launch loop: if (context == NULL) { fprintf(stderr, "calog: %s: failed to open a context\n", ...); } else if (calogContextEval(...) != calogOkE) { ...; calogContextClose(context); } -- neither branch touches gExitCode. Concrete failure: calog foo.lua && deploy where the Lua context fails to open -> gLaunchedCount stays 0, liveCount==0 sets gShutdown at line 494, and main returns gExitCode which is still 0. The error message goes to stderr but the process reports SUCCESS, so shell automation proceeds. This is inconsistent with the other two failure paths: resolveArg failure returns 1 (line 413), and a runtime script error forces gExitCode to 1 via the CAS at line 509. The launch-failure branches should force gExitCode to 1 the same way.

Verifier: Confirmed in src/calogMain.c: the launch loop's two failure branches (lines 476-480) only fprintf to stderr; gExitCode (init 0, line 73) is never set. Failed launches are not added to gLaunched (line 485 increments only on success), so onError's failed-flag lookup and the pump-loop CAS to gExitCode=1 (line 509) can never apply to them; calogContextEval failure is a synchronous enqueue/OOM return (context.c:670-693), not routed through the error handler. With zero successful launches, liveCount==0 sets gShutdown at line 494, the pump loop is skipped, and main returns 0 at line 533 -- success reported after running nothing, contradicting the sibling paths (resolveArg failure returns 1 at line 413 under a fail-fast comment; runtime errors force exit 1 at line 509). No comment documents exit-0-on-launch-failure as intentional. Reachability is limited to OOM, context-cap, or pthread_create failure (context.c:328-391), so it is an edge-case bug, not a normal-use one.

M24. [error-path] src/context.c:227 --- FIXED

Status: FIXED

Shutdown signaling depends on a heap allocation: if the calloc for the SHUTDOWN message fails, the message is silently skipped but pthread_join is still called, hanging calogDestroy/calogContextClose forever.

calogActorShutdown lines 227-231: message = (MessageT *)calloc(1, sizeof(*message)); if (message != NULL) { message->kind = messageShutdownE; enqueueRaw(context, message); } -- on allocation failure no shutdown is delivered, yet line 240 unconditionally does pthread_join(context->thread, NULL); the context thread sits in messageDequeue's pthread_cond_wait (line 907) forever, so shutdown deadlocks. calogContextClose lines 486-491 have the identical pattern. Shutdown should not be able to fail under OOM: either preallocate the shutdown message per context, or signal via an allocation-free flag -- e.g. a shutdownRequested bool set under queueMutex plus pthread_cond_signal, which messageDequeue already effectively supports via its !context->shuttingDown wait predicate (all shuttingDown writes would need to move under queueMutex, which also removes the currently-unlocked writes at lines 1027 and 1087).

Verifier: Confirmed in src/context.c: calogActorShutdown (227-231) and calogContextClose (486-490) both skip enqueueing messageShutdownE when calloc fails, then unconditionally pthread_join (240, 491). The worker's only exit paths are message-driven: shuttingDown is set solely at lines 1027 and 1086 upon receiving the shutdown message, and messageDequeue's wait predicate (line 906, cond_wait at 907) blocks while head==NULL && !shuttingDown. With no message enqueued and the host blocked in join, no producer remains, so the join hangs forever. Not a documented tradeoff -- the codebase does document intentional OOM leniency elsewhere (registryFreePush, line 1033 comment) but has no such comment here.

M25. [leak] src/context.c:275 --- FIXED

Status: FIXED

calogActorShutdown drains the host queue with messageFree, leaking any pending RELEASE callable; contextDrainQueue exists for exactly this and is not used.

Line 275-277: while ((message = tryDequeue(calog->hostContext)) != NULL) { messageFree(message); }. messageFree (line 924) frees args/source/result by kind but never touches message->callable, so a pending messageReleaseE loses its CalogFnT (struct + userData, per calogFnFinalize in value.c:205). This path is hit in ordinary single-runtime use: during the join phase (lines 236-242) each script interpreter is destroyed and drops its retained host-owned callables (calogFnFromNative, owner CALOG_HOST_ID); actorReleaseCallable -> contextPostRelease enqueues messageReleaseE onto the host queue, which is then drained AFTER the joins with plain messageFree -- the callable is never finalized. The script-context drain (contextDrainQueue, line 615-622) explicitly finalizes orphaned releases for this exact race, and its comment even calls out cross-context releases during teardown; the host queue just doesn't get the same treatment. Fix: replace the loop at line 275 with contextDrainQueue(calog->hostContext); -- it is a CalogContextT* and the function does precisely the right thing, also removing the duplicated drain logic.

Verifier: Confirmed in code: context.c:275-277 drains the host queue with messageFree, which (lines 924-943) frees args/source/result by kind but never message->callable, so a pending messageReleaseE leaks its CalogFnT. The path is reachable: during calogActorShutdown's join phase, script threads run destroyInterpreter (context.c:1142-1144) whose adapters calogFnRelease retained host-injected callables (berryAdapter.c:139, luaAdapter.c:314, jsAdapter.c:345, etc.); when that is the last ref, actorReleaseCallable (context.c:187-204) on the script thread posts a messageReleaseE to CALOG_HOST_ID, which registryResolveLocked (1056) resolves to hostContext. Nothing pumps the host queue after the joins, so line 275 frees the shell without calogFnFinalize. contextDrainQueue (608-623) exists for exactly this, its comment documents the orphaned-release case, hostContext is a CalogContextT*, and line 255 already uses it for script contexts -- the one-line fix is valid. Minor overstatement in the claim: calogFnFromNative callables have release == NULL, so calogFnFinalize frees only the CalogFnT struct (value.c:226), not userData; the leak is ~sizeof(CalogFnT) per callable. Severity adjusted to medium: a real, reachable leak, but a small bounded allocation at teardown -- no memory-unsafety, data loss, or deadlock; it only accrues across repeated runtime create/destroy (hot reload) and dirties ASan runs.

M26. [leak] src/context.c:615 --- FIXED

Status: FIXED

context->stash is never freed at teardown: contextDrainQueue drains only head/tail, so an orphaned stashed REPLY (message + deep-copied result value) leaks when the context is freed.

contextDrainQueue (line 615) loops on tryDequeue, which walks context->head only; context->stash (populated at pumpUntil line 1007-1008 with replies whose token does not match the awaited one) is never visited, and free(context) at lines 258/503 and free(calog->hostContext) at line 280 drop it on the floor. Concrete path: nested in-flight calls when shutdown arrives -- inner pumpUntil frame (token 2) exits via the shutdown-error path (line 996-998, queue empty); the reply for token 2 then arrives while the outer frame (token 1) is still pumping, gets stashed at line 1007 (token mismatch), and no live frame will ever claim it; the outer frame exits, serveLoop never touches the stash, the thread ends, and calogContextClose/calogActorShutdown free the context with the stash chain still allocated -- each entry a MessageT plus a deep-copied CalogValueT result. Fix: at the end of contextDrainQueue, walk context->stash calling messageFree on each node (kind is messageReplyE, so the result is freed) and null it.

Verifier: Confirmed in src/context.c: context->stash is touched only inside pumpUntil (claim at 976-993, push at 1007-1008); contextDrainQueue (608-623) drains only head/tail via tryDequeue (1107-1123), and all three teardown paths (calogContextClose 500-503, actor shutdown 255-258, host teardown 275-280) free the context without walking stash. The orphan path is reachable: pumpUntil abandons its token only on the shutdown-empty-queue path (996-998), nested pumps exist via contextDispatchCall re-entrancy (1011-1013) plus contextSendBlocking (816), and replies arrive asynchronously on the caller's queue via contextReply (786), so a late reply for an abandoned inner token gets stashed by the still-active outer frame and is never claimed; each stash node is a messageReplyE MessageT holding a deep-copied CalogValueT result that only messageFree (939-941) would release. The drain comment (611-614) shows intent to free every pending message at teardown, so this is an oversight, not a documented tradeoff. Fix as claimed: walk and messageFree the stash in contextDrainQueue.

M27. [multiple-truth] src/js/jsAdapter.c:374 --- FIXED

Status: FIXED

The keyed-aggregate egress policy pairCount > 0 || kind == calogMapE is hand-copied into six adapters (once inverted), so a policy change (e.g. calogBothE handling) must be edited in six places.

The same predicate deciding 'does this aggregate materialize as a map/object/table or a list/array' is duplicated: jsAdapter.c:374 asObject = aggregate->pairCount > 0 || aggregate->kind == calogMapE;, squirrelAdapter.c:383 asTable = aggregate->pairCount > 0 || aggregate->kind == calogMapE;, berryAdapter.c:312 if (aggregate->pairCount > 0 || aggregate->kind == calogMapE) {, s7Adapter.c:374 same expression, wrenAdapter.c:444 same expression, and mybasicAdapter.c:365 the DE MORGAN inverse asList = (aggregate->pairCount == 0 && aggregate->kind != calogMapE);. Six independent copies of one semantic fact is a textbook multiple-sources-of-truth: if the broker ever adds a rule (e.g. treat calogBothE arrayCount==0 specially, or honor kind==calogListE with stray pairs differently), one adapter will inevitably drift -- the mybasic inversion already shows the copy mutating in transit. Add a one-line inline predicate next to CalogAggT (e.g. static inline bool calogAggIsKeyed(const CalogAggT *a)) in src/calog.h and use it in all six.

Verifier: Grep confirms the keyed-egress predicate is duplicated exactly as claimed: pairCount > 0 || kind == calogMapE at jsAdapter.c:374, squirrelAdapter.c:383, berryAdapter.c:312, s7Adapter.c:374, wrenAdapter.c:444, plus the De Morgan inverse at mybasicAdapter.c:365. No shared helper exists (no calogAggIsKeyed anywhere), calog.h defines CalogAggT/CalogKindE without any keyed predicate, and value.c never canonicalizes kind after creation, so these six inline expressions are the sole encoding of the list-vs-map materialization policy. The policy is nontrivial (calogBothE exists in the enum and is handled only implicitly), so a rule change requires six coordinated edits in two textual forms. All copies currently agree, so it is a maintainability finding, not a live bug -- but it matches the codebase owner's explicit multiple-sources-of-truth criterion and the fix is a one-line static inline in calog.h.

M28. [bug] src/js/jsAdapter.c:623 --- FIXED

Status: FIXED

Object-key marshalling uses JS_AtomToCString + strlen, truncating any property key containing an embedded NUL and colliding distinct keys, breaking the documented binary-safe string guarantee.

In jsToValue's object branch: keyStr = JS_AtomToCString(ctx, tab[index].atom); (line 623) then status = calogValueString(&keyValue, keyStr, (int64_t)strlen(keyStr)); (line 629). QuickJS encodes U+0000 as a literal 0x00 byte, so a JS object {"a\0b": 1, "a\0c": 2} marshals both keys as "a" -- the second silently overwrites the first in the CalogAggT map. This is asymmetric with the rest of the file, which is careful to be binary-safe: string values use JS_ToCStringLen (line 535), keys going OUT use JS_NewAtomLen with the exact length (line 401), and jsResolveOwnProperty already uses JS_AtomToCStringLen (line 466). Fix: JS_AtomToCStringLen(ctx, &len, tab[index].atom) and pass len.

Verifier: jsAdapter.c:623-629 confirmed: JS_AtomToCString + strlen(keyStr) truncates keys at the first NUL. QuickJS-ng's utf8_encode (cutils.h) emits U+0000 as a literal 0x00 byte and the narrow-string fast path returns the raw buffer, so a key like "a\0b" yields bytes 61 00 62 and strlen cuts it to "a"; distinct keys collide and calogAggSet silently overwrites. design.md (lines 50, 58, 620, 838) documents strings as length-prefixed binary-safe, and line 165 explicitly treats bare-strlen NUL truncation as a defect class; no comment marks this an intentional exception. The rest of the file is binary-safe (JS_ToCStringLen at 535, JS_NewAtomLen at 401, JS_AtomToCStringLen at 466), and JS_AtomToCStringLen exists in the vendored quickjs.h:625, so the proposed fix is valid.

M29. [duplication] src/lua/luaAdapter.c:216 --- FIXED

Status: FIXED

The BindingT struct, expose bookkeeping (strdup + growth block), and destroy free-loop are duplicated verbatim between the Lua and Squirrel adapters (and a third copy exists in mybasicAdapter.c).

luaAdapter.c:30-33/216-256/159-163 and squirrelAdapter.c:30-33/219-262/149-153 contain identical code: typedef struct BindingT { CalogT *broker; char *name; }, the calogLookup guard, malloc+strdup with the same error unwinding, the identical capacity-growth block (newCap = (cap == 0) ? *_BINDING_INITIAL : cap * CALOG_GROWTH_FACTOR; realloc; free name/binding on failure), and the identical destroy loop freeing name+binding. Only the final 4-6 lines (installing the closure into the VM) differ. The duplicated constants LUA_BINDING_INITIAL (luaAdapter.c:21) and SQUIRREL_BINDING_INITIAL (squirrelAdapter.c:22), both 8, are a minor multiple-sources-of-truth on the same fact. A shared binding-registry helper in calogInternal (create/append/destroy) would collapse all copies; grep shows mybasicAdapter.c carries a third BindingT.

Verifier: Diffing luaAdapter.c:216-250 against squirrelAdapter.c:219-253 shows the expose bookkeeping is byte-identical except for the signature, VM-handle variable, and the two duplicated initial-capacity constants (both 8); the BindingT structs, their comments, and the destroy free-loops (lua 159-163, squirrel 149-153) are also identical, only the 3-6 VM-install lines differ, and no shared helper exists in calogInternal.h nor any comment marking the duplication intentional. The 'third copy in mybasicAdapter.c' is overstated: its BindingT is a single const char* in a fixed 256-slot bank with no strdup/growth/free-loop, so only the struct name coincides. Still, two verbatim ~45-line blocks whose OOM-unwind order and growth policy must stay in sync are a real, mechanically extractable cleanup confined to exactly two of seven adapters.

M30. [duplication] src/lua/luaAdapter.c:259 --- FIXED

Status: FIXED

Four near-identical marshal-dispatch-cleanup trampolines: luaFunctionCall vs luaTrampoline, and squirrelForeignCall vs squirrelTrampoline, should each collapse into one shared routine.

luaAdapter.c luaFunctionCall (259-307) and luaTrampoline (572-620) are line-for-line the same pipeline -- calloc args array, per-arg luaToValueDepth with cleanup loop, dispatch, free args, error-string push (identical (result.type == calogStringE) ? result.as.s.bytes : ... + lua_pushlstring + lua_error block), luaPushValueDepth of result -- differing only in the arg base index (2 vs 1) and the dispatch call (calogFnInvoke(callable,...) vs calogCall(binding->broker, binding->name,...)). squirrelAdapter.c squirrelForeignCall (268-321) and squirrelTrampoline (618-674) repeat the same duplication and there even the arg base (index + 2) is identical. A per-adapter helper taking (argBase, dispatch-callback) removes ~90 duplicated lines across the two files in scope.

Verifier: Verified in both files: luaFunctionCall (luaAdapter.c:259-307) and luaTrampoline (572-620) share an identical ~35-line pipeline (calloc args, marshal loop with partial-cleanup, dispatch, free loop, identical calogStringE error-ternary + lua_pushlstring + lua_error, luaPushValueDepth) differing only in callable source, arg base (2 vs 1), dispatch call, and message noun. squirrelForeignCall (squirrelAdapter.c:268-321) and squirrelTrampoline (618-674) duplicate the same pipeline with even the arg base/count logic identical. No comment documents it as intentional; a per-adapter helper with a dispatch callback is feasible and removes ~80-90 lines of memory-management-sensitive duplication where a fix to one copy could miss the other.

M31. [duplication] src/lua/luaAdapter.c:572 --- PARTIAL

Status: PARTIAL -- each adapter's own trampoline-vs-foreign-call pair was collapsed (per-adapter dedup, done). The full cross-7-adapter shared skeleton was declined: the core marshalling is irreducibly per-engine, so a callback-driven shared helper across 7 engines is a net readability/correctness loss.

The trampoline/invoke body -- marshal-args loop with partial-cleanup, dispatch via calogCall/calogFnInvoke, free-args loop, and error-message extraction from the result -- is duplicated 13 times across the seven adapters and belongs in a shared adapterCommon helper.

The identical five-step skeleton appears twice per adapter (native trampoline + foreign-fn invoke): luaTrampoline (luaAdapter.c:572-620) and luaFunctionCall (259-307); jsTrampoline (jsAdapter.c:654-705) and jsForeignCall (290-334); squirrelTrampoline (squirrelAdapter.c:618-674) and squirrelForeignCall (268-321); berryTrampoline (berryAdapter.c:546-605) and berryForeignCall (227-277); s7DispatchNative (s7Adapter.c:142-192) and s7ForeignApply (285-335); wrenDispatch (wrenAdapter.c:218-270) and wrenForeignInvoke (358-409); plus mbDispatch (mybasicAdapter.c:538-647). Each repeats verbatim: args = (CalogValueT *)calloc((size_t)argCount, sizeof(CalogValueT)) with an OOM throw, the for (cleanup = 0; cleanup < index; cleanup++) { calogValueFree(&args[cleanup]); } free(args); partial-unwind, the post-call for (...) calogValueFree(&args[index]); free(args);, and the error extraction message = (result.type == calogStringE) ? result.as.s.bytes : "native call failed" (the fallback strings 'native call failed'/'function value failed' are themselves 13 duplicated literals). Only the per-engine toValue call and the throw mechanism differ. A shared helper pair -- e.g. calogAdapterMarshalArgs(toValueFn, ctx, n, &args) returning status with cleanup done, and calogAdapterErrorMessage(&result) -- would delete several hundred duplicated lines and make the cleanup semantics single-source.

Verifier: Verified in the code: luaTrampoline (luaAdapter.c:572-620) and luaFunctionCall (259-307) are near-verbatim clones, and grep confirms 12 identical partial-unwind loops (calogValueFree(&args[cleanup])), 12 copies of the error-extraction ternary with the duplicated "native call failed"/"function value failed" literals, and the same calloc+OOM/marshal/free skeleton across all seven adapters (berry/js name the count 'argc', mybasic's mbDispatch is the partial 13th site via mb_value_t). No adapterCommon or shared helper exists in src/ and no comment documents the duplication as intentional. The engine-varying parts (toValue fetch, throw mechanism) factor cleanly behind a fetch-callback marshal helper plus a message-extraction helper, so the proposed consolidation is concrete and would single-source the partial-cleanup semantics. Not a runtime bug (each copy's unwind is currently correct), so it stays a duplication/cleanup finding, not higher.

M32. [bug] src/mybasic/mybasicAdapter.c:404 --- FIXED

Status: FIXED

In mbAggregateToCollDepth, a LIST/DICT dict KEY is stored via mb_set_coll without the mbRetainBeforeSet reference that values get, so an aggregate-keyed map corrupts refcounts at dict teardown.

The pair loop does mbRetainBeforeSet(context, l, element); mb_set_coll(context->bas, l, coll, key, element); mbReleaseSetValue(context, key); mbReleaseSetValue(context, element); (lines 404-407). mbRetainBeforeSet is applied only to element, never to key, yet mbReleaseSetValue skips LIST/DICT for both. In the vendored fork, _set_dict wraps key and value identically (_create_internal_object_from_public_value stores a collection by pointer, no ref added) and _destroy_dict tears both down identically via _ht_foreach(c->dict, _destroy_object_with_extra). So keys and values receive identical teardown but non-identical setup: the value path (with the extra retain) is the one proven leak-free by the ASan-clean nested-collection tests, which means a LIST/DICT key stored without that retain is off by one reference and is over-released (or leaked, depending on the creation-count convention) when the parent dict is destroyed. Concrete trigger: a host native returns a CalogAggT map whose key is itself a list/map (calogAggSet accepts any CalogValueT key) and a .bas script receives it -- mbFromValueDepth marshals the key through mbAggregateToCollDepth into an MB_DT_LIST/MB_DT_DICT key value. Fix: call mbRetainBeforeSet for key too, symmetrically with element.

Verifier: Verified in vendor/ourbasic/ourBasic.c: _set_dict stores LIST/DICT keys and values identically by pointer with no ref added (_create_internal_object_from_public_value -> _public_value_to_internal_object just copies the pointer), and _destroy_dict tears both down identically (_destroy_object_with_extra -> _dispose_object -> _UNREF_COLL -> one _unref each). _create_ref starts the count at _NONE_REF (1), so the retained value path (mb_init_coll=1, mbRetainBeforeSet=2, teardown unref -> 1 -> destroyed) is balanced, while the unretained key path underflows: teardown _unref drops the count to 0, tripping mb_assert(*ref->count >= _NONE_REF) in debug builds and, in release, leaving _unref_dict/_unref_list unable to ever destroy the key (leak) while _gc_add records it with a corrupted count. The trigger is reachable: calogAggSet (src/value.c:153) accepts any CalogValueT key including calogAggE, and mbAggregateToCollDepth's pair loop (mybasicAdapter.c:393) marshals such a key into an MB_DT_LIST/MB_DT_DICT dict key with no retain at line 404-405. Fix is the claimed one-liner: mbRetainBeforeSet(context, l, key) before mb_set_coll.

M33. [bug] src/s7/s7Adapter.c:111 --- FIXED

Status: FIXED

s7CallableInvoke builds argList with s7_cons unprotected while s7FromValue allocates, so earlier argument cells can be garbage-collected before s7_call runs.

Lines 111-117: argList = s7_nil(sc); for (index = argCount - 1; index >= 0; index--) { element = s7FromValue(context, &args[index], 0); argList = s7_cons(sc, element, argList); } ret = s7_call(sc, export->proc, argList);. Same root cause as the s7FromValue finding: the argList chain is unrooted; if any argument is a string-heavy aggregate, its marshalling can allocate past the 256-cell GC temp window and collect the cons cells / elements already built for later-index arguments. Failure scenario: a foreign context invokes an exported s7 callback with (bigTable, x) -- while marshalling bigTable (index 0, done last is fine, but with args (x, bigTable) the index-1 aggregate is marshalled first and its result plus the first cons sit unprotected while nothing shields them once the window rolls) -- the callback receives freed/retyped cells. Fix: s7_gc_protect a holder for argList during the loop (s7_call itself roots its args once entered).

Verifier: s7Adapter.c:111-117 builds argList with s7_cons/s7FromValue with zero GC protection (file's only s7_gc_protect calls are for the exported proc at line 250 and an iterator at line 531), and the vendored s7.c GC (lines 8185-8198) marks only the most recent GC_TEMPS_SIZE=256 allocations as implicit temps -- C locals are not roots. Marshalling a large aggregate at any index below argCount-1 allocates enough cells (one-plus per entry in s7FromValue) to push the previously built cons chain out of the 256-cell window; if the free heap empties during that marshal, the GC sweeps the earlier conses and elements, and s7_call then walks freed/retyped cells. s7_call roots its args only once entered, so the loop is the unguarded window. No comment documents this as an intentional tradeoff.

M34. [error-path] src/s7/s7Adapter.c:117 --- FIXED

Status: FIXED

s7CallableInvoke is the only one of the seven callable-invoke paths that propagates no errors: argument-marshal failures and callback errors are silently converted to calogOkE.

s7CallableInvoke (s7Adapter.c:98-119) does element = s7FromValue(context, &args[index], 0); with no status check (s7FromValue returns s7_unspecified() on depth overflow or unknown type), then ret = s7_call(sc, export->proc, argList); return s7ToValue(context, ret, result, 0); with no error check. Per vendor/s7/s7.h:668 s7_call 'makes sure some sort of catch exists if an error occurs', so a raising Scheme callback yields an error object, which falls through every s7ToValue test to the '#, eof, symbols, etc. -> nil' fallthrough (s7Adapter.c:602) and returns calogOkE with a nil result. Every other adapter converts callback failure into calogFail with the engine's message (luaAdapter.c:84-91 via lua_pcall, jsAdapter.c:166-178 via JS_IsException, squirrelAdapter.c:87-90 via sq_call, berryAdapter.c:103-110 via be_pcall, wrenAdapter.c:125-131 via wrenCall, mybasicAdapter.c:455-462 via code check). Failure scenario: a pubsub/timer library that checks calogFnInvoke's status to detect a dead/faulty subscriber sees the failure on six engines but gets calogOkE + nil from an s7 callback that raised -- the error is unobservable.

Verifier: Verified in src/s7/s7Adapter.c:98-119: s7CallableInvoke checks neither s7FromValue marshalling nor s7_call errors; vendor/s7/s7.c:56836-56871 shows s7_call returns sc->value unconditionally with no C-visible error flag after an Error_Jump, and the returned error object falls through every type test in s7ToValue to the calogOkE+nil fallthrough at s7Adapter.c:601-602. All six other adapters (lua_pcall, JS_IsException, sq_call, be_pcall, wrenCall, MB_FUNC_OK) were verified to return calogFail with a message on both marshal and callback failure. The file-top error-model comment documents only the ingress (s7_error) and run (catch wrapper) paths, not this egress path, so it is not a documented tradeoff; s7_call_with_catch (s7.h:659) exists so it is fixable. Concrete impact: calogExport.c:99 propagates invoke status to the calling script, so a cross-language calogCall into a raising s7 function returns Ok+nil while the same call into any other engine returns a failure. Severity is medium, not high: no memory unsafety/data loss/deadlock; pubsub (only distinguishes DeadE/NotFoundE) and timer (only DeadE) behave nearly identically either way, and s7 still prints the error to stderr, so the bug is programmatic unobservability of callback errors in an edge case (raising callback) on one engine.

M35. [bug] src/s7/s7Adapter.c:156 --- FIXED

Status: FIXED

s7DispatchNative calls s7_string on its first argument without checking it is a string, so (%calog-call 42) is undefined behavior.

Line 156: name = s7_string(s7_car(args));. The dispatcher %calog-call is defined as an ordinary global (line 79), so scripts can call it directly with a non-string first argument; the generated wrappers always pass a literal string, but nothing stops (%calog-call 42). Vendored s7_string is an unchecked field access -- const char *s7_string(s7_pointer str) {return(string_value(str));} (vendor/s7/s7.c:29364) -- so on an integer cell it yields a garbage pointer that flows into calogLookup's strcmp. Failure scenario: one malformed script line crashes or corrupts the broker instead of surfacing 'as a value rather than aborting' per the file's own error model. Fix: if (!s7_is_string(s7_car(args))) { return s7_error(sc, s7_make_symbol(sc, "wrong-type-arg"), ...); } (or register the dispatcher with a typed signature).

Verifier: Verified end-to-end: %calog-call is registered as an untyped ordinary global (s7_define_function, s7Adapter.c:79) so scripts can call it directly; s7DispatchNative passes s7_car(args) to s7_string with no s7_is_string check (line 156); vendored s7_string is an unchecked union-field read in this build (s7.c:29364 -> string_value -> T_Str(P)=P since S7_DEBUGGING defaults to 0 at s7.c:339 and the Makefile never enables it); the resulting garbage pointer is dereferenced by calogCall's name lookup at line 179. This violates the file's own documented error model (lines 14-16, 'surface as a value rather than aborting') which every other failure path in the same function honors via s7_error. Medium, not high: generated wrappers (line 276) always pass a literal string, so it is unreachable in normal use and requires a script deliberately calling the internal dispatcher with a non-string -- but one such line crashes the whole multi-context broker instead of raising a catchable Scheme error.

M36. [duplication] src/s7/s7Adapter.c:285 --- FIXED

Status: FIXED

s7ForeignApply (lines 285-335) duplicates s7DispatchNative (lines 142-192) almost line-for-line; the marshal/invoke/cleanup/error machinery should be one shared routine.

Both functions contain the identical sequence: argCount = s7_list_length(sc, rest); cargs = NULL; if (argCount > 0) { cargs = calloc(...); ... } then the per-node s7ToValue loop with the partial-cleanup for (cleanup = 0; cleanup < index; cleanup++) calogValueFree(&cargs[cleanup]); free(cargs); error path, then the post-invoke free loop, then the (result.type == calogStringE) ? result.as.s.bytes : ... error-to-s7_error conversion, then sresult = s7FromValue(...); calogValueFree(&result); return sresult;. The only differences are the invoke call (calogCall vs calogFnInvoke) and three string literals. A shared helper (e.g. marshal an s7 arg list to a CalogValueT array, plus a shared finish-invocation tail) collapses roughly 40 duplicated lines and guarantees fixes like the missing s7_is_string check land in both paths. This is exactly the grouping opportunity the project guidelines call for.

Verifier: Verified in src/s7/s7Adapter.c: lines 158-191 (s7DispatchNative) and 300-334 (s7ForeignApply) are ~34-line near-verbatim duplicates -- same calloc/OOM path, same s7ToValue marshal loop with identical partial-cleanup error path, same post-invoke free loop, same (result.type == calogStringE) error conversion, same s7FromValue/calogValueFree tail. Only the first-arg extraction (name string vs c-object callable), the invoke call (calogCall line 179 vs calogFnInvoke line 321), and string literals differ. The error tails have already drifted (snprintf into a stack buffer at 184-188 vs direct s7_make_string at 326-331), demonstrating the maintenance cost. No comment marks the duplication as intentional, and a marshal-args helper plus a finish-invocation helper factors it cleanly without contortion.

M37. [duplication] src/squirrel/squirrelAdapter.c:30 --- FIXED

Status: FIXED

BindingT (broker+name), its dynamic-array grow block, and its destroy loop are duplicated verbatim between luaAdapter.c and squirrelAdapter.c, with berryTrackForeign repeating the same grow idiom a third time.

squirrelAdapter.c:30-33 and luaAdapter.c:30-33 define the identical typedef struct BindingT { CalogT *broker; char *name; } BindingT;. The push-with-growth block is duplicated line for line: luaAdapter.c:236-250 and squirrelAdapter.c:239-253 both read newCap = (context->bindingCap == 0) ? *_BINDING_INITIAL : context->bindingCap * CALOG_GROWTH_FACTOR; resized = (BindingT **)realloc(context->bindings, (size_t)newCap * sizeof(BindingT *)); ... including the same free(binding->name)/free(binding) error unwind, and the destroy loops are identical too (luaAdapter.c:159-163, squirrelAdapter.c:149-153). berryTrackForeign (berryAdapter.c:528-543) is the same grow idiom for a CalogFnT* array. The binding allocation prologue (calogLookup -> malloc -> strdup -> unwind) at luaAdapter.c:222-235 / squirrelAdapter.c:225-238 is also byte-identical. Extract a shared bindingList/ptr-vector helper (adapterCommon.c) so the grow/unwind/destroy semantics exist once; today a fix in one copy (e.g. an overflow guard on newCap) would have to be re-applied in three files.

Verifier: Verified byte-identical duplication: BindingT typedef (luaAdapter.c:30-33 / squirrelAdapter.c:30-33, plus a third copy in mybasicAdapter.c:23-26), the malloc/strdup/unwind expose prologue, the realloc grow block, and the destroy loop all match line-for-line between luaAdapter.c and squirrelAdapter.c; berryTrackForeign (berryAdapter.c:528-543) repeats the same grow idiom. The grow idiom actually appears in 7 places (also mybasicAdapter.c:574 and context.c:358/745/1039), and the copies have already diverged: value.c:308-311 has an overflow guard on newCap that none of the adapter/context copies have -- concrete evidence of the multiple-sources-of-truth failure mode. No comment documents this as an intentional tradeoff. Not a runtime bug (overflow needs 2^31 bindings), but a shared ptr-vector push helper would collapse 7 divergent copies into one.

M38. [error-path] src/squirrel/squirrelAdapter.c:456 --- FIXED

Status: FIXED

All Squirrel compile and runtime error detail is silently discarded: no compiler error handler, no runtime error handler, and sq_getlasterror is never consulted.

calogSquirrelRun prints only the fixed strings "squirrel compile error" / "squirrel run error" (lines 457, 463) with no message, source name, or line number. Verified against the vendored VM: sqstate.cpp:19 and sqvm.cpp:121 leave _errorfunc/_errorhandler null, sqvm.cpp:1149 CallErrorHandler is a no-op when _errorhandler is null, and sqcompiler.cpp:193 only reports if _compilererrorhandler was set -- the adapter sets neither and never calls sq_getlasterror after a failed sq_compilebuffer/sq_call. This also swallows the carefully-propagated native error strings that squirrelTrampoline throws at line 664 (sq_throwerror(v, message) with the broker's error text) whenever a script does not try/catch. Contrast luaAdapter.c:428/433 which prints lua_tostring(L, -1). The comment in squirrelEngine.c:58-59 ("calogSquirrelRun has already written the detailed diagnostic to stderr") is false for this engine. Related: squirrelAdapter.c:124 registers squirrelPrint (which writes to stdout) as BOTH the print func and the error func -- once handlers are added, error output should go to stderr. Fix: sq_setcompilererrorhandler + fetch/print sq_getlasterror after a failed sq_call, mirroring the Lua adapter.

Verifier: Verified in squirrelAdapter.c:456-466 that only fixed strings "squirrel compile error"/"squirrel run error" are printed; grep confirms no sq_setcompilererrorhandler, sq_seterrorhandler, or sq_getlasterror call anywhere in src/ (only sq_setprintfunc at line 124). Vendored VM confirms the consequence: sqstate.cpp:17 leaves _compilererrorhandler NULL, sqvm.cpp:121 leaves _errorhandler null and sqvm.cpp:1149 makes CallErrorHandler a no-op then, and sqcompiler.cpp:193 emits diagnostics only when _compilererrorhandler is set. So compile-error line numbers, runtime error messages, and the broker error strings thrown via sq_throwerror at squirrelAdapter.c:664 are all silently lost. The squirrelEngine.c:58-59 comment claiming a detailed diagnostic was written is false, and luaAdapter.c:428/433 shows the intended contract (printing the actual error) that this engine breaks. Not high severity: no memory unsafety or data corruption, purely lost diagnostics, but it fires on every Squirrel script error, so medium is right.

M39. [optimization] src/value.c:77 --- FIXED

Status: FIXED

Deep-copying an aggregate's map part is O(n^2) because the copy loop goes through calogAggSet, which linearly re-scans all previously copied pairs for a duplicate key that provably cannot exist.

aggregateCopyDepth's pair loop calls status = calogAggSet(copy, &key, &value); (value.c:77), and calogAggSet (value.c:158-165) first scans every existing pair with calogValueEquals before appending. But the source map's keys are unique by construction (they were inserted via calogAggSet), and copying cannot create a collision: scalar/string keys copy to equal-if-and-only-if-equal values, aggregate keys copy to fresh pointers (compared by identity), fn keys keep their pointer, and NaN real keys never match anything either way -- so the duplicate scan in the copy path always finds nothing. Since the file header states all cross-boundary data is deep-copied ("calogValueCopy deep-copies aggregates ... so no two owners ... ever share a heap pointer"), every script->native marshal of an n-entry map pays ~n^2/2 calogValueEquals calls including memcmp on string keys -- 50M comparisons for a 10k-entry map, per copy, per hop. Fix: append directly in the copy loop (ensureCapacity + calogValueMove into pairs[pairCount], i.e. the tail of calogAggSet split out as a shared static aggAppendPair helper), skipping the scan.

Verifier: Verified in src/value.c: aggregateCopyDepth (line 77) inserts each copied pair via calogAggSet, whose dedup scan (lines 158-165) walks all prior pairs with calogValueEquals. The scan is provably dead in the copy path: pairCount++ exists only in calogAggSet (value.c:174) and repo-wide grep shows no other writer to pairs, so source keys are unique by construction; per calogValueEquals semantics, copied scalar/string keys preserve equality exactly, copied aggregate keys are fresh calloc pointers compared by identity (never equal), fn keys keep their unique pointer, and NaN keys never match under either path -- so direct append is behavior-preserving and drops the pair-copy phase from O(n^2) to O(n). No comment documents the scan as an intentional tradeoff, and the file header confirms this copy runs on every cross-boundary marshal (context.c call args, calogKv, calogPubsub per-subscriber fan-out). Fix is trivial: share calogAggSet's append tail (lines 166-175) as a static helper. Severity stays medium (not higher) because it is pure wasted CPU, not a correctness issue, and map construction via calogAggSet is already O(n^2) by assoc-list design, so very large maps were slow before the copy anyway.

M40. [dead-code] src/value.c:249 --- KEPT

Status: KEPT -- calogFnMarkDead/alive has test coverage (tests/testBroker.c:274), so the "never used" premise is false. Deleting a tested safety primitive is not warranted.

calogFnMarkDead and the _Atomic alive flag are never used by any production code -- the alive check in calogFnInvoke is a condition that can never be false in a real runtime.

void calogFnMarkDead(CalogFnT *callable) (value.c:249-254) is declared in calogInternal.h:100 under "callable lifecycle (driven by the engine adapters)", but grep over src/ and libs/ shows zero callers: no engine adapter and no core path ever calls it; the only caller in the repo is tests/testBroker.c:274. Consequently _Atomic bool alive (value.c:28) is initialized true (value.c:193) and can never become false, so the guard in calogFnInvoke -- if (!atomic_load_explicit(&callable->alive, memory_order_acquire)) { return calogFail(result, calogErrNotFoundE, "callable owner no longer exists"); } (value.c:237-239) -- is an unnecessary conditional (plus an atomic acquire load) paid on every single callable invoke while being unreachable. In production, a dead owner is instead detected by contextDispatch returning calogErrDeadE (context.c:644). Either wire context teardown to mark its exported callables dead (which would give the nicer error), or delete calogFnMarkDead, the alive field, and the check (and the memory_order includes if nothing else needs them).

Verifier: Repo-wide grep confirms calogFnMarkDead (value.c:249, declared calogInternal.h:100) has exactly one caller: tests/testBroker.c:274. No engine adapter or core path calls it, so the _Atomic alive flag (init true at value.c:193) can never become false in production, making the acquire-load + branch in calogFnInvoke (value.c:237-239) unreachable dead-guard cost on every invoke. Production dead-owner detection verifiably happens elsewhere: actorInvokeCallable -> contextDispatch returns calogErrDeadE via registry resolution (context.c:639-645), and calogFnFinalize uses calogContextRegistered (value.c:220) -- a second, working source of truth. No comment documents alive as an intentional tradeoff, and the header's "driven by the engine adapters" claim is false. Concrete cleanup: remove the function/field/branch/test (or wire context teardown to call it for the better error message).

M41. [duplication] src/value.c:299 --- FIXED

Status: FIXED

ensureCapacity is a general grow-doubling helper kept static in value.c while context.c hand-rolls the identical pattern three more times -- without ensureCapacity's overflow guards.

value.c:299-323 implements the canonical growth routine (newCap = (*cap == 0) ? BROKER_MIN_CAPACITY : *cap; while (newCap < needed) { ... newCap *= CALOG_GROWTH_FACTOR; } plus INT64_MAX and SIZE_MAX overflow checks before realloc), but it is static, so context.c re-implements the same doubling-realloc dance three times: ctxSlots growth (context.c:356-368), engines growth (context.c:743-751), and ctxFree growth (context.c:1037-1045), e.g. newCap = (calog->engineCap == 0) ? CONTEXT_INITIAL_ENGINES : calog->engineCap * CALOG_GROWTH_FACTOR;. None of the three copies carries the multiply-overflow or SIZE_MAX/elemSize checks the shared routine has, so the growth policy exists in four places that have already drifted in robustness. Export ensureCapacity through calogInternal.h (it already takes a void** buffer, cap pointer, needed count, and element size -- exactly what all four sites need, with the initial-capacity floor as a parameter) and delete the three hand-rolled copies. Also note the misleading name: BROKER_MIN_CAPACITY (value.c:18) is the aggregate growth floor, unrelated to the broker registry, which has its own BROKER_INITIAL_SLOTS 16 in broker.c:16.

Verifier: Verified: value.c:299 ensureCapacity is a static, fully general grow-doubling helper (void** buffer, cap, needed, elemSize) with INT64_MAX and SIZE_MAX overflow guards, while context.c hand-rolls the same doubling-realloc three times (ctxSlots 355-369, engines 742-752, ctxFree 1036-1046) with no guards and per-site floor constants -- the only thing ensureCapacity hardcodes. calogInternal.h exists as the export path, and BROKER_MIN_CAPACITY in value.c does collide in name with broker.c's separate BROKER_INITIAL_SLOTS. The claim's guard-drift angle is not an exploitable bug (all three sites grow by one from bounded counts -- ctxCount capped at CONTEXT_INDEX_MASK 0xFFFFFFFF -- so int64 overflow is unreachable before realloc OOM), but the duplication is real: one growth policy in four places, already drifted, with an exact-fit existing helper needing only a floor parameter. Concrete, mechanical consolidation with clear payoff.

M42. [duplication] src/wren/wrenAdapter.c:358 --- FIXED

Status: FIXED

wrenForeignInvoke (lines 358-409) duplicates wrenDispatch (lines 218-270): same calloc/marshal-loop/partial-cleanup/free-loop/abort-fiber error skeleton with only the invoke call and slot indices differing.

Both contain: the cargs = calloc((size_t)argCount, sizeof(CalogValueT)) guard with an OOM abort-fiber path, the wrenGetListElement(vm, listSlot, index, scratch); status = wrenToValue(context, scratch, &cargs[index], 0); loop with the identical for (cleanup = 0; cleanup < index; cleanup++) calogValueFree(&cargs[cleanup]); free(cargs); wrenSetSlotString(vm, 0, ...); wrenAbortFiber(vm, 0); failure block, the post-call free loop, the snprintf(message, sizeof(message), "%s", (result.type == calogStringE) ? result.as.s.bytes : ...) error path, and the wrenFromValue(context, &result, 0, 0); calogValueFree(&result); tail. Only calogCall vs calogFnInvoke and the list/scratch slot numbers differ. A shared static helper taking (context, listSlot, scratchSlot, invoke fn) removes ~45 duplicated lines and keeps the two dispatch paths from drifting (e.g. the type-validation fix must currently be written twice).

Verifier: Verified in src/wren/wrenAdapter.c: wrenDispatch (218-270) and wrenForeignInvoke (358-409) repeat the same ~45-line skeleton -- calloc OOM guard with abort-fiber, wrenGetListElement/wrenToValue marshal loop with byte-identical partial-cleanup failure block, post-call calogValueFree loop + free(cargs), identical snprintf/"%s" error tail, and identical wrenFromValue/calogValueFree success tail. Only the invoke call (calogCall vs calogFnInvoke), target acquisition, slot indices (list 2/scratch 3 vs list 1/scratch 2), and message strings differ, all parameterizable via a shared static helper. No comment documents the duplication as intentional. Drift risk is concrete: any fix to the marshal/cleanup/abort path must be written twice, 140 lines apart.

M43. [multiple-truth] src/wren/wrenAdapter.c:412 --- FIXED

Status: FIXED

The fromValue direction enforces the shared depth cap inconsistently: Lua/Squirrel/JS/mybasic propagate calogErrDepthE from any nesting level, Berry checks it only at the top level (recursive statuses discarded), and Wren/s7 cannot report it at all.

wrenFromValue is static void wrenFromValue(...) (wrenAdapter.c:412) -- on depth > CALOG_MAX_DEPTH it does wrenSetSlotNull(vm, slot); return; so callers (wrenCallableInvoke:121, wrenDispatch:268, wrenForeignInvoke:407) cannot distinguish failure from a legitimate null. s7FromValue similarly returns s7_unspecified(sc) on overflow (s7Adapter.c:353-355) and no caller can tell. Berry's berryFromValue DOES return int32_t, but its own aggregate recursion throws the status away: berryAdapter.c:317 berryFromValue(context, &aggregate->array[index], depth + 1); be_setindex(vm, -3);, likewise lines 331 and 339, and berryForeignCall:274 ignores the result-marshal status too -- so an over-deep element deep inside a container silently becomes nil while the call reports calogOkE. Contrast luaPushValueDepth (luaAdapter.c:386-403), squirrelPushValueDepth (squirrelAdapter.c:387-426), jsFromValue (jsAdapter.c:380-397), and mbAggregateToCollDepth (mybasicAdapter.c:378-403), which all propagate the child status. Failure scenario: the same 65-deep aggregate returned by a native errors loudly in Lua/Squirrel/JS/BASIC but is silently truncated to nulls in Berry/Wren/s7 scripts. The signature drift (void / s7_pointer / int32_t for the same job) is the root cause and belongs to one shared convention.

Verifier: Every cited line checks out: wrenFromValue (wrenAdapter.c:412) is void and maps depth overflow to wrenSetSlotNull, indistinguishable from a real nil at all three call sites; s7FromValue (s7Adapter.c:353-355) returns s7_unspecified on overflow, the same object its calogNilE case returns; berryFromValue returns calogErrDepthE but its own recursive calls at berryAdapter.c:317/331/339 and berryForeignCall:274 discard the status, so a deep element inside a container silently becomes nil while the call reports calogOkE. Lua (luaAdapter.c:386-403, caller raises luaL_error at 303-305) and JS (jsAdapter.c:351, JS_ThrowRangeError propagated via JS_IsException) propagate the error from any nesting level. The path is reachable, not defensive dead code: calogAggPush/calogAggSet use calogValueMove with no depth check so a native can incrementally build a >64-deep aggregate via the public API, and calogFnInvoke (value.c:235-246) hands the native's result to fromValue with no calogValueCopy, bypassing the only enforced bound (value.c:42). calog.h:30 documents the contract as failing with calogErrDepthE, so the silent-null behavior contradicts documented intent rather than being a stated tradeoff.

M44. [duplication] src/wren/wrenAdapter.c:526 --- FIXED

Status: FIXED

The double-to-int64 round-trip classifier, including the bare +/-9223372036854775808.0 magic literals, is copy-pasted between jsAdapter.c and wrenAdapter.c instead of living in value.c.

wrenAdapter.c:526 and jsAdapter.c:524 contain the byte-identical line if (isfinite(number) && number == floor(number) && number >= -9223372036854775808.0 && number < 9223372036854775808.0) { calogValueInt(out, (int64_t)number); } else { calogValueReal(out, number); }. This is the canonical 'engine number is a double' ingress rule and any future engine with double-only numbers will need a third copy; the 2^63 boundary literals are unnamed in both. grep confirms no shared helper exists in src/value.c or src/calog.h. Hoist into value.c as e.g. void calogValueFromDouble(CalogValueT *out, double number) (or an INT64_EXACT_MIN/MAX_DOUBLE pair of named constants) so the subtle asymmetric bounds (>= min, strictly < max, because 2^63 is not representable) are encoded exactly once.

Verifier: Verified byte-identical classifier at wrenAdapter.c:526 and jsAdapter.c:524; grep confirms no shared helper exists in value.c or calog.h (only calogValueInt/calogValueReal constructors). The duplicated line is not boilerplate: it encodes asymmetric UB-critical bounds (>= -2^63 but < 2^63, since +2^63 is unrepresentable as a double and an out-of-range cast is UB), with unnamed 2^63 literals in both copies. Drift has already begun -- the explanatory comment exists only in the js copy, not the wren copy. Hoisting into a calogValueFromDouble helper in value.c is a concrete, worthwhile single-source-of-truth cleanup.

M45. [error-path] vendor/ourbasic/ourBasic.c:19527 --- FIXED

Status: FIXED

mb_eval_routine_cold leaves a stale landing node on s->sub_stack every time the invoked routine errors, accumulating one leaked list node per failed callback on a long-lived interpreter.

mb_eval_routine_cold() -> mb_eval_routine() -> _eval_script_routine() pushes the landing node with _ls_pushback(s->sub_stack, ast); (ourBasic.c:4594). That entry is only popped by _core_return (16536) or _core_enddef (16760) on a successful MB_SUB_RETURN. Every error exit taken after the push -- the ast == *l guard (4615), MB_FUNC_SUSPEND (4626), any body statement error (4631), and null entry (4609) -- does goto _exit without popping, so the node stays on s->sub_stack forever (only freed at mb_close). Upstream tolerates this because an error aborts the whole run, but the entire point of the calog cold entry point is REPEATED invocation on a long-lived idle interpreter: calog keeps the context alive after a callback failure (mbCallableInvoke at src/mybasic/mybasicAdapter.c:461 just returns calogFail) so e.g. a periodic timer whose BASIC callback errors on every tick grows sub_stack by one _ls_node_t per tick, unbounded. Fix inside the patch: record the sub_stack tail/count before calling mb_eval_routine and _ls_popback back down to it when the result is not MB_FUNC_OK.

Verifier: Confirmed in vendor/ourbasic/ourBasic.c: _eval_script_routine pushes the landing node at line 4594, and every error exit after the push (null entry 4609, stuck-ast 4615, SUSPEND 4626, statement error 4631) jumps to _exit without popping; the only pops are success-path _core_return (16536), _core_enddef (16760), and _execute_statement (11257). sub_stack is otherwise cleared only by mb_reset (12442, which destroys the AST and is never called by calog) or freed at mb_close. calog's mbCallableInvoke (src/mybasic/mybasicAdapter.c:453,461) invokes mb_eval_routine_cold repeatedly on a long-lived interpreter and on failure just returns calogFail, keeping the context alive, so each failed callback leaks one _ls_node_t, unbounded. Stale entries additionally let a later mismatched RETURN pop a stale landing node instead of raising SE_RN_NO_RETURN_POINT.

LOW (62)

L1. [dead-code] Makefile:33 --- FIXED

Status: FIXED

The CORE variable (CORE = obj/value.o obj/broker.o) is defined and never referenced anywhere in the Makefile.

grep '\$(CORE)' finds no uses; every place that needs those objects (STRICTOBJ line 146, CALOGLIB line 272, the tsan targets) lists obj/value.o and obj/broker.o explicitly. The variable is a stale leftover that also invites drift -- someone could edit CORE expecting it to affect the build and see no change. Delete it (or, better, actually use it in CALOGLIB/STRICTOBJ so the core object list has one home).

Verifier: Makefile line 33 defines CORE = obj/value.o obj/broker.o, and grep confirms $(CORE) (or ${CORE}) is expanded nowhere; all 11 other "CORE" hits are the unrelated COREFLAGS variable. The two objects are instead listed explicitly in STRICTOBJ (line 146) and CALOGLIB (line 272), so the variable is dead and editing it silently does nothing. Concrete cleanup: delete CORE or reference it from STRICTOBJ/CALOGLIB so the core object list has a single home.

L2. [multiple-truth] Makefile:393 --- FIXED

Status: FIXED

bin/calog (and testCrypto/testHttp/testHttps/testSsh/testDbPg/testDbMysql) consume $(SSLARCH)/$(PGARCHIVES)/$(MYSQLARCH) only in the recipe, not as prerequisites, so rebuilding those vendored archives never triggers a relink.

Line 393: $(CC) $(LDFLAGS) -pthread -o $@ $^ -Wl,--start-group $(PGARCHIVES) -Wl,--end-group $(MYSQLARCH) $(SSLARCH) -lstdc++ -ldl -lm -lpthread -- the archives appear after $^ so they are invisible to make's dependency graph (same at 338, 341, 451, 463, 466, 470). $(LIBSSH2LIB) by contrast IS a prerequisite at line 392. The vendored-deps work is ongoing (OpenSSL et al get rebuilt); after such a rebuild, make reports bin/calog up to date and the binary keeps the stale crypto code. Listing the archive paths as normal prerequisites (they need no rules while hand-built) fixes staleness and turns a missing archive into a clear 'No rule to make target vendor/openssl/libssl.a' instead of a raw ld error.

Verifier: Verified in Makefile: $(SSLARCH)/$(PGARCHIVES)/$(MYSQLARCH) appear only in recipes after $^ at lines 338, 341, 393, 451, 463, 466, 470 and never in any prerequisite list, while $(LIBSSH2LIB) is correctly a prerequisite (lines 392, 469) with a rule at 164 -- proving the intended pattern exists and these archives deviate from it. No Makefile rule builds the SSL/PG/MariaDB archives (hand-built, per in-progress vendored-deps work), and no comment documents excluding them as intentional. Rebuilding vendor/openssl therefore leaves bin/calog and the six test binaries reported up-to-date with stale crypto code, and a missing archive yields a raw ld error instead of make's clear missing-target diagnostic.

L3. [multiple-truth] Makefile:428 --- FIXED

Status: FIXED

The seven -DCALOG_WITH_ engine defines are spelled out literally in the obj/testLoad.o rule instead of using the $(ENGINEDEFS) variable defined for exactly this purpose.*

Line 222 defines ENGINEDEFS = -DCALOG_WITH_LUA -DCALOG_WITH_JS -DCALOG_WITH_SQUIRREL -DCALOG_WITH_MYBASIC -DCALOG_WITH_BERRY -DCALOG_WITH_S7 -DCALOG_WITH_WREN and it is used by the calogTask.o (225) and calogMain.o (230) rules, but line 428 repeats the full literal list: $(CC) $(COREFLAGS) -Isrc -pthread -DCALOG_WITH_LUA -DCALOG_WITH_JS ... -c -o $@ $<. Adding an eighth engine now requires remembering this second copy; if it drifts, testLoad silently stops exercising the new engine while still passing. Replace with $(ENGINEDEFS).

Verifier: Makefile:222 defines ENGINEDEFS with exactly the seven -DCALOG_WITH_* flags and uses it at lines 225 (calogTask.o) and 230 (calogMain.o), while line 428 (obj/testLoad.o recipe) repeats the identical seven defines literally. No comment documents an intentional divergence; the comment at 424-426 explicitly states testLoad is compiled with all CALOG_WITH_* flags, so the variable encodes the intended contract and line 428 is a second source of truth. If an eighth engine is added to ENGINEDEFS but line 428 is missed, bin/testLoad still builds and passes while silently not exercising the new engine. Drop-in fix: replace the literal list with $(ENGINEDEFS).

L4. [multiple-truth] Makefile:515 --- FIXED

Status: FIXED

The QuickJS core file list (quickjs libregexp libunicode dtoa) is maintained in four places: QJSOBJ and three lines of the tsanjs target.

Line 91 QJSOBJ = obj/quickjs.o obj/libregexp.o obj/libunicode.o obj/dtoa.o is the canonical list, but tsanjs re-enumerates it at line 515 (for f in quickjs libregexp libunicode dtoa; do ...), line 518 (the four .tsan.o link inputs), and line 520 (the rm). If QuickJS-ng ever re-splits cutils.c out of the headers (the comment at line 86 notes it moved once already), tsanjs silently tests a partial VM or fails to link. Derive the tsanjs names from QJSOBJ (e.g. $(patsubst obj/%.o,%,$(QJSOBJ))) the way tsansq/tsanberry derive theirs from SQSRC/BERRYSRC.

Verifier: Makefile confirms all four copies of the QuickJS core file list: QJSOBJ at line 91, the tsanjs for-loop at line 515, the link inputs at line 518, and the rm at line 520 -- grep shows no other literal enumerations. The comment at line 86 ('cutils is header-only now') proves the list has already changed once upstream, and sibling targets tsansq (line 508/510) and tsanberry (line 537/540/542) already derive their tsan lists from SQSRC/BERRYSRC, making tsanjs an outlier from the file's own pattern with a trivial patsubst fix. The only inaccuracy is the word 'silently': drift actually fails loudly (missing .c compile error or undefined-reference link error, and tsan objects are rm-ed each run), so this is a real maintenance duplication in a test-only target, not a silent-partial-testing hazard.

L5. [bug] libs/calogCrypto.c:119 --- FIXED

Status: FIXED

cryptoBase64Encode's INT_MAX input guard is too loose: EVP_EncodeBlock's int return overflows for inputs above ~1.61 GB.

if (args[0].as.s.length > INT_MAX) permits inLen up to 2147483647, but EVP_EncodeBlock returns the output length as int and accumulates it in int internally; output is ((inLen+2)/3)*4, which exceeds INT_MAX for any inLen > (INT_MAX/4)*3 = 1610612733 bytes. Concrete failure: cryptoBase64Encode on a 1.7 GB string (e.g. fsRead of a large file) causes signed overflow inside OpenSSL and a garbage/negative written at line 131, so calogValueString gets a negative length and the native fails with the misleading message 'out of memory'. The output buffer itself is sized in size_t arithmetic so there is no corruption, but the guard should be > (INT_MAX / 4) * 3.

Verifier: Verified against both sides of the call. libs/calogCrypto.c:119 guards only > INT_MAX, but the vendored OpenSSL (vendor/openssl/crypto/evp/encode.c, evp_encodeblock_int) accumulates the return length as int ret += 4 per 3-byte group, so any inLen > (INT_MAX/4)*3 = 1610612733 overflows the int return (UB, wraps negative in practice). The claim's downstream trace is also accurate: negative written at line 131 is rejected by calogValueString (src/value.c:461, length<0 -> calogErrRangeE), and line 135 reports the hardcoded misleading message 'cryptoBase64Encode: out of memory'. No memory corruption since outCap is size_t-sized and EVP writes via pointer arithmetic. No comment or header documents this as an intentional limit, and no other cap prevents 1.6-2.0 GB strings from reaching the native. The proposed fix > (INT_MAX / 4) * 3 is exact (1610612733 -> output 2147483644, fits).

L6. [duplication] libs/calogCrypto.c:250 --- FIXED

Status: FIXED

Hex-digit decode logic and the hex-digit table are each maintained in two places.

cryptoHexNibble (calogCrypto.c:250-261) and the inline digit decode in jsonHexQuad (calogJson.c:318-325) are the same three-range if-chain ('0'-'9' / 'a'-'f' / 'A'-'F' else error) written twice; a tiny shared helper in src/ would serve both. Within calogCrypto.c itself, static const char digits[] = "0123456789abcdef"; is declared twice (cryptoBytesToHex at line 142 and cryptoUuidNative at line 319) with the byte-to-two-nibbles emit loop duplicated at lines 154-159 and 341-342 -- one file-scope table (and arguably one nibble-emit helper) would remove the duplicate source of truth.

Verifier: Verified: static const char digits[] = "0123456789abcdef" is declared twice within libs/calogCrypto.c (line 142 in cryptoBytesToHex, line 319 in cryptoUuidNative) with the byte>>4 / byte&0x0F emit pair duplicated at lines 157-158 and 341-342 -- a genuine intra-file duplicated source of truth fixable by one file-scope constant, with no comment marking it intentional. The cross-file half of the claim (cryptoHexNibble at calogCrypto.c:250-261 vs the identical inline decode at calogJson.c:318-325) is factually accurate but not worth acting on: the two libs are deliberately self-contained ("No shared state" per the file header), and sharing ~10 stable lines via src/ would add coupling for no payoff. Likewise cryptoUuidNative cannot reuse cryptoBytesToHex directly (dash interleaving, stack buffer vs malloc+CalogValueT), so only the table hoist is the concrete cleanup.

L7. [bug] libs/calogDb.c:109 --- FIXED

Status: FIXED

calogDbRegister ignores the int32_t return of all four calogRegisterInline calls and returns calogOkE unconditionally.

Lines 109-112: calogRegisterInline(calog, "dbOpen", dbOpen, gDbLib); (and dbExec/dbQuery/dbClose) discard the status. registerEntry (src/broker.c) can fail with calogErrOomE from brokerGrow or strdup. Failure scenario: OOM during the third registration -> calogDbRegister still returns calogOkE and refCount is already incremented, but the runtime is missing dbQuery/dbClose; scripts fail later with an unrelated unknown-native error while the host believes DB support registered cleanly. The codebase's own convention checks this call (src/calogMain.c:430 calogRegisterInline(...) != calogOkE ||). Fix: accumulate/short-circuit the four statuses and roll back the refCount on failure.

Verifier: Confirmed: libs/calogDb.c:109-112 discards all four calogRegisterInline results and line 113 returns calogOkE unconditionally. registerEntry (src/broker.c:164) really can fail for new names (brokerGrow status or calogErrOomE from strdup), the header (calogDb.h:35) documents "Returns calogOkE or an error", and the caller (src/calogMain.c:432) gates startup on the return value -- so a partial registration slips past the check and surfaces later as an unknown-native error. refCount at line 107 is also incremented with no rollback. However, the trigger is OOM during startup registration only, the consequence is a misleading deferred error (no memory unsafety), and the identical discard pattern exists in calogCrypto/calogFs/calogKv/calogExport, so it is a systemic low-severity error-propagation gap.

L8. [optimization] libs/calogDb.c:271 --- FIXED

Status: FIXED

dbResolve probes the mutex-guarded handle table up to three times to discover a type tag the table already stores, and dbClose then performs a fourth lookup.

dbResolve (lines 271-298) calls calogHandleGet once per compiled backend (DB_TYPE_SQLITE, DB_TYPE_PG, DB_TYPE_MYSQL), each acquiring the table mutex, just to learn which tag matches -- information calogHandleTableT stores alongside every entry. Every dbExec/dbQuery pays up to 3 lock/probe rounds. dbClose is worse: dbResolve fetches the conn into conn (line 143), discards it, and refetches it via conn = calogHandleRemove(lib->handles, args[0].as.i, backend); (line 149) -- 4 registry probes to close one handle. A calogHandle API variant that returns the stored type (e.g. calogHandleGetAny(table, handle, &typeOut) / calogHandleRemoveAny) reduces resolve to one probe and close to one probe; correctness is unaffected today only because handles are documented never-reused.

Verifier: Verified in code: dbResolve (calogDb.c:271-298) calls calogHandleGet once per compiled backend, and each calogHandleGet (calogHandle.c:83-102) acquires the table mutex and linearly scans, so a MySQL handle or invalid handle costs 3 lock/scan rounds; the type tag being probed for is already stored in every entry (HandleEntryT.type, calogHandle.c:15). dbClose (calogDb.c:133-155) demonstrably fetches conn via dbResolve at line 143, discards it, and refetches via calogHandleRemove at line 149 -- 4 probes to close one handle. No comment documents this as an intentional tradeoff; the line 147-148 comment only covers the atomic-remove double-close guard, which a RemoveAny(table, handle, &typeOut) variant preserves exactly. The proposed GetAny/RemoveAny cleanup is concrete: dbResolve's ~28-line ifdef cascade collapses to one probe, dbClose to one atomic probe, and calogNet's 2-type remove pair (calogNet.c:317-319) could reuse it. That said, the mutexes are uncontended and the table tiny, and every caller proceeds to do SQL/network I/O that dwarfs the saved lock rounds -- the payoff is structural tidiness, not measurable performance, so severity stays low.

L9. [bug] libs/calogDb.c:395 --- FIXED

Status: FIXED

mysqlOpen silently truncates the connection string at 511 bytes, corrupting whichever key=value token straddles the cut.

char buffer[512]; (line 374) and snprintf(buffer, sizeof(buffer), "%s", conninfo); (line 395). snprintf truncates without error, so a conninfo longer than 511 bytes (long password, long socket path) loses its tail mid-token: password=verylongsecret... becomes a shorter wrong password -> baffling auth failure; a truncated sslmode=require -> sslmode=requir is at least rejected, but a host/dbname cut just yields a wrong connect target. Fix: reject with calogErrArgE when strlen(conninfo) >= sizeof(buffer), or strdup the input instead of a fixed buffer (512 is also an unnamed magic size).

Verifier: Confirmed: mysqlOpen copies the script-supplied conninfo into char buffer[512] with snprintf (libs/calogDb.c:374,395), ignoring the return value; no length check exists there or upstream (dbOpen at line 233 passes args[1].as.s.bytes with no bound, and the codebase has no string-length limit, only CALOG_MAX_DEPTH). A conninfo >= 512 bytes is silently truncated mid-token, yielding a shortened password/host/dbname and a baffling connect failure. No comment documents this as an intentional tradeoff (the nearby comments cover conninfo format and sslmode only), and pgOpen avoids the problem by passing conninfo directly to PQconnectdb. Mitigations that cap severity: the strict sslmode validation (lines 432-444) rejects a truncated sslmode value, so no silent TLS downgrade; and in practice every truncation scenario produces a visible connect/auth failure rather than silent wrong results, and 512+ byte conninfos are rare. Real bug, trivial fix, minor impact.

L10. [dead-code] libs/calogDb.c:604 --- FIXED

Status: FIXED

mysqlQuery allocates and binds the per-column errors array but never reads it, so per-column truncation reports are silently discarded.

errors = (my_bool *)calloc(columnCount, sizeof(my_bool)); (line 604) is bound via binds[column].error = &errors[column]; (line 622), then never inspected: the fetch loop at line 634 accepts rc == MYSQL_DATA_TRUNCATED and marshals the row without consulting errors[]. The vendored connector substitutes internal storage when bind->error is NULL (mysql_stmt_bind_result), so the array can simply be deleted from the allocation set and mysqlCleanup signature -- or, better, be checked so a truncated fetch becomes an error instead of silently marshalling clipped data (buffers are sized from max_length so truncation should be impossible today, which is precisely why the array is dead weight).

Verifier: Verified in libs/calogDb.c: errors is calloc'd (line 604), bound to binds[column].error (line 622), and freed (mysqlCleanup line 324) but never read anywhere -- every other reference is a pass-through argument to mysqlCleanup, while the sibling arrays isNull (read line 649) and lengths (read lines 652-653) are consumed. The fetch loop (line 634) accepts MYSQL_DATA_TRUNCATED without consulting errors[], and no comment documents intent. The vendored connector (vendor/mariadb/libmariadb/mariadb_stmt.c:1503-1505) substitutes internal storage when bind->error is NULL, so the array plus its mysqlCleanup parameter can be deleted safely. Not a bug (buffers sized from max_length+1 after store_result make truncation unreachable), just genuine dead weight.

L11. [redundant-conditional] libs/calogDb.c:650 --- FIXED

Status: FIXED

mysqlQuery re-nils value inside the isNull branch although it was already nil'd at the top of the column loop.

Line 646 runs calogValueNil(&value); unconditionally, then lines 649-651 read if (isNull[column]) { calogValueNil(&value); } -- the inner call is a guaranteed no-op re-set of an already-nil value (exactly the "don't check/re-set what is already set" pattern). The if/else itself is needed to skip mysqlColumn; the body of the true branch is not. Make the branch if (!isNull[column]) { ...NUL-terminate + mysqlColumn...; } with no else.

Verifier: Confirmed in libs/calogDb.c: line 646 unconditionally nils value at the top of the column loop, and the only intervening call (calogValueString on line 647) writes key, not value; calogValueNil (src/value.c:446) is a pure memset+type setter, so the inner calogValueNil at line 650 is a guaranteed no-op. Rewriting as if (!isNull[column]) { NUL-terminate + mysqlColumn } is behavior-preserving, and the sibling pgColumn (line 698-701) already uses the nil-once pattern, showing no documented intent behind the duplicate.

L12. [dead-code] libs/calogDb.c:846 --- FIXED

Status: FIXED

pgRun's resultFormat parameter is a constant 0 at both call sites, making it a dead parameter.

static PGresult *pgRun(PGconn *conn, ..., int resultFormat, CalogValueT *result) is called only from pgExec line 746 and pgQuery line 785, both as pgRun(conn, sqlValue, params, paramCount, 0, result). No code path ever passes a nonzero value and pgColumn is written exclusively for text-format results (it parses "\x" hex bytea and strtoll/strtod on text), so binary results (resultFormat=1) would be misdecoded anyway. Drop the parameter and pass 0 directly to PQexecParams at line 906.

Verifier: pgRun (libs/calogDb.c:846, prototype line 79) is static with exactly two callers (pgExec:746, pgQuery:785), both passing literal 0 for resultFormat; the value is only forwarded to PQexecParams at line 906. pgColumn (line 693) is commented "text result format" and decodes only text representations (strtoll/strtod, "\x" hex bytea), so a nonzero resultFormat could never be used correctly without rewriting the decoder. No comment marks the parameter as a deliberate extension point. Concrete cleanup: remove the parameter and pass 0 directly.

L13. [bug] libs/calogHttp.c:735 --- FIXED

Status: FIXED

httpParseUrl misparses IPv6 literal URLs: the host scan stops at the first ':' inside the brackets.

while (pos < len && url[pos] != ':' && url[pos] != '/') { pos++; } has no '[' handling, so httpGet("http://[::1]:8080/") parses host="[" and port=":1]:8080", and getaddrinfo fails with a baffling resolver error instead of connecting. The connect path is otherwise fully IPv6-capable (hints.ai_family = AF_UNSPEC, line 382), so this is the only thing blocking IPv6-literal endpoints, and nothing documents them as unsupported. Fix: when url[pos]=='[', scan to the matching ']' for the host (stripping brackets for getaddrinfo/SNI) before looking for ':' -- or explicitly reject bracket hosts with a clear error.

Verifier: Verified in libs/calogHttp.c: the host scan at line 735 stops at the first ':' with no '[' handling, so 'http://[::1]:8080/' parses host="[" and port=":1]:8080", and getaddrinfo (line 384) fails with a raw gai_strerror message (line 386). No comment documents IPv6 literals as unsupported (file header, lines 1-8, is silent on it), and the connect path is AF_UNSPEC (line 382), so the parser is the sole blocker as claimed.

L14. [multiple-truth] libs/calogHttp.c:759 --- FIXED

Status: FIXED

The default ports "80" and "443" are maintained as string literals in two functions that can drift.

httpParseUrl fills in the default with strcpy(out->port, out->https ? "443" : "80"); (line 759) while httpDefaultPort independently re-encodes the same facts as return strcmp(url->port, "443") == 0; / return strcmp(url->port, "80") == 0; (lines 566-569). If either literal is ever touched (or a scheme added), the Host-header port-suppression logic in httpPerform (line 806) silently disagrees with URL parsing. Fix: #define HTTP_PORT_HTTP "80" / #define HTTP_PORT_HTTPS "443" next to the other HTTP_* constants and use them in both places.

Verifier: Verified in libs/calogHttp.c: line 759 hardcodes "443"/"80" as defaults in httpParseUrl while lines 565-570 (httpDefaultPort) independently hardcode the same scheme->port mapping, and line 806 uses httpDefaultPort to decide Host-header port suppression -- so the two sites must stay in agreement but share no constant. No comment documents this as intentional, and a constants block (HTTP_PORT_MAX, line 31) already exists for the proposed defines. Not a bug today (80/443 are RFC-fixed and both sites agree), so it is a real but minor multiple-sources-of-truth cleanup.

L15. [bug] libs/calogHttp.c:793 --- FIXED

Status: FIXED

Raw CR/LF in the method string or URL path is copied verbatim into the request line, enabling request splitting.

httpPerform builds the request with status = httpBufPutStr(&request, method); (line 793) and httpBufPutStr(&request, url.path) (line 798), and httpParseUrl copies path bytes unvalidated (httpCopyField(out->path, ..., url + pos, len - pos), line 762). Neither rejects CR, LF, or SP, so httpRequest({method: "GET / HTTP/1.1\r\nX-Evil: 1\r\n\r\nGET", url: ...}) -- or a URL assembled from untrusted input containing a raw "\r\n" -- injects extra headers or a second smuggled request. The header docs explicitly bless arbitrary octets in header VALUES only; method and path carrying CR/LF is neither documented nor useful. Fix: fail calogErrArgE if method or path contains '\r', '\n', or (for method) ' '.

Verifier: Confirmed in libs/calogHttp.c: httpPerform writes method (line 793) and url.path (line 798) verbatim into the request line; method comes unfiltered from the script (line 940, method = methodValue->as.s.bytes) and httpParseUrl copies path bytes via httpCopyField (lines 761-764), which is a bare memcpy+NUL (lines 497-504). No CR/LF/SP validation exists anywhere on the path, so raw "\r\n" in the method or URL path injects headers or a smuggled second request. The only binary-octets blessing in the code is the comment at lines 99-100, which covers header VALUES only, so this is not a documented tradeoff for the request line.

L16. [bug] libs/calogHttp.c:830 --- FIXED

Status: FIXED

A POST/PUT with an empty or absent body sends no Content-Length header at all, which servers may reject with 411.

if (bodyLen > 0) { ... snprintf(lengthLine, ..., "Content-Length: %lld\r\n", ...); } means httpRequest({method: "POST", url: ..., body: ""}) (or a bodyless POST) emits a POST with neither Content-Length nor Transfer-Encoding. RFC 9110/7230 says a client SHOULD send Content-Length: 0 for request methods that expect a body; real servers (and strict proxies) answer 411 Length Required or hang waiting for framing. Concrete failure: empty-body POSTs to compliant-but-strict endpoints fail even though the same request works from curl. Fix: emit "Content-Length: 0\r\n" whenever the method is not GET/HEAD (or simply always when body was provided, even empty).

Verifier: Confirmed at libs/calogHttp.c:830: if (bodyLen > 0) is the sole gate on emitting Content-Length, and httpRequestNative maps both an absent body and body:"" to bodyLen=0, so an empty-body POST/PUT is sent with neither Content-Length nor Transfer-Encoding. No comment or header-contract text documents this as intentional. This violates RFC 9110 8.6 (UA SHOULD send Content-Length: 0 on POST) and strict servers/proxies do answer 411 Length Required. However, the claim's 'hang waiting for framing' scenario is wrong -- a request with neither header has no body by HTTP framing rules and is valid -- and a per-call workaround exists because httpAppendHeaders passes a caller-supplied Content-Length: 0 through verbatim. Real but minor interop gap, correctly filed as low.

L17. [redundant-conditional] libs/calogJson.c:422 --- FIXED

Status: FIXED

Redundant frees/nil-sets: jsonParseNative pre-frees result before calogFail (which frees it again), and fsMkdir/fsStat re-nil an already-nil result.

calogFail (src/broker.c:82) begins with calogValueFree(result), so the calogValueFree(result); at calogJson.c:422 and :427 immediately before return calogFail(...) is redundant -- and inconsistent, since no other native in these four files pre-frees. Same category: fsMkdirNative sets calogValueNil(result) at calogFs.c:161 and again at :170 and :175 with result untouched in between; fsStatNative repeats it at calogFs.c:307; and fsPutFile's total = (length > 0) ? (size_t)length : (size_t)0; (calogFs.c:194) guards against a negative length that calogValueString (src/value.c) already makes impossible for string values. All are just-set-it cases; delete the redundant calls.

Verifier: Verified in code: calogFail (broker.c:85) starts with calogValueFree(result), and calogValueFree nils after freeing (value.c:418), so the pre-frees at calogJson.c:422/:427 (and also calogKv.c:168, which the claim missed) are redundant no-ops. fsMkdirNative re-nils an untouched result at calogFs.c:170/175 after niling at 161; fsStatNative does the same at 307 after 299. fsPutFile's negative-length guard at calogFs.c:194 is unreachable: its only two callers pass string-value lengths, and calogValueString (value.c:461) is the sole string producer and rejects length < 0. No comments mark any of this as intentional. No runtime defect exists (double-free is impossible), so this is pure cleanup that matches the owner's stated review criteria.

L18. [bug] libs/calogJson.c:450 --- FIXED

Status: FIXED

jsonParseNumber accepts non-JSON number forms ("+1", ".5", "5.", "01") because validation is delegated to strtod/strtoll, which are more permissive than RFC 8259.

The manual scan } else if (c == '.' || c == 'e' || c == 'E' || c == '+' || c == '-') { isReal = true; ... } consumes any sign/dot/exponent character anywhere, and correctness then rests on stop != p->cur after strtod/strtoll. But strtod accepts a leading '+', a bare leading '.', a trailing '.', and strtoll accepts leading zeros, so jsonParse("+1") yields real 1.0, jsonParse(".5") yields 0.5, jsonParse("5.") yields 5.0, and jsonParse("01") yields int 1 -- all of which every conforming JSON parser rejects. Unlike the int64-overflow-to-real behavior (documented at lines 477-478) this leniency is undocumented, so it reads as unintentional. Related: the strtod/strtoll calls rely on the input's trailing NUL (guaranteed today by calogValueString) even though the struct comment at line 25 claims the parser is 'bounded by end, not a NUL' -- worth a comment or an explicit grammar check so a future zero-copy string path cannot read past end.

Verifier: Verified by tracing libs/calogJson.c: jsonParseValue's default case (line 697) routes '+' and '.' into jsonParseNumber; the scan loop (lines 445-456) consumes sign/dot/exponent chars anywhere, and the only validation is stop == p->cur after strtod/strtoll. strtod accepts "+1", ".5", and "5." with stop landing exactly at p->cur, and strtoll accepts "01", so all four non-RFC-8259 forms parse successfully (confirmed against the exact code, including the trailing-junk check at line 426 which they pass). The leniency is undocumented -- the only documented number tradeoff is the int64-overflow comment at lines 477-478 -- and the scan deliberately blocks inf/nan/hex, showing strict validation was intended, so these are genuine gaps. Secondary point also confirmed: strtod/strtoll rely on NUL termination (guaranteed by calogValueString, src/value.c:477) contradicting the "bounded by end, not a NUL" comment at line 25, a latent hazard for any future zero-copy string. No memory unsafety or data loss today; valid JSON is unaffected, so severity is low.

L19. [duplication] libs/calogKv.c:257 --- FIXED

Status: FIXED

The grow-the-array block is copy-pasted into four in-scope files (plus calogHandle.c), each with its own INITIAL_CAP/GROWTH defines, while src/value.c already contains a shareable ensureCapacity() that even has the overflow guards these copies lack.

kvSet lines 257-268 (if (gCount == gCap) { newCap = (gCap == 0) ? KV_INITIAL_CAP : gCap * KV_GROWTH; grown = realloc(...); ... }) is byte-for-byte the same idiom as pubsubSubscribe (calogPubsub.c:182-193), exportPublish (calogExport.c:138-148), timerSchedule (calogTimer.c:243-253), and calogHandleAdd (calogHandle.c:41-56), each declaring its own *_INITIAL_CAP 8 / *_GROWTH 2 pair (calogKv.c:25-26, calogPubsub.c:30-31, calogTimer.c:29-30, calogExport.c:21-22, calogHandle.c:8-9). Meanwhile src/value.c:299 has a static ensureCapacity(void **buffer, int64_t *cap, int64_t needed, size_t elemSize) that additionally guards INT64/SIZE_MAX overflow (lines 308-315). Exposing that helper (or adding a libs-shared one) collapses five copies into one and gives every registry the overflow checks for free -- exactly the group-into-shared-routines cleanup requested.

Verifier: Verified all five copies of the identical grow-the-array block exist as claimed (calogKv.c:257-269, calogPubsub.c:182-194, calogExport.c:138-149, calogTimer.c:243-254, calogHandle.c:42-53), each with its own duplicate INITIAL_CAP=8/GROWTH=2 macro pair, and src/value.c:299 does contain a static ensureCapacity() with INT64/SIZE_MAX overflow guards. The duplication is a ~12-line pattern times five files plus ten redundant macros, not trivial boilerplate, and no comment documents intentional self-containment; the libs already share infrastructure (calogHandle itself), so a shared helper fits. Caveats: ensureCapacity is not drop-in (static, int64_t caps vs the libs' int32_t, different growth policy), and the missing overflow guards are unreachable in practice (int32 cap overflow requires >2^30 live entries), so this is a pure maintainability cleanup with no concrete failure mode.

L20. [error-path] libs/calogNet.c:101 --- FIXED

Status: FIXED

calogNetRegister ignores all 16 calogRegisterInline return values, so a registration OOM silently produces a runtime with some natives missing.

calogRegisterInline returns int32_t (src/calog.h:145), but lines 101-116 discard it 16 times: calogRegisterInline(calog, "tcpConnect", tcpConnect, gNetLib); etc. If one registration fails (OOM), calogNetRegister still returns calogOkE and the script later gets a confusing unknown-native error at call time instead of a clean startup failure -- worse, the refCount was already incremented so the partial registration is never rolled back. (calogTask.c and calogHttp.c share this pattern, so fixing it belongs in one place -- e.g. a shared table-driven register helper.) Secondary cleanup on the same lines: after unlocking gNetLibMutex at line 100, the code re-reads the global gNetLib 16 times instead of capturing it into a local pointer while the lock was held.

Verifier: Verified: calogRegisterInline can fail with calogErrOomE (broker.c registerEntry, brokerGrow calloc at :125 and strdup at :186), calogNet.c:101-116 discards all 16 return values and line 117 unconditionally returns calogOkE, contradicting calogNet.h:35's documented 'Returns calogOkE or an error' contract and defeating the fail-fast checks callers actually perform (calogMain.c:438 treats a non-OK return as fatal, and calogMain.c:430 checks its own direct calogRegisterInline call, proving the intended style). The refCount at calogNet.c:99 is incremented with no rollback on registration failure. The pattern repeats unchecked across all libs (76 calls: calogHttp.c:89, calogTask.c:98, calogDb.c:109, etc.), so a shared table-driven register helper has genuine consolidation payoff. The post-unlock re-read of gNetLib 16 times is accurate but unobservable in practice (registration is single-threaded startup). Severity stays low because the only trigger is malloc failure during startup registration, and the consequence is a confusing late unknown-native error rather than memory unsafety or data loss.

L21. [duplication] libs/calogNet.c:458 --- FIXED

Status: FIXED

udpClose is a copy of tcpClose minus one type tag, and the port-range validation is repeated six times; both should be shared helpers.

udpClose (lines 458-474) and tcpClose (lines 308-327) are identical except for the type tags tried and the error strings: remove from table, close(sock->fd); free(sock);. One netSocketClose(lib, handle, type1, type2, label) helper (or a single script-facing close that tries all three socket tags) removes the duplication -- and its body already exists a third time as the socket arm of netCloser (lines 142-147). Separately, the exact check args[N].as.i < 0 || args[N].as.i > NET_PORT_MAX appears six times (lines 342, 379, 487, 569, 635, 695); a tiny netPortOk(int64_t) helper would collapse them. The user's stated goal of grouping similar functionality makes both worth doing while the file is small.

Verifier: Verified in libs/calogNet.c: udpClose (458-474) and tcpClose (308-327) are ~13-line structural clones differing only in handle type tags and error-string prefix, with the close/free body appearing a third time in netCloser (143-146); the port-range check appears verbatim at exactly the six cited lines (342, 379, 487, 569, 635, 695). No comment documents keeping them separate, and the file already uses shared helpers (netStore, netOpenBound, netMapSet*), so a netSocketClose helper fits the existing pattern without changing the script API. The port-check half is marginal since NET_PORT_MAX is already a named constant and each site still needs its own error message, but the close-function consolidation is a concrete, worthwhile cleanup per the user's explicit group-shared-functionality instruction.

L22. [magic-number] libs/calogNet.c:733 --- FIXED

Status: FIXED

enetSend validates the channel against a bare 255 instead of ENet's own ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT constant.

if (args[1].as.i < 0 || args[1].as.i > 255) at line 733 hard-codes the ENet channel ceiling. The vendored ENet exposes ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT (== 255) in enet/protocol.h; using it (or a local NET_ENET_CHANNEL_MAX defined from it) keeps the limit tied to the library that enforces it, matching how the file already names NET_PORT_MAX and NET_MAX_RECV instead of using bare literals.

Verifier: Line 733 of libs/calogNet.c does hard-code 255 as the enetSend channel ceiling, the vendored enet/protocol.h:18 defines ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT = 255 and is already transitively included via <enet/enet.h>, and the file's own convention names equally obvious limits (NET_PORT_MAX = 65535, NET_MAX_RECV) at the top of the file -- making this lone bare literal an inconsistency worth fixing. However, it is purely cosmetic: the value is correct, ENet's enet_peer_send independently rejects out-of-range channel IDs, and the (enet_uint8) cast at line 748 makes 255 the truncation-safe bound, so there is no reachable misbehavior.

L23. [bug] libs/calogNet.c:777 --- FIXED

Status: FIXED

enetService silently truncates the int64 timeout to enet_uint32; a script passing a value >= 2^32 ms gets a wrapped (possibly 0ms) timeout instead of an error.

Line 770 checks only the lower bound (if (args[1].as.i < 0)), then line 777 does enet_host_service(host, &event, (enet_uint32)args[1].as.i). A script that passes a large "wait forever" sentinel such as 2^32 (4294967296) gets timeout 0 -- a busy-poll instead of a block -- and any value above UINT32_MAX wraps modulo 2^32 with no diagnostic. This is inconsistent with the rest of the file, where every other narrowing conversion is range-checked first (ports against NET_PORT_MAX at lines 342/379/487/569/635/695, channel against 255 at line 733). Add an upper-bound check (e.g. clamp to UINT32_MAX or reject) before the cast.

Verifier: Confirmed in libs/calogNet.c: line 770 validates only args[1].as.i < 0, then line 777 casts the int64 timeout straight to enet_uint32 in enet_host_service(), so 2^32 wraps to a 0ms busy-poll and larger values wrap mod 2^32 with no diagnostic. No comment documents this as intentional, and it breaks the file's own convention of range-checking every narrowing cast (NET_PORT_MAX checks at lines 342/379/487/569/635/695, channel<=255 at line 733 before the enet_uint8 cast at 748). Real bug, but only reachable with a timeout >= ~49.7 days, and the effect is a wrong wait duration, not memory unsafety.

L24. [multiple-truth] libs/calogNet.c:810 --- FIXED

Status: FIXED

String literals and their hand-counted lengths are maintained as two separate facts in every netMapSetStr call for fixed keys/values.

netMapSetStr(map, "type", "connect", 7) (line 810), "receive", 7 (813), "disconnect", 10 (823), and "none", 4 (786, 829): the length literal duplicates the string and will drift if anyone edits the text (a change to "disconnected" with a stale 10 silently truncates the value). netMapSetStr already derives the KEY length via strlen internally (line 187), so the value length for C-string literals should be derived too -- either a netMapSetLit(map, key, lit) wrapper using sizeof(lit) - 1/strlen, or have callers pass strlen for literals as line 543 already does for hostBuffer. Same pattern exists in the sibling http/ssh copies, so fold it into the shared helper from the duplication finding.

Verifier: Confirmed at calogNet.c lines 786/810/813/823/829: string literals paired with hand-counted lengths (e.g. "disconnect", 10), while netMapSetStr already strlen()s the key (line 187) and line 543 shows the strlen-for-values idiom already in use; editing a literal with a stale count would silently truncate the value with no error. All current counts are correct, so no live bug. Claim overstates scope: calogHttp.c/calogSsh.c do NOT have this pattern (all their value lengths are computed), so the cleanup is five sites in calogNet.c only.

L25. [dead-code] libs/calogNet.c:828 --- FIXED

Status: FIXED

The default case in enetService's event switch is unreachable, and if it could run it would emit a malformed "none" event carrying a peer handle.

Line 785 already returns for serviced == 0 || event.type == ENET_EVENT_TYPE_NONE, and ENetEventType has exactly four values (NONE/CONNECT/RECEIVE/DISCONNECT), so the default: status = netMapSetStr(map, "type", "none", 4); break; at lines 828-830 can never execute. It is also wrong-by-construction: control would fall through to lines 832-834 which add a "peer" key (and lines 797-806 may have just added a fresh peer handle to the table for it), whereas the real "none" result built at line 786 has no peer key -- so this is not a harmless defensive default. Delete it or turn it into an explicit internal-error return.

Verifier: Verified: line 785 returns early for serviced==0 || ENET_EVENT_TYPE_NONE, and the vendored enet.h enum has exactly four values (NONE/CONNECT/DISCONNECT/RECEIVE), the three non-NONE ones all having explicit cases -- so the default at lines 828-830 is unreachable. It is also not a safe defensive default: it falls through to lines 832-834 which add a "peer" key (after lines 797-806 possibly allocated a fresh peer handle), yielding a {type:"none", peer:N} shape that contradicts the real peer-less "none" event at line 786. Caveat: the build uses -Wall -Wextra -Werror, so the default also serves to satisfy -Wswitch and keep status initialized; the fix must be an explicit internal-error return (the claim's second option), not bare deletion.

L26. [duplication] libs/calogPubsub.c:68 --- FIXED

Status: FIXED

The refcounted-static-registry Register/Shutdown scaffolding is duplicated near-verbatim across calogKv.c, calogPubsub.c, calogExport.c, and (with thread handling added) calogTimer.c.

calogPubsubShutdown lines 68-87 (lock gInitMutex; if (gRefCount > 0) gRefCount--; if (gRefCount <= 0) { lock list mutex; for each entry free...; free(gEntries); gEntries = NULL; gCount = 0; gCap = 0; unlock; } unlock) is structurally identical to calogKvShutdown (calogKv.c:65-84) and calogExportShutdown (calogExport.c:57-76); the matching Register functions (calogKv.c:52-55, calogPubsub.c:57-60, calogExport.c:45-48, calogTimer.c:71-74) are identical too, and each file re-declares the same static pthread_mutex_t gInitMutex + static int32_t gRefCount pair (calogKv.c:40-41, calogPubsub.c:48-49, calogExport.c:35-36, calogTimer.c:57-58). A tiny shared helper -- refcount up; refcount down returning whether this was the last, plus a per-entry free callback -- would collapse four maintained copies of the same lifecycle logic into one.

Verifier: Verified verbatim: calogKvShutdown (calogKv.c:65-84), calogPubsubShutdown (calogPubsub.c:68-87), and calogExportShutdown (calogExport.c:57-76) are identical except for the list-mutex name and two per-entry free calls; the 3-line refcount-up Register preamble and the static gInitMutex+gRefCount pair are repeated identically in all four files, with calogTimer.c:82-125 sharing the same decrement gate plus thread handling. No shared helper exists (grep confirmed), the header comments document only the never-free-static-state tradeoff and not the duplication, and a retain/release-with-cleanup-callback helper invoked under gInitMutex would preserve the exact locking semantics of all four copies (each holds gInitMutex across the whole cleanup, including timer's pthread_join). Four copies of concurrency-sensitive lifecycle logic that has already drifted structurally in the timer variant is a concrete, viable consolidation, though the payoff is bounded because the newer libs (calogTask/Db/Net/Ssh) use a different heap-struct lifecycle pattern, so this pattern is not spreading.

L27. [redundant-conditional] libs/calogPubsub.c:147 --- FIXED

Status: FIXED

No-op defensive calls against contracts the callees already guarantee: pre-nil of callResult before calogFnInvoke, and calogValueFree of a value after a failed calogValueCopy.

calogValueNil(&callResult); at calogPubsub.c:147 is redundant -- calogFnInvoke's first statement is calogValueNil(result) (src/value.c:236), and calogTimer.c:332 already relies on that by passing an uninitialized res. Likewise calogValueFree(&messageCopy); at calogPubsub.c:143, calogValueFree(result); at calogKv.c:168, and calogValueFree(&copy); at calogKv.c:244 all run only after calogValueCopy failed, but valueCopyDepth nils dst before doing anything and every error path leaves it nil (src/value.c:335 and calogValueString's error paths at value.c:461-472), so these frees are guaranteed no-ops. Dropping them (or trusting the documented contract consistently) removes the implied uncertainty about who initializes what.

Verifier: Verified in code: calogFnInvoke's first statement is calogValueNil(result) (src/value.c:236) on all paths, so the pre-nil at calogPubsub.c:147 is a strict no-op, and calogTimer.c:332-333 already relies on that contract by passing an uninitialized res. valueCopyDepth nils dst at entry (src/value.c:335) and every error path leaves it nil (calogValueString nils on all three failure paths at value.c:461-472; the aggregate failure path returns without touching the nil'd dst), so the frees at calogPubsub.c:143, calogKv.c:168, and calogKv.c:244 run only on values that are provably nil, and calogValueFree(nil) is a no-op. calogKv.c:168 is doubly dead because calogFail itself starts with calogValueFree(result) (src/broker.c:85). Concrete cleanup: four dead statements plus a caller/callee output-initialization inconsistency, but zero behavioral impact.

L28. [bug] libs/calogSsh.c:169 --- FIXED

Status: FIXED

Off-by-one at the transfer cap: a file/stream of exactly SSH_MAX_TRANSFER (64MiB) bytes fails with calogErrRangeE even though it does not exceed the limit the comment promises ('Exceeding it fails').

sftpGet lines 166-172 and sshDrain lines 649-654: if (length == cap) { if (cap >= SSH_MAX_TRANSFER) { ... return calogErrRangeE; }. Once the buffer is full at exactly 64MiB (the doubling sequences land exactly on it: 655362^10 and 40962^14 both equal 67108864), the next loop iteration errors out without probing whether the next read would return 0 (EOF). Concrete scenario: sftpGet of an exactly-67108864-byte file fails 'file exceeds the maximum transfer size', contradicting the header comment at lines 32-35. Fix: when the buffer is full at the cap, attempt one read into a small stack probe buffer and only fail if it returns > 0.

Verifier: Confirmed off-by-one at the transfer cap in libs/calogSsh.c. CALOG_GROWTH_FACTOR=2 (src/calog.h:36) makes both growth sequences land exactly on SSH_MAX_TRANSFER (sftpGet: 655362^10=67108864 at line 174; sshDrain: 40962^14=67108864 at line 656). For a source of exactly 67108864 bytes, length reaches cap and the next iteration (lines 166-173 / 649-655) returns calogErrRangeE before any read could observe EOF, so an exactly-64MiB file/stream fails. This contradicts the header comment at lines 32-35 ('Exceeding it fails') and diverges from calogHttp, which the comment claims to match: httpReadResponse (libs/calogHttp.c:886) uses a strictly-greater-than check after reading into a stack chunk, so an exactly-64MiB HTTP response succeeds. No pre-stat or other guard covers the case.

L29. [magic-number] libs/calogSsh.c:174 --- FIXED

Status: FIXED

Bare buffer-size literals: initial drain capacities 65536 (sftpGet:174) and 4096 (sshDrain:656) -- inconsistent between two otherwise-identical loops -- and the 512-byte readdir name buffer (sftpList:240).

Line 174 want = (cap == 0) ? 65536 : cap * CALOG_GROWTH_FACTOR; vs line 656 want = (cap == 0) ? 4096 : cap * CALOG_GROWTH_FACTOR; -- two unnamed initial capacities for the same grow-loop pattern with no comment explaining why they differ (a shared drain routine, see the duplication finding, would force one named constant). Line 240 char name[512]; sizes the libssh2_sftp_readdir filename buffer; a remote entry name longer than 511 bytes makes readdir fail and the whole sftpList aborts with 'read failed' -- the 512 deserves a named constant (e.g. SSH_NAME_MAX) documenting that trade-off.

Verifier: Verified in libs/calogSsh.c: line 174 uses bare 65536 and line 656 bare 4096 as initial capacities in two structurally identical grow-loops with no comment explaining the difference; line 240 uses bare char name[512] and the vendored libssh2 sftp_readdir (vendor/libssh2/src/sftp.c ~1856) confirms a >=512-byte name returns LIBSSH2_ERROR_BUFFER_TOO_SMALL, which sftpList turns into aborting the whole listing. The file itself names SSH_PORT_MAX/SSH_MAX_TRANSFER with comments, and sibling calogHttp.c names exactly these constants (HTTP_BUF_INITIAL, HTTP_READ_CHUNK), so the bare literals break the codebase's own convention. Concrete cleanup, but no runtime bug in practice (NAME_MAX is 255 everywhere real), so severity stays low.

L30. [redundant-conditional] libs/calogSsh.c:519 --- FIXED

Status: FIXED

sshClose's NULL check on calogHandleRemove is unreachable by the code's own documented invariant, duplicating the 'invalid ssh handle' failure a second time.

Lines 510-522: the comment states 'because the owner is a single thread, the peek and the remove below cannot race another context claiming this handle' -- and the owner check at 516 guarantees only this (single-threaded) context can reach the remove for this handle, while table destroy only happens after runtime teardown (calogSsh.h lines 33-35). Given calogHandleGet just returned non-NULL at line 512, conn = calogHandleRemove(...); if (conn == NULL) { return calogFail(..."sshClose: invalid ssh handle"); } at 519-522 is a branch that cannot execute. Drop the check (or assert) so there is one source of the not-found error, not two.

Verifier: Confirmed by reading libs/calogSsh.c:510-522, libs/calogHandle.c:105-125, and calogSsh.h:19-36. calogHandleRemove returns NULL only when the entry is absent or type-mismatched; the peek at line 512 just resolved the same handle/type, the owner check at 516 restricts line 519 to the single owning context thread (calogCurrentId, src/context.c:468), and the only other removal paths are table destroy at shutdown (contractually after runtime teardown, calogSsh.h:33-36) or the pre-registration failure path with an empty table. The comment at 510-511 documents this exact no-race invariant. The one theoretical race (two host threads sharing CALOG_HOST_ID double-closing) already commits use-after-free at line 516 before reaching 520, and the header declares that undefined, so the check is not a valid race guard. Lines 520-522 are unreachable dead code duplicating the "sshClose: invalid ssh handle" error emitted at 513-514.

L31. [duplication] libs/calogSsh.c:523 --- FIXED

Status: FIXED

The connection teardown block (sftp_shutdown, session_disconnect, session_free, close, free) is written out identically in sshClose and sshCloser, and the session-teardown subset is repeated in two sshConnect error paths.

sshClose lines 523-529 and sshCloser lines 539-545 differ only in the disconnect string ("calog: closing" vs "calog: shutdown"); sshConnect lines 614-616 and 625-627 repeat libssh2_session_disconnect(session, "out of memory"); libssh2_session_free(session); close(fd);. One static void sshConnDestroy(SshConnT *conn, const char *reason) (plus letting sshCloser and sshClose call it) keeps the resource-release order in a single place, per the stated goal of grouping shared functionality -- and the two sshConnect OOM paths could share a single cleanup tail.

Verifier: Verified: sshClose (calogSsh.c:523-529) and sshCloser (:539-545) contain a byte-identical 7-line teardown (conditional sftp_shutdown, session_disconnect, session_free, close, free) differing only in the disconnect reason string, with no comment documenting the duplication as intentional; sshConnect repeats the session+fd teardown at :614-616 and :625-627. The sibling library calogDb.c:153 already uses the exact proposed pattern (dbClose delegates to dbCloser after handle removal), so a shared sshConnDestroy(conn, reason) is a concrete, precedented cleanup that keeps the ordering-sensitive release sequence in one place. No runtime defect, purely duplication/maintainability.

L32. [dead-code] libs/calogSsh.c:673 --- FIXED

Status: FIXED

The four LIBSSH2_ERROR_EAGAIN 'continue' branches are dead: the session is put in blocking mode (line 606), and libssh2 documents that EAGAIN is never returned to the caller in blocking mode (BLOCK_ADJUST retries internally).

sftpGet lines 192-193 (} else if (n == LIBSSH2_ERROR_EAGAIN) { continue; }), sftpList lines 247-249, sftpPut lines 352-354, and sshDrain lines 673-674 all handle an error code that libssh2_sftp_read/libssh2_sftp_readdir/libssh2_sftp_write/libssh2_channel_read_ex cannot return once libssh2_session_set_blocking(session, 1) is set (sshConnect line 606; libssh2's blocking wrappers loop on EAGAIN internally via BLOCK_ADJUST). Four unreachable branches; worse, if the code ever switched to non-blocking they would busy-spin with no poll/select. Either delete them or leave one commented explanation -- currently they read as if EAGAIN were a live path in this file.

Verifier: Confirmed: calogSsh.c sets blocking mode once (line 606, the file's only set_blocking call) and never reverts it; the vendored libssh2 wraps libssh2_sftp_read/readdir/write and libssh2_channel_read_ex in BLOCK_ADJUST (vendor/libssh2/src/session.h:55), which in blocking mode only exits via a successful retry or a _libssh2_wait_socket error, and _libssh2_wait_socket cannot return EAGAIN (keepalive.c:85 explicitly swallows it; other returns are 0/TIMEOUT/socket errors). So the four EAGAIN 'continue' branches at lines 192, 247, 352, and 673 are unreachable dead code with no documenting comment. Not a bug (branches are harmless), just cleanup the codebase owner explicitly asks for.

L33. [dumb-code] libs/calogSsh.c:727 --- FIXED

Status: FIXED

sshExec validates the command length only AFTER opening a channel, wasting a network round trip (and a channel) on an input that is rejected locally.

Lines 723-730: channel = libssh2_channel_open_session(conn->session); ... if (args[1].as.s.length > SSH_MAX_TRANSFER) { libssh2_channel_free(channel); return calogFail(..."sshExec: command too long"); }. The length check depends only on the argument, so it belongs with the other argument validation at line 716, before the (blocking, remote round-trip) channel open. Work is done and then thrown away on the reject path.

Verifier: Code at libs/calogSsh.c:723-730 matches the claim: libssh2_channel_open_session (blocking mode, set at line 606, so a real server round trip) runs before the length check at 727, which depends only on args[1].as.s.length vs the compile-time constant SSH_MAX_TRANSFER (line 35). No session/channel state is needed for the check and no comment documents the ordering as intentional; the check belongs with the argument validation at line 716. The reject path frees the channel correctly, so it is purely wasted work, only reachable with a >64 MiB command string.

L34. [error-path] libs/calogSsh.c:741 --- FIXED

Status: FIXED

sshExec reports 'out of memory' for every sshDrain failure, but sshDrain also fails with calogErrRangeE (output exceeds 64MB) and calogErrArgE (channel read failed), so the script author gets the wrong diagnosis.

Lines 739-741 and 744-747: status = sshDrain(...); if (status != calogOkE) { ... return calogFail(result, status, "sshExec: out of memory"); }. sshDrain returns calogErrRangeE at line 654 when the stream exceeds SSH_MAX_TRANSFER and calogErrArgE at line 677 on a read error; both surface to the script as 'sshExec: out of memory' (with the correct status code but a contradicting message). Concrete scenario: a command whose stdout is 100MB fails with a message claiming memory exhaustion instead of the transfer cap, sending the user debugging the wrong thing. Map the three statuses to distinct messages (like sftpGet does at lines 172/182/197) or have sshDrain produce the message.

Verifier: sshDrain in libs/calogSsh.c returns calogErrRangeE (cap >= SSH_MAX_TRANSFER), calogErrOomE (realloc fail), or calogErrArgE (channel read error), and both sshDrain call sites in sshExec pass the status through calogFail with the fixed message "sshExec: out of memory"; calogFail (src/broker.c:82) puts that string into result, so a >64MB stdout surfaces to the script as an OOM message with a Range status. sftpGet in the same file proves the intended convention is distinct messages per failure mode. However, the status code itself is forwarded correctly and nothing is memory-unsafe or lossy -- only the diagnostic string is wrong -- so this is minor.

L35. [multiple-truth] libs/calogTask.c:41 --- DECLINED

Status: DECLINED -- gTaskEngines uses intentional short aliases ("js"/"s7"), a different namespace from the vtables' canonical names ("javascript"/"scheme"); unifying would break taskSpawn("js"). Invalid as a mechanical dedup.

gTaskEngines restates each engine's name as a string literal even though every CalogEngineT vtable already carries its canonical .name, so the two can drift.

The table entries like { "lua", &calogLuaEngine } and { "wren", &calogWrenEngine } (lines 41-64) duplicate the name field already stored first in each vtable (e.g. src/lua/luaEngine.c:20 "lua", src/wren/wrenEngine.c:19 "wren" -- CalogEngineT.name, calog.h:126). If an engine is renamed or added with a mismatched literal, taskSpawn's lookup silently diverges from the engine's own name used elsewhere. The #ifdef guards must stay (to avoid linking unselected vtables), but the table can shrink to static const CalogEngineT *gTaskEngines[] of vtable pointers, with taskEngineByName matching strcmp(gTaskEngines[index]->name, name), leaving one source of truth.

Verifier: gTaskEngines (libs/calogTask.c:41-64) does duplicate each vtable's .name, and the predicted drift has already occurred: the table says "js" and "s7" while calogJsEngine.name is "javascript" (src/js/jsEngine.c:19) and calogS7Engine.name is "scheme" (src/s7/s7Engine.c:19); no comment documents this as intentional aliasing. However the impact is capped: CalogEngineT.name has no functional consumer anywhere (dispatch uses .extensions), so nothing misbehaves at runtime, and the claim's exact refactor (match on vtable ->name) would break the tested taskSpawn('js'/'s7') API (tests/testTask.c:151) unless the vtable names are renamed first -- the fix needs a canonical-name decision, not a mechanical table collapse.

L36. [duplication] libs/calogTask.c:280 --- FIXED

Status: FIXED

taskSpawn and taskLoad duplicate the entire wrap-context-into-handle tail (malloc TaskT, set fields, calogHandleAdd, and the free/close error unwind).

taskLoad lines 228-242 and taskSpawn lines 280-299 both do: task = (TaskT *)malloc(sizeof(*task)); if (task == NULL) { calogContextClose(context); return calogFail(result, calogErrOomE, ...); } task->context = context; task->ownerId = calogCurrentId(); handle = calogHandleAdd(lib->handles, TASK_TYPE_CONTEXT, task); if (handle == 0) { free(task); calogContextClose(context); return calogFail(result, calogErrOomE, ...); } -- identical except for the message prefix. A shared static helper (e.g. taskWrapContext(lib, context, result, label) returning the handle or 0 after unwinding) removes the second copy; taskSpawn keeps only its extra calogContextEval step and its unwind.

Verifier: Verified in libs/calogTask.c: taskLoad lines 228-241 and taskSpawn lines 280-292 are identical ~13-line wrap-context tails (malloc TaskT, close-context-on-OOM unwind, set context/ownerId, calogHandleAdd, free+close unwind on handle failure, calogValueInt) differing only in the "taskLoad:"/"taskSpawn:" message prefix. taskSpawn's only unique code is the trailing calogContextEval and its unwind, which the claim correctly excludes. This exceeds trivial boilerplate because it duplicates a multi-branch error-unwind sequence that can drift, and the proposed helper needs no awkward parameters beyond a label. Both copies are currently correct, so it is purely a duplication cleanup.

L37. [magic-number] libs/calogTime.c:77 --- FIXED

Status: FIXED

Nanosecond/millisecond conversion factors are bare literals repeated across timeSleepNative and timeToSeconds.

request.tv_sec = (time_t)(nanos / 1000000000); and request.tv_nsec = (long)(nanos % 1000000000); (lines 77-78), ms * 1000000.0 (line 76), and / 1000000000.0 in timeToSeconds (line 90) repeat the same conversion constants as bare literals -- the nanoseconds-per-second value appears three times in one small file. Named constants (e.g. TIME_NS_PER_SEC, TIME_NS_PER_MS) would remove both the magic numbers and the duplicated truth, and give the timeSleep overflow fix (see the calogTime.c:76 finding) a clean bound to test against.

Verifier: Confirmed in libs/calogTime.c: bare 1000000.0 (line 76) and 1000000000 three times (lines 77, 78, 90); grep shows the same bare literals repeated four more times in libs/calogTimer.c (lines 202, 241, 304, 305) with no named constant anywhere in non-vendor code -- seven copies of the same conversion factors across two files, and the project's own review guidelines explicitly target magic numbers and duplicated sources of truth. No runtime defect, purely maintainability, so severity remains low.

L38. [multiple-truth] libs/calogTimer.c:139 --- FIXED

Status: FIXED

Two different validation policies for the same kind of library-issued id argument: timerCancel demands calogIntE only, while psUnsubscribe accepts int-or-real and coerces.

timerCancel: if (argCount != 1 || args[0].type != calogIntE) (line 139) rejects a real. pubsubUnsubscribe: if (argCount != 1 || (args[0].type != calogIntE && args[0].type != calogRealE)) then id = (args[0].type == calogIntE) ? args[0].as.i : (int64_t)args[0].as.r; (calogPubsub.c:215-218). Both ids are produced identically via calogValueInt. Either the coercion is needed (an engine or script arithmetic can hand the id back as a real -- then timerCancel is broken for that caller) or it is not (adapters normalize integral numbers to calogIntE, e.g. src/wren/wrenAdapter.c:526-527 -- then pubsub's real branch is dead defensive code). Pick one policy and put it in a shared getIdArg helper so the two natives cannot disagree.

Verifier: Confirmed by reading both files: calogTimer.c:139 requires calogIntE only while calogPubsub.c:215-218 accepts calogIntE-or-calogRealE and truncate-coerces, for ids minted identically via calogValueInt (timer:271, pubsub:204). Neither file's tradeoff-documenting header comments mention the divergence. The inconsistency is script-observable: the Lua adapter (luaAdapter.c:461-464) emits calogRealE for integral floats without normalizing (unlike JS jsAdapter.c:524 and Wren wrenAdapter.c:526-527), so a Lua script that float-ifies an id succeeds with psUnsubscribe but gets calogErrArgE from timerCancel. calogTimer.c itself accepts int-or-real for ms (line 231), so strict-int is not even file-consistent. Real two-policies-for-one-concept defect with a cheap fix, but the failure path requires unusual script arithmetic and normal use is unaffected.

L39. [magic-number] libs/calogTimer.c:202 --- FIXED

Status: FIXED

Nanosecond conversion factors are repeated as bare literals in three places instead of named constants.

return (int64_t)ts.tv_sec * 1000000000 + (int64_t)ts.tv_nsec; (line 202), delayNs = (int64_t)(ms * 1000000.0); (line 241), and target.tv_sec = (time_t)(fireAt / 1000000000); target.tv_nsec = (long)(fireAt % 1000000000); (lines 304-305) restate the same two conversion facts four times. Named constants (e.g. #define NS_PER_SEC 1000000000LL and #define NS_PER_MS 1000000.0) next to the existing TIMER_INITIAL_CAP defines make the arithmetic self-describing and keep the factor in one place. (The 9.0e12 ms bound at line 238 is fine -- it is explained by the comment above it.)

Verifier: Verified all cited sites in libs/calogTimer.c: bare 1000000000 (ns/sec) appears at line 202 (composing the monotonic timestamp) and twice at lines 304-305 (decomposing fireAt for pthread_cond_timedwait), and bare 1000000.0 (ns/ms) at line 241. The compose/decompose sites must agree, making this a real single-source-of-truth issue, and the 1e6 factor is implicitly coupled to the 9.0e12 bound justified in the line 235-237 comment. The file already uses named #defines (TIMER_INITIAL_CAP, TIMER_GROWTH), so the proposed NS_PER_SEC/NS_PER_MS constants fit the established convention. No functional bug -- arithmetic and int64 promotion are correct -- so this is purely a maintainability cleanup.

L40. [magic-number] src/berry/berryAdapter.c:28 --- PARTIAL

Status: PARTIAL -- the 256-byte error cap was unified into CALOG_ERR_MSG_CAP and the lua/squirrel binding initials into adapterBinding.h + calogGrow. The remaining BERRY_FOREIGN_INITIAL / MB_INITIAL_ARGS are distinct arrays that merely share the value 8, left as separate named constants (merging would be false coupling).

The same numeric facts are re-defined per adapter under different names: the 256-byte error-message cap four times, the initial capacity 8 four times, and the marshal stack reserve 8 twice -- each should be one shared constant beside CALOG_MAX_DEPTH/CALOG_GROWTH_FACTOR.

The 256 error-truncation cap: #define JS_ERR_MSG_CAP 256 (jsAdapter.c:33), #define BERRY_ERR_CAP 256 (berryAdapter.c:28), #define S7_ERR_CAP 256 (s7Adapter.c:29), #define WREN_ERR_CAP 256 (wrenAdapter.c:26) -- four names for one fact (and note Lua/Squirrel pass full-length messages with no cap, so the cap is also silent behavioral drift for long native error strings). Initial capacity 8: #define LUA_BINDING_INITIAL 8 (luaAdapter.c:21), #define SQUIRREL_BINDING_INITIAL 8 (squirrelAdapter.c:22), #define BERRY_FOREIGN_INITIAL 8 (berryAdapter.c:30), #define MB_INITIAL_ARGS 8 (mybasicAdapter.c:19). Marshal-slot reserve 8: #define LUA_MARSHAL_SLOTS 8 (luaAdapter.c:23), #define SQUIRREL_MARSHAL_SLOTS 8 (squirrelAdapter.c:24). src/calog.h already centralizes CALOG_MAX_DEPTH (calog.h:32) and CALOG_GROWTH_FACTOR (calog.h:36); add CALOG_ERR_MSG_CAP, CALOG_INITIAL_CAP, and CALOG_MARSHAL_SLOTS there so retuning one of these does not require a seven-file audit.

Verifier: Partially confirmed. The 256 error-cap portion is real: JS_ERR_MSG_CAP (jsAdapter.c:33), BERRY_ERR_CAP (berryAdapter.c:28), S7_ERR_CAP (s7Adapter.c:29), and WREN_ERR_CAP (wrenAdapter.c:26) all back the identical duplicated pattern (snprintf payload into a fixed buffer, calogValueFree, raise) implementing one cross-adapter policy, and the behavioral-drift sub-claim is verified in code: luaAdapter.c:606-612 delivers the full-length payload via lua_pushlstring and squirrelAdapter.c:662-664 passes the full string to sq_throwerror, while the other four engines silently truncate at 255 bytes; calog.h:32/36 already centralizes cross-adapter numeric policy (CALOG_MAX_DEPTH, CALOG_GROWTH_FACTOR), and no comment documents the caps as intentionally per-engine. However, the other two groups are refuted: the initial-capacity-8 defines size different data structures and the codebase deliberately varies these per structure (CONTEXT_INITIAL_ENGINES=4, BROKER_INITIAL_SLOTS=16 with a power-of-two assert), and LUA_MARSHAL_SLOTS vs SQUIRREL_MARSHAL_SLOTS are per-VM stack-headroom requirements (lua_checkstack vs sq_reservestack) that must be independently tunable. Only CALOG_ERR_MSG_CAP is a worthwhile consolidation; a global CALOG_INITIAL_CAP/CALOG_MARSHAL_SLOTS would falsely couple independent tunables.

L41. [dead-code] src/berry/berryAdapter.c:93 --- FIXED

Status: FIXED

The !be_getglobal calogErrDeadE branch in berryCallableInvoke is unreachable (the refName global always exists once created), it hides a one-slot stack leak, and the real released-callable case surfaces as a confusing pcall error instead of calogErrDeadE.

if (!be_getglobal(vm, export->refName)) { return calogFail(result, calogErrDeadE, "berry callable no longer exists"); } (lines 93-95). be_getglobal returns bfalse only when the name was never registered (vendor/berry/src/be_api.c:598), but berryExportValue registered refName via be_setglobal, and Berry never removes global names -- berryCallableRelease merely sets the slot to nil, after which be_getglobal returns btrue with nil pushed. So (a) the branch can never execute; (b) if it ever could, it returns without popping the value be_getglobal unconditionally pushes (be_incrtop even on the miss path), leaking a VM stack slot per call; (c) the case the branch was written for -- invoking after the pinned function was dropped or a script overwrote _calog_fn_N -- falls through to be_pcall on nil and reports a generic "'nil' value is not callable" calogErrArgE instead of the intended calogErrDeadE. Fix: after be_getglobal, test be_isnil(vm, -1), pop back to base, and return calogErrDeadE.

Verifier: Verified in vendor/berry/src/be_api.c:598 that be_getglobal returns bfalse only for never-registered names and pushes a nil slot (be_incrtop) even on the miss path; berryExportValue registers refName via be_setglobal before calogFnCreate, and berryCallableRelease only nils the slot (names are never removed from Berry's global vtab per be_var.c), so the calogErrDeadE branch at berryAdapter.c:93-95 cannot fire in normal operation, and its early return skips the pop-to-base every other exit path performs (latent one-slot leak). The one reachable dead scenario -- a script overwriting _calog_fn_N with nil -- passes the check (btrue, nil pushed) and fails in be_pcall as a generic calogErrArgE instead of the intended calogErrDeadE. The suggested be_isnil fix is correct. No comment documents this as intentional.

L42. [multiple-truth] src/calogMain.c:234 --- FIXED

Status: FIXED

printUsage hardcodes the extension list ".lua .js .nut .bas .be .scm .wren" and the library-name list, duplicating truths owned by gEngines[]->extensions and the register calls.

Line 234 fprintf(stream, " .lua .js .nut .bas .be .scm .wren\n"); restates what gEngines[] (line 90) plus each engine's extensions array already encode -- the very data engineForExtension/resolveArg consult at runtime -- and lines 238-239 restate the 13-library list from the register chain at lines 429-443. The same extension list appears a third time in the file-header comment (lines 4-5). When an engine gains an extension or a library is added, the help text silently lies. printUsage can loop for each gEngines[i], for each extensions[which] to print the list from the single runtime truth, exactly as the resolver does.

Verifier: Line 234 of src/calogMain.c hardcodes the extension list that gEngines[] (line 90) plus each engine's NULL-terminated extensions array already own, and which engineForExtension (lines 116-131) and resolveArg (lines 334-347) iterate at runtime in the same file; the header comment (lines 4-5) repeats it a third time. Lines 238-239 likewise restate the 13-library register chain at lines 431-443 one-for-one. No comment marks this as an intentional tradeoff. The extension-list fix is concrete and trivial (loop gEngines like the resolver does, preserving the priority order the prose promises); the library-list half is weaker since no library table exists to derive from. Drift risk is live: calogSsh was just added in the working tree and required a manual prose edit.

L43. [dumb-code] src/calogMain.c:348 --- FIXED

Status: FIXED

The extensionless search opens every candidate file twice (fileReadable's fopen/fclose probe, then readFile's fopen), and the gap between them turns a vanished file into a hard whole-run failure instead of continuing the search.

Lines 348-357: if (!fileReadable(path)) { free(path); continue; } source = readFile(path); if (source == NULL) { ...; return false; }. fileReadable (line 154) exists solely for this call site and duplicates the fopen readFile is about to do. If the file disappears between the probe and the read (TOCTOU), the code aborts resolution of ALL scripts (return false -> fail-fast exit 1) where the intended semantics of the probe was 'not here, keep searching'. Dropping fileReadable and calling readFile directly -- continue when it returns NULL with errno==ENOENT, hard-fail otherwise -- halves the opens, deletes a single-use helper, and closes the window.

Verifier: Confirmed in src/calogMain.c: fileReadable (line 154) has exactly one caller (line 348) and duplicates the fopen readFile (line 275) performs, so every existing candidate is opened twice during extensionless search. If the file disappears between probe and read, readFile returns NULL and resolveArg returns false (lines 352-357), which main's fail-fast loop (lines 405-414) turns into exit 1 for the whole run instead of continuing the extension search. readFile's documented contract ('Returns NULL on failure, leaving errno set', line 274) makes the proposed continue-on-ENOENT fix directly implementable. No comment documents the probe or the double-open as an intentional tradeoff; the fail-fast comment covers bad names, not this race. Minor caveat: an ENOENT-only continue would hard-fail on EACCES candidates that fileReadable currently skips silently.

L44. [duplication] src/context.c:360 --- FIXED

Status: FIXED

calogContextOpen repeats the same four-line failure cleanup (unlock, mutex_destroy, cond_destroy, free) three times.

Lines 349-353 (index-cap check), 361-365 (realloc failure), and 380-387 (pthread_create failure) each hand-roll pthread_mutex_unlock(&broker->ctxMutex); pthread_mutex_destroy(&context->queueMutex); pthread_cond_destroy(&context->queueCond); free(context); return NULL; (the third with the registry rollback added). A single fail: cleanup label (or small static helper destroying the context shell) would leave one copy of the teardown, so a future field added to CalogContextT cannot be missed in one of the three paths.

Verifier: src/context.c calogContextOpen contains three literal copies of the teardown sequence (pthread_mutex_destroy(&context->queueMutex); pthread_cond_destroy(&context->queueCond); free(context); return NULL;) at lines 349-353, 361-365, and 384-387, differing only in lock/rollback handling before the core; no comment documents this as intentional, and the codebase already uses the goto-cleanup idiom (mybasicAdapter.c:569 cleanupArgs), so a single fail: label is a concrete, style-consistent consolidation. The third path has already diverged once (registry rollback), demonstrating the future-field divergence hazard the claim cites. No correctness issue, so severity stays low.

L45. [dead-code] src/context.c:1035 --- FIXED

Status: FIXED

registryFreePush returns an int32_t status that both call sites ignore, making the return value dead.

registryFreePush (line 1035) returns calogErrOomE/calogOkE, but both callers discard it: calogContextOpen line 382 registryFreePush(broker, index); and calogContextClose line 497 registryFreePush(broker, index);. The OOM policy is documented in the function comment (the index is simply not recycled), so the status serves no caller; declare it void to make the intentional ignore explicit rather than looking like two unchecked error returns.

Verifier: Verified in src/context.c: registryFreePush (static, line 1035) returns int32_t calogErrOomE/calogOkE, but its only two call sites (calogContextOpen line 382 and calogContextClose line 497) discard the value as bare statements; the function's own comment documents the OOM policy (slot simply not recycled, never a correctness fault), so no caller can meaningfully act on the status. The return value is genuinely dead and declaring the function void is a concrete, correct cleanup -- though purely cosmetic with no behavioral impact.

L46. [duplication] src/context.c:1090 --- FIXED

Status: FIXED

The message-kind dispatch is written three times (serveLoop, pumpUntil, hostDispatch) with identical call/eval/release handling; the divergence this invites already produced the host-drain leak.

serveLoop lines 1090-1102, pumpUntil lines 1011-1026, and hostDispatch lines 864-877 each independently dispatch messageCallE -> contextDispatchCall, messageEvalE -> contextDispatchEval, messageReleaseE -> contextDispatchRelease, differing only in their loop-specific kinds (REPLY, SHUTDOWN, ERROR). A shared helper, e.g. static bool dispatchCommon(CalogContextT *context, MessageT *message) returning whether the kind was consumed, would collapse the three copies and keep future kinds from being handled inconsistently -- exactly the failure mode of the calogActorShutdown host drain (finding 1), where one of the parallel copies forgot the RELEASE case.

Verifier: Confirmed in src/context.c: serveLoop (1090-1102), pumpUntil (1011-1026), and hostDispatch (864-877) each independently repeat the CALL->contextDispatchCall / RELEASE->contextDispatchRelease (and EVAL in two of three; hostDispatch omits EVAL, a minor inaccuracy in the claim) dispatch, differing only in loop-specific kinds. The predicted failure mode of such parallel copies is already live in the code: contextDrainQueue (615-622) finalizes messageReleaseE callables but the parallel host drain in calogActorShutdown (275-277) uses bare messageFree, which has no RELEASE-callable handling (924-943), leaking the callable -- exactly the referenced finding-1 leak. A dispatchCommon helper returning consumed/not-consumed fits all three loops cleanly since each keeps its own specific kinds and default. Real, concrete cleanup, but not itself a bug.

L47. [multiple-truth] src/js/jsAdapter.c:33 --- FIXED

Status: FIXED

The 256-byte error-message cap is independently defined in four adapters (JS_ERR_MSG_CAP, BERRY_ERR_CAP, S7_ERR_CAP, WREN_ERR_CAP) and can drift; the JS copy is also unnecessary since jsForeignCall proves the throw-then-free pattern needs no buffer.

#define JS_ERR_MSG_CAP 256 (src/js/jsAdapter.c:33), #define BERRY_ERR_CAP 256 (src/berry/berryAdapter.c:28), #define S7_ERR_CAP 256 (src/s7/s7Adapter.c:29), #define WREN_ERR_CAP 256 (src/wren/wrenAdapter.c:26) -- one fact in four files; hoist a single CALOG_ERR_MSG_CAP into calogInternal.h next to CALOG_MAX_DEPTH. In Berry the buffer is genuinely required (be_raise longjmps, so result must be freed first), but in the JS adapter jsTrampoline (lines 698-700) copies into the buffer only so it can free result before throwing, while jsForeignCall (lines 327-329) already uses the simpler pattern -- JS_ThrowTypeError copies its formatted message immediately, so throw first, then calogValueFree, and the buffer, the snprintf, and JS_ERR_MSG_CAP disappear from the JS adapter entirely.

Verifier: All four defines exist as claimed (jsAdapter.c:33, berryAdapter.c:28, s7Adapter.c:29, wrenAdapter.c:26), all 256, no comments marking them as intentional per-engine choices. The JS-specific point is confirmed by the file itself: jsTrampoline (lines 662, 698-700) snprintfs into a stack buffer only so it can free the result before JS_ThrowTypeError, while jsForeignCall (lines 327-329) in the same file already throws first and frees after -- safe because JS_ThrowTypeError copies its formatted message immediately -- proving the buffer, snprintf, and JS_ERR_MSG_CAP are removable with no behavior change (QuickJS's internal error-format buffer is also 256 bytes). Minor inaccuracy in the claim: CALOG_MAX_DEPTH is defined in src/calog.h:32, not calogInternal.h, so the hoisted constant belongs there or in calogInternal.h; this does not affect the finding. No runtime bug -- drift would only cause inconsistent truncation lengths -- so this is a low-severity multiple-sources-of-truth cleanup, which the project conventions explicitly request.

L48. [error-path] src/js/jsAdapter.c:488 --- FIXED

Status: FIXED

jsResolveOwnProperty stores the unchecked result of jsFromValue into desc->value and returns 1, so on OOM the JS_EXCEPTION sentinel is handed to the engine as a real property value, violating the exotic get_own_property contract.

desc->value = jsFromValue(ctx, &result, 0); (line 488) followed unconditionally by return 1; (line 493). jsFromValue's calogFnE path can fail (JS_NewObjectClass OOM at line 419 returns JS_EXCEPTION with a pending exception). QuickJS's exotic get_own_property must return -1 when an exception is pending; returning 1 with desc->value == JS_EXCEPTION makes the engine treat the special exception tag as the property's value while an unrelated pending exception is left set. Fix: if (JS_IsException(desc->value)) { calogValueFree(&result); return -1; }.

Verifier: Verified in src/js/jsAdapter.c: line 478 guarantees result.type == calogFnE, so jsFromValue at line 488 takes the calogFnE branch where JS_NewObjectClass (line 419) can return JS_EXCEPTION on OOM with a pending exception; jsResolveOwnProperty stores that sentinel in desc->value with flags JS_PROP_C_W_E and unconditionally returns 1 (line 493), violating the get_own_property contract in vendor/quickjs/quickjs.h ("Return -1 if exception"). Every other jsFromValue call site in the file (lines 150, 381, 393) checks JS_IsException -- this is the lone unchecked one, and no comment documents it as intentional; no abort-on-OOM allocator is installed (plain JS_NewRuntime at line 74). Only reachable under QuickJS allocation failure, so error-path-only.

L49. [leak] src/lua/luaAdapter.c:203 --- FIXED

Status: FIXED

luaExportAt mallocs the LuaExportT before luaL_ref, so an OOM longjmp while growing the registry table strands the allocation -- violating the longjmp-ordering discipline this file documents elsewhere.

Lines 198-203: export = malloc(...); ... lua_pushvalue(L, idx); ref = luaL_ref(L, LUA_REGISTRYINDEX);. luaL_ref can raise a Lua error (longjmp) on OOM when the registry table grows. When luaExportAt runs inside a protected call -- e.g. LUA_TFUNCTION argument marshalling in luaToValueDepth:475 reached from luaTrampoline under the lua_pcall in calogLuaRun -- the longjmp unwinds past luaExportAt and the malloc'd export leaks (along with the caller's partially-marshalled args array, which is only freed on status-return paths). The file's own comment at 409-411 ("Retain only after the __gc-bearing userdata is installed, so an allocation failure (longjmp) ... cannot strand a reference") establishes exactly this discipline; this spot violates it. Fix: take the ref first, malloc second, and luaL_unref on malloc failure (luaL_unref does not allocate).

Verifier: Verified in src/lua/luaAdapter.c: luaExportAt mallocs LuaExportT (line 198) before luaL_ref (line 203), and luaL_ref can raise LUA_ERRMEM on registry growth with the stock luaL_newstate allocator (line 116). The path is reachable inside a protected call: luaTrampoline (572) -> luaToValueDepth (590) -> LUA_TFUNCTION case (473-475) -> luaExportAt, under the lua_pcall at calogLuaRun:432 or luaCallableInvoke:84; a longjmp there strands export plus the trampoline's partially-marshalled args array, which is freed only on status-return paths. The file itself documents the ref-after-alloc longjmp discipline at lines 409-411 and converts other alloc-adjacent Lua failures to status codes (lua_checkstack -> calogErrOomE), so this is a genuine violation, not a documented tradeoff. Fix as proposed is sound since luaL_unref cannot raise.

L50. [redundant-conditional] src/lua/luaAdapter.c:544 --- FIXED

Status: FIXED

The first branch of the table-kind reclassification assigns calogListE to an aggregate that was created as calogListE and never modified.

Line 484 creates the aggregate with calogAggCreate(&aggregate, calogListE) and nothing between there and line 544 touches kind, so if (aggregate->pairCount == 0) { aggregate->kind = calogListE; } re-assigns the value it already holds. The classification collapses to: if (aggregate->pairCount > 0) { aggregate->kind = (aggregate->arrayCount == 0) ? calogMapE : calogBothE; }. (The Squirrel adapter needs no equivalent because array/table are distinct VM types.)

Verifier: Verified in src/lua/luaAdapter.c and src/value.c: the aggregate is created with kind=calogListE at line 484, and the only functions called on it before line 544 (calogAggPush at value.c:135, calogAggSet at value.c:153) never write aggregate->kind -- the sole kind assignment in value.c is in calogAggCreate (line 98). So the pairCount==0 branch at lines 544-545 re-assigns calogListE to a field that already holds it; the conditional collapses to the claimed two-way form. No comment documents the block as intentional and no other adapter shares the idiom. It is a genuine redundant conditional (a class the project's CLAUDE.md explicitly asks to be flagged), but purely cosmetic with no behavioral effect.

L51. [multiple-truth] src/mybasic/mybasicAdapter.c:18 --- FIXED

Status: FIXED

The trampoline count 256 is maintained in three places that can silently drift: #define MB_BANK_SIZE 256, the 256 hand-listed MB_TRAMPOLINE(n) expansions, and the 256-entry mbTrampTable initializer.

#define MB_BANK_SIZE 256 (line 18), the MB_TRAMPOLINE(0)..MB_TRAMPOLINE(255) block (lines 61-316), and the static const mb_func_t mbTrampTable[MB_BANK_SIZE] = { mbTramp0, ... mbTramp255 } initializer (lines 319-352) all encode the same count independently. Shrinking MB_BANK_SIZE fails at compile time (excess initializers), but RAISING it compiles cleanly: slots >= 256 are zero-initialized NULLs, so calogMyBasicExpose passes NULL to mb_register_func (which returns 0 for !f) and every native past the 256th silently fails to expose with calogErrArgE at context creation -- natives vanish from BASIC with no build-time signal. A _Static_assert tying the table to 256 entries (e.g. a sentinel count), or generating both the trampolines and the table from one X-macro list (nested MB_T16/MB_T256 repetition macros), would collapse the three sources to one.

Verifier: Verified all three independent encodings of 256 in src/mybasic/mybasicAdapter.c: MB_BANK_SIZE (line 18), the hand-listed MB_TRAMPOLINE(0..255) block (lines 61-316), and the 256-entry mbTrampTable[MB_BANK_SIZE] initializer (lines 319-352). The failure mode is real and concrete: raising MB_BANK_SIZE compiles cleanly with NULL-filled tail slots, mb_register_func (vendor/ourbasic/ourBasic.c:12612) returns 0 for a NULL function, and the production caller mybasicExposeVisitor (src/mybasic/mybasicEngine.c:78) discards calogMyBasicExpose's error, so natives past slot 255 would silently vanish from BASIC. No _Static_assert or sync-documenting comment exists; the file header only explains why the trampoline bank exists. Cheap fix exists (unsized table + _Static_assert on element count, or one repetition macro generating both blocks). However, the current code is correct as-is -- the trap only fires on a future edit -- so this is a latent maintenance hazard, not a live bug.

L52. [multiple-truth] src/mybasic/mybasicAdapter.c:42 --- FIXED

Status: FIXED

The process-wide refcount static int32_t mbContextCount lives in the adapter but the mutex that makes it correct lives in a different translation unit (mybasicEngine.c's mbLifecycleLock), and neither the adapter nor its header states the serialization requirement.

static int32_t mbContextCount = 0; (mybasicAdapter.c:42) guards the mb_init()/mb_dispose() lifecycle singletons in calogMyBasicCreate (lines 833-835, 838-843) and calogMyBasicDestroy (lines 864-867), but the only synchronization is pthread_mutex mbLifecycleLock declared in src/mybasic/mybasicEngine.c:29 and taken only by the engine vtable wrappers. calogMyBasicCreate/Destroy are public API in mybasicAdapter.h with zero mention that callers must serialize them; tests (testMyBasic.c, testPolyglot.c) already call them directly. Any future direct caller creating two contexts on different threads races mbContextCount and double-runs mb_init. Every other process-wide refcounted registry in this codebase keeps the static mutex next to the state it guards (per the library pattern); moving the lock (or at least the locking obligation, documented) into the adapter would restore a single source of truth for the invariant.

Verifier: All cited facts confirmed in code: mbContextCount (mybasicAdapter.c:42) gates process-global mb_init/mb_dispose at lines 833-843/864-867 with no lock in the adapter; the only serialization is mbLifecycleLock in mybasicEngine.c:29, held only by the engine vtable wrappers in a different TU; mybasicAdapter.h documents its other invariant (serving requirement) but is silent on the create/destroy serialization obligation, and tests already call the public API directly. Every other process-global refcount in the codebase (gInitMutex in calogKv/calogPubsub/calogExport/calogTimer, plus the lib mutexes in calogTask/calogNet/calogSsh/calogDb) keeps the mutex in the same TU as the state -- this is the lone deviation. However, no race is reachable in normal use today: production creation goes through the locked vtable and the direct-calling tests are single-threaded, so it is a latent hazard/consistency defect, not a live bug.

L53. [error-path] src/mybasic/mybasicAdapter.c:577 --- FIXED

Status: FIXED

mbDispatch's realloc-failure path disposes a popped LIST/DICT but not a popped MB_DT_ROUTINE, leaking a lambda's scope -- inconsistent with the refused-argument path 20 lines below.

On realloc failure the code runs if (popped.type == MB_DT_LIST || popped.type == MB_DT_DICT) { mb_dispose_value(context->bas, popped); } (lines 577-579) and bails, but the marshal-refused path at lines 597-599 additionally does if (popped.type == MB_DT_ROUTINE) { mb_dispose_value(context->bas, popped); } with the comment "A refused routine argument (e.g. a lambda created just for this call) was popped and is ours to free". The vendor/ourbasic CHANGELOG documents exactly this rule ("a routine (lambda) popped as a refused/errored native argument is now disposed on the error path so its scope is not leaked"). Concrete failure: a script passes a lambda as the argument that triggers capacity growth (e.g. the 9th arg, MB_INITIAL_ARGS boundary) and realloc fails -- the lambda's scope is leaked. The two error paths should share one dispose helper covering LIST/DICT/ROUTINE.

Verifier: Confirmed in src/mybasic/mybasicAdapter.c: the realloc-failure branch (lines 576-582) disposes only LIST/DICT, while the marshal-refused branch (lines 590-601) also disposes MB_DT_ROUTINE with an explicit comment that a popped routine is owned by the native and must be disposed "so its scope is not leaked". The ownership rule is verified in vendor/ourbasic/ourBasic.c: mb_pop_value takes a ref via _REF (line 13048) that only mb_dispose_value's _UNREF releases, and the fork CHANGELOG (lines 16-18) documents that a routine popped as a "refused/errored" native argument must be disposed on error paths. On realloc failure a popped lambda never reaches mbToValueDepth/mbWrapRoutine, so its ref is never released and its scope leaks. Reachable even for the first argument (realloc(NULL,...)), but only when allocation fails.

L54. [dead-code] src/mybasic/mybasicAdapter.c:622 --- FIXED

Status: FIXED

args = NULL; after the post-call free in mbDispatch is a dead store: no path after it reads args or can reach the cleanupArgs label.

Lines 618-622: for (...) { calogValueFree(&args[index]); } free(args); args = NULL;. After this point the function only returns (lines 626, 632, 637, 639); the cleanupArgs label at line 641 is reachable solely from the gotos at lines 569, 581, 600, and 607, all of which precede the calogCall at line 615. The NULL assignment is therefore never observed. Note the companion invariant is also unguarded: if the free-then-NULL is meant as protection against a future goto, it is incomplete anyway because argCount is not reset to 0, so a hypothetical later jump to cleanupArgs would iterate calogValueFree over a NULL args. Either delete the dead store or (if defensiveness is wanted) also zero argCount; as written it is half of a safety measure that no code path uses.

Verifier: Verified in src/mybasic/mybasicAdapter.c: args = NULL; at line 622 follows the post-calogCall free at 621; all four goto cleanupArgs sites (569, 581, 601, 607) precede calogCall at 615, and every path after line 622 returns (raise-error returns, mb_push_string, mb_push_value) without reading args or reaching the cleanupArgs label at 641. The NULL store is never observed -- a true dead store. The companion point is also correct: argCount is not reset, so as a defensive measure it is incomplete anyway. No comment documents it as intentional. Purely cosmetic; no runtime failure is reachable.

L55. [dumb-code] src/mybasic/mybasicAdapter.c:928 --- FIXED

Status: FIXED

calogMyBasicRun reports only the raw numeric error code ("my-basic load error: %d") although the interpreter provides mb_get_last_error/mb_get_error_desc, making it the only engine adapter that prints no human-readable message.

fprintf(stderr, "my-basic load error: %d\n", code); (line 928) and fprintf(stderr, "my-basic run error: %d\n", code); (line 933). Every sibling adapter prints the engine's message text (luaAdapter.c:428 lua_tostring(L, -1), berryAdapter.c:393, jsAdapter.c:443, wrenAdapter.c:500, s7Adapter.c:450), and vendor/ourbasic/ourBasic.h:685-686 exposes mb_get_last_error(s, &file, &pos, &row, &col) plus mb_get_error_desc(err) which would give the description and the row/col of the failure. A script author debugging a .bas syntax error currently gets an opaque enum integer where every other language gives a message and location.

Verifier: Confirmed: mybasicAdapter.c:928/933 print only the raw int code ('my-basic load error: %d'); no mb_set_error_handler is registered anywhere in the codebase, so no human-readable message path exists; vendor/ourbasic/ourBasic.h:685-686 expose mb_get_last_error/mb_get_error_desc which supply description plus row/col; sibling adapters (luaAdapter.c:428, berryAdapter.c:393) print the engine's message text, making my-basic the sole adapter emitting an opaque enum integer. Concrete fix is a two-branch change using the available API. No comment documents this as intentional.

L56. [multiple-truth] src/s7/s7Adapter.c:434 --- FIXED

Status: FIXED

The buffer slack constants 128 (calogS7Run) and 64 (calogS7Expose) hand-encode the lengths of format strings defined elsewhere, and the calogS7Run margin is only ~11 bytes.

Line 434: size = strlen(escaped) + 128; must cover the wrapper at lines 441-443 -- "(catch #t (lambda () (eval-string \"%s\")) (lambda (tag . info) (cons '%s (object->string (cons tag info)))))" plus S7_ERROR_TAG -- whose fixed part is ~117 bytes, leaving ~11 bytes of headroom. Line 271: size = strlen(name) * 2 + 64; similarly mirrors the define-wrapper format at line 276. The buffer size and the format text are two sources of truth: a modest edit to either format string (or a longer S7_ERROR_TAG) silently exceeds the slack, snprintf truncates, and the truncated text is handed to s7_eval_c_string as mangled Scheme that fails with a baffling read error instead of a build-time signal. Fix: size with snprintf(NULL, 0, fmt, ...) + 1, or define the slack as sizeof of the literal fixed part so the two cannot drift.

Verifier: Verified by direct measurement: the calogS7Run wrapper's fixed text (format literal at s7Adapter.c:442 minus the two %s, plus S7_ERROR_TAG "calog-error" and the NUL) is exactly 117 bytes against a hand-coded slack of 128 at line 434 -- precisely 11 bytes of headroom, as claimed. calogS7Expose (line 271) similarly hand-encodes its 41-byte fixed part with a 64-byte slack. Neither constant is documented or derived from the format strings, so a modest edit to the wrapper text (or a longer error tag) silently truncates via snprintf and hands mangled Scheme to s7_eval_c_string, producing a baffling runtime read error with no build-time signal. This matches the codebase's explicit review criteria (magic numbers, multiple sources of truth). It is not a live bug -- snprintf bounds correctly and no truncation is reachable with the current literals -- so it stays a low-severity latent-maintenance finding, not a defect in normal use.

L57. [bug] src/s7/s7Adapter.c:447 --- FIXED

Status: FIXED

calogS7Run detects script failure by in-band matching the result against the 'calog-error tag, so a script legitimately returning such a pair is falsely reported as failed.

Line 447: if (s7_is_pair(result) && s7_is_eq(s7_car(result), s7_make_symbol(sc, S7_ERROR_TAG))) treats any returned pair whose car is the symbol *calog-error* as a caught error. Failure scenario: a script whose final expression evaluates to (cons '*calog-error* "x") (e.g. data that round-trips the tag, or a script echoing a previously captured error value) runs successfully yet calogS7Run prints 's7 error: x' and returns calogErrArgE, which retires the context in the calog runner. Cheap fix: have the catch handler return an unforgeable sentinel instead of a plain symbol-tagged cons -- e.g. cons a gensym created at calogS7Create time (kept in CalogS7T and gc-protected) and compare with s7_is_eq against that exact object, which no script can fabricate.

Verifier: Confirmed in src/s7/s7Adapter.c: the catch handler (lines 441-443) tags errors with a plain cons whose car is the interned symbol calog-error (S7_ERROR_TAG, line 32), and line 447 detects failure via s7_is_pair + s7_is_eq against s7_make_symbol of that same name. Since symbols are interned, a script whose final expression is (cons 'calog-error "x") returns a value indistinguishable from the handler's sentinel, so calogS7Run prints 's7 error: x' and returns calogErrArgE despite the script succeeding. No guard or documented-tradeoff comment exists; the only comment (line 440) describes the catch wrapper, not the in-band collision. The gensym-sentinel fix is valid.

L58. [redundant-conditional] src/squirrel/squirrelAdapter.c:282 --- FIXED

Status: FIXED

Dead defensive branches: the if (argCount < 0) argCount = 0; guards in squirrelForeignCall and squirrelTrampoline can never fire, and the NULL check in squirrelForeignRelease is impossible.

Lines 282-284 and 634-636: a native-closure invocation always has 'this' at stack slot 1 plus the closure's free variable pushed at the top (per the adapter's own comments at 266-267 and 628-630), so top >= 2 and argCount = (int32_t)(top - 2) is always >= 0 -- both guards are unreachable. Similarly lines 329-332: callable = *(CalogFnT **)p; if (callable != NULL) { calogFnRelease(callable); } -- squirrelPushValueDepth:439 unconditionally assigns *slot = value->as.fn (never NULL; calogValueFn values carry a live pointer) before the release hook is installed at line 440, so the NULL check never triggers. The Lua counterparts (luaFunctionCall:268, luaTrampoline:581, luaFunctionGc:313-314) correctly omit both checks.

Verifier: Verified in vendored VM source: SQVM::CallNative (vendor/squirrel-src/squirrel/sqvm.cpp:1181) sets frame top to newbase + nargs + _noutervalues; nargs always includes 'this' (>= 1) and both closures are built with sq_newclosure(v, fn, 1) so _noutervalues == 1, guaranteeing top >= 2 and argCount = top - 2 >= 0 -- the clamps at squirrelAdapter.c:282-284 and 635-637 cannot fire. The NULL check at 329-332 is likewise unreachable: *slot = value->as.fn is assigned at line 439 before the release hook is installed at 440, nothing else writes the slot, and every calogFnE producer (all 7 adapters plus libs/calogExport.c:200) only stores a live pointer, returning early on error. The Lua counterparts (luaAdapter.c:268, 582, 313-314) correctly omit both checks, confirming the inconsistency. Real dead code, but purely a minor cleanup with no behavioral or safety impact.

L59. [multiple-truth] src/value.c:42 --- FIXED

Status: FIXED

The CALOG_MAX_DEPTH comparison convention has drifted: value.c and all seven engine adapters use depth > CALOG_MAX_DEPTH (permitting 65 nesting levels) while libs/calogJson.c and libs/calogKv.c use depth >= CALOG_MAX_DEPTH (permitting 64).

value.c:42 if (depth > CALOG_MAX_DEPTH) { return calogErrDepthE; } with depth starting at 0 for the outermost aggregate allows depth values 0..64, i.e. 65 aggregate levels -- one more than the documented limit in calog.h:29-32 ("Maximum nesting depth honored by the recursive copy/marshal paths" with CALOG_MAX_DEPTH 64). All engine adapters (lua/js/squirrel/mybasic/berry/s7/wren) match value.c's >, but libs/calogJson.c:117 and :678 and libs/calogKv.c:93 use depth >= CALOG_MAX_DEPTH, allowing only 64 levels. Concrete failure: a 65-level-deep aggregate copies cleanly through calogValueCopy and marshals into and out of every engine, but the same value then fails calogErrDepthE when handed to JSON encode or the KV store -- the single named limit means two different depths depending on which code path interprets it. Pick one convention (>= matches the documentation) and apply it everywhere.

Verifier: Verified in code: value.c:42 and all seven engine adapters check depth > CALOG_MAX_DEPTH with depth starting at 0 (calogValueCopy passes 0 at value.c:327; e.g. luaAdapter.c:78/92/277/301 all pass 0), permitting aggregates at depths 0..64 (65 levels), while calogJson.c:117/678 and calogKv.c:93 check depth >= CALOG_MAX_DEPTH also starting at 0 (jsonStringifyNative:754, jsonParseNative:420, kvSet:239), permitting only 64. A 65-level aggregate (empty innermost) copies via calogValueCopy and marshals through every engine but fails jsonStringify with calogErrDepthE and is rejected by kvSet (through the misleading contains-function gate). No comment documents an intentional difference: calog.h:29-32 describes one limit, calogJson.h:9 says "bounded by CALOG_MAX_DEPTH both ways", and tests use a +5 overshoot so neither boundary is pinned. Minor nuance: value.c additionally never depth-checks scalars (a third behavior), so the claim slightly overstates engine tolerance for payload-carrying values, but the core drift and failure scenario are confirmed.

L60. [magic-number] src/value.c:201 --- FIXED

Status: FIXED

calogFnFromNative passes the bare literal 0 as the host owner id where every other site in the codebase uses the CALOG_HOST_ID constant.

value.c:201 return calogFnCreate(out, calog, fn, userData, NULL, 0); -- the trailing 0 is the host context id, spelled out only in the adjacent comment "(owner id 0 = host)". Nineteen lines later the same fact is compared symbolically: if (callable->ownerCtxId == CALOG_HOST_ID || ...) (value.c:220), and context.c uses CALOG_HOST_ID at every one of its six sites (context.c:147, 213, 469, 963, 1056, and the comment at :44 explicitly noting the macro is "shared with value.c's finalize path"). value.c already includes calogInternal.h where CALOG_HOST_ID is defined (calogInternal.h:15), so the fix is a one-token change; as written, the two spellings of the same truth can drift if the host id ever changes.

Verifier: value.c:201 does pass bare literal 0 as the ownerCtxId argument to calogFnCreate (signature at value.c:179 confirms the parameter), while value.c:220 in the same file compares ownerCtxId against CALOG_HOST_ID, and all six context.c sites (147, 213, 469, 963, 1056, comment at 44) use the macro. calogInternal.h:15 defines CALOG_HOST_ID and is already included by value.c (proven by line 220 compiling). The literal and the macro are two spellings of the same truth that the finalize-path comparison depends on; a one-token change fixes it. Real but cosmetic-consistency only, no reachable runtime defect.

L61. [multiple-truth] vendor/ourbasic/ourBasic.c:19526 --- FIXED

Status: FIXED

Both calog patch comments reference pre-rename filenames that no longer exist: "See myBasic.h" (19526) and "see myBasic.c.orig" (1331).

vendor/ourbasic/ contains only ourBasic.c, ourBasic.h, ourBasic.c.upstream, CHANGELOG, NOTICE. The comment above mb_eval_routine_cold says See myBasic.h. (line 19526) and the _Atomic patch comment says calog patch (see myBasic.c.orig) (line 1331); after the mybasic -> ourbasic rebrand those files are ourBasic.h and ourBasic.c.upstream respectively. Anyone following the comments to compare against upstream hits dead references. Update both comments to the current filenames.

Verifier: Verified in vendor/ourbasic/ourBasic.c: line 19526 says "See myBasic.h." and line 1331 says "calog patch (see myBasic.c.orig)", but the directory contains only ourBasic.c, ourBasic.h, ourBasic.c.upstream, CHANGELOG, NOTICE (git shows the mybasic->ourbasic rename in commit b5cf00b). Both comments exist to point maintainers at the fork-patch counterparts (ourBasic.h has the mb_eval_routine_cold prototype at line 667; ourBasic.c.upstream is the pristine copy) and both are now dead references. Real but purely comment rot; fix is two one-word edits with zero runtime impact.

L62. [optimization] vendor/ourbasic/ourBasic.c:19532 --- FIXED

Status: FIXED

mb_eval_routine_cold walks the entire program AST (while(n && n->next) n = n->next;) on every callback dispatch even though the list keeps an O(1) tail pointer.

s->ast is an _ls_create() sentinel list whose prev always points at the tail: _ls_pushback (2466-2471) itself relies on _ls_back(list) (= list->prev) to append, and every AST node is added via _ls_pushback(s->ast, ...) (5623, 6100, 6108, 6148, 6153), so the invariant is guaranteed. The O(n) walk therefore re-derives a value already stored, on EVERY cold callback (timer/pubsub/export) -- for a large script with a fast timer that is thousands of pointless node hops per tick. Replace lines 19531-19533 with land = _ls_back(s->ast);. Bonus: for a (theoretical) empty AST, _ls_back returns NULL, which mb_eval_routine handles cleanly, whereas the current walk returns the sentinel node itself, whose data field is the list COUNT (see _ls_pushback line 2472), not an _object_t.

Verifier: Verified in vendor/ourbasic/ourBasic.c: mb_eval_routine_cold (19531-19533) walks the whole AST to find the tail, yet the list's tail pointer invariant is airtight -- s->ast is _ls_create()'d (12308), every append goes through _ls_pushback(s->ast,...) (5623, 6100, 6108, 6148, 6153) which always sets list->prev to the new tail (2471), and the only removals are wholesale _ls_clear/_ls_destroy (12440, 12346) where _ls_clear resets prev=0 (2752). Upstream code already uses the proposed idiom _ls_back(s->ast) for the same purpose at line 11289, proving the invariant is relied upon. The sole caller is the callback dispatch in src/mybasic/mybasicAdapter.c:453 (timer/pubsub/export), so the O(n) walk runs on every callback fire. The empty-AST bonus point is also accurate (sentinel's data is the count per line 2472, not an _object_t). Concrete one-line equivalent fix; no behavior change in reachable states.

216 KiB Raw Blame History

calog full audit -- 114 confirmed findings

Remediation status

HIGH (7)

H1. [bug] libs/calogHttp.c:449 --- FIXED

H2. [bug] libs/calogSsh.c:738 --- FIXED

H3. [race] libs/calogTask.c:198 --- FIXED

H4. [race] src/context.c:151 --- FIXED

H5. [bug] src/s7/s7Adapter.c:378 --- FIXED

H6. [bug] src/wren/wrenAdapter.c:230 --- FIXED

H7. [bug] src/wren/wrenAdapter.c:369 --- FIXED

MEDIUM (45)

M1. [duplication] Makefile:492 --- FIXED

M2. [error-path] Makefile:504 --- FIXED

M3. [bug] libs/calogCrypto.c:86 --- FIXED

M4. [duplication] libs/calogDb.c:796 --- FIXED

M5. [error-path] libs/calogDb.c:820 --- FIXED

M6. [bug] libs/calogDb.c:1014 --- FIXED

M7. [bug] libs/calogExport.c:109 --- FIXED

M8. [bug] libs/calogFs.c:243 --- FIXED

M9. [bug] libs/calogHttp.c:394 --- FIXED

M10. [bug] libs/calogHttp.c:706 --- FIXED

M11. [error-path] libs/calogHttp.c:883 --- FIXED

M12. [duplication] libs/calogHttp.c:921 --- FIXED

M13. [bug] libs/calogJson.c:166 --- FIXED

M14. [duplication] libs/calogNet.c:163 --- FIXED

M15. [error-path] libs/calogNet.c:781 --- FIXED

M16. [duplication] libs/calogSsh.c:164 --- FIXED

M17. [multiple-truth] libs/calogSsh.c:778 --- FIXED

M18. [bug] libs/calogTime.c:76 --- FIXED

M19. [race] libs/calogTimer.c:255 --- FIXED

M20. [leak] src/berry/berryAdapter.c:183 --- FIXED

M21. [bug] src/berry/berryAdapter.c:317 --- FIXED

M22. [duplication] src/calogMain.c:397 --- FIXED

M23. [error-path] src/calogMain.c:476 --- FIXED

M24. [error-path] src/context.c:227 --- FIXED

M25. [leak] src/context.c:275 --- FIXED

M26. [leak] src/context.c:615 --- FIXED

M27. [multiple-truth] src/js/jsAdapter.c:374 --- FIXED

M28. [bug] src/js/jsAdapter.c:623 --- FIXED

M29. [duplication] src/lua/luaAdapter.c:216 --- FIXED

M30. [duplication] src/lua/luaAdapter.c:259 --- FIXED

M31. [duplication] src/lua/luaAdapter.c:572 --- PARTIAL

M32. [bug] src/mybasic/mybasicAdapter.c:404 --- FIXED

M33. [bug] src/s7/s7Adapter.c:111 --- FIXED

M34. [error-path] src/s7/s7Adapter.c:117 --- FIXED

M35. [bug] src/s7/s7Adapter.c:156 --- FIXED

M36. [duplication] src/s7/s7Adapter.c:285 --- FIXED

M37. [duplication] src/squirrel/squirrelAdapter.c:30 --- FIXED

M38. [error-path] src/squirrel/squirrelAdapter.c:456 --- FIXED

M39. [optimization] src/value.c:77 --- FIXED

M40. [dead-code] src/value.c:249 --- KEPT

M41. [duplication] src/value.c:299 --- FIXED

M42. [duplication] src/wren/wrenAdapter.c:358 --- FIXED

M43. [multiple-truth] src/wren/wrenAdapter.c:412 --- FIXED

M44. [duplication] src/wren/wrenAdapter.c:526 --- FIXED

M45. [error-path] vendor/ourbasic/ourBasic.c:19527 --- FIXED

LOW (62)

L1. [dead-code] Makefile:33 --- FIXED

L2. [multiple-truth] Makefile:393 --- FIXED

L3. [multiple-truth] Makefile:428 --- FIXED

L4. [multiple-truth] Makefile:515 --- FIXED

L5. [bug] libs/calogCrypto.c:119 --- FIXED

L6. [duplication] libs/calogCrypto.c:250 --- FIXED

L7. [bug] libs/calogDb.c:109 --- FIXED

L8. [optimization] libs/calogDb.c:271 --- FIXED

L9. [bug] libs/calogDb.c:395 --- FIXED

L10. [dead-code] libs/calogDb.c:604 --- FIXED

L11. [redundant-conditional] libs/calogDb.c:650 --- FIXED

L12. [dead-code] libs/calogDb.c:846 --- FIXED

L13. [bug] libs/calogHttp.c:735 --- FIXED

L14. [multiple-truth] libs/calogHttp.c:759 --- FIXED

L15. [bug] libs/calogHttp.c:793 --- FIXED

L16. [bug] libs/calogHttp.c:830 --- FIXED

L17. [redundant-conditional] libs/calogJson.c:422 --- FIXED

L18. [bug] libs/calogJson.c:450 --- FIXED

L19. [duplication] libs/calogKv.c:257 --- FIXED

L20. [error-path] libs/calogNet.c:101 --- FIXED

L21. [duplication] libs/calogNet.c:458 --- FIXED

L22. [magic-number] libs/calogNet.c:733 --- FIXED

216 KiB

Raw Blame History