From 1f6eb5cfe837856f1817da490718f6d360872dc9 Mon Sep 17 00:00:00 2001 From: Scott Duensing Date: Tue, 4 Aug 2026 18:50:26 -0500 Subject: [PATCH] Several issues fixed that were found while dogfooding. --- API.md | 7 +- AUDIT.md | 321 ++++++++++++++++++++++++++++++++++++- Makefile | 15 ++ design.md | 135 +++++++++++++++- examples/scripts/README.md | 3 + libs/calogArchive.c | 17 ++ libs/calogCrypto.c | 26 +-- libs/calogDb.c | 19 ++- libs/calogExport.c | 26 ++- libs/calogFs.c | 24 +-- libs/calogHttp.c | 12 +- libs/calogJson.c | 12 +- libs/calogKv.c | 22 ++- libs/calogProc.c | 78 ++++++--- libs/calogPubsub.c | 24 ++- libs/calogRegex.c | 25 ++- libs/calogSsh.c | 34 ++-- libs/calogTask.c | 28 +++- libs/calogTime.c | 18 ++- libs/calogTimer.c | 25 ++- libs/calogXml.c | 20 ++- src/broker.c | 14 ++ src/calog.h | 6 - src/calogInternal.h | 33 +++- src/calogMain.c | 109 +++++++++++-- src/context.c | 152 +++++++++++++----- src/value.c | 73 ++++++++- tests/testArchive.c | 6 + tests/testTeardown.c | 219 +++++++++++++++++++++++++ tests/testUtil.c | 5 + 30 files changed, 1334 insertions(+), 174 deletions(-) diff --git a/API.md b/API.md index 92914099..06baa7c1 100644 --- a/API.md +++ b/API.md @@ -144,6 +144,7 @@ of them registers a native for it -- which is the point, because then it is poli | Function | Description | |---|---| | `calogPrint(...values: any)` | Write each argument to stdout, space-separated, with a trailing newline. | +| `calogArgs() -> list[string]` | The arguments given after `--` on the command line (`calog build.lua -- release v2`), the same list for every script in the run, empty when none were given. This is the sanctioned way to parameterize a run: the engines' own `getenv`/`argv` are removed, so without it every project would invent its own convention. | | `calogExit([code: int])` | Tear down the runtime and exit the process with `code` (default `0`). **Does not return** -- it unwinds the calling script at the call site, so nothing after it runs, and stops every other live script at its next native call, or at its interpreter's next hook if it calls none. The first caller's `code` is the one reported; a script that catches the unwind still cannot call another native. | ## archive @@ -155,7 +156,7 @@ decompression bomb. | Function | Description | | --- | --- | | `compress(data: string, filter: string [, level: int]) -> string` | Compress `data`. `filter`: `"gzip"`, `"bzip2"`, `"xz"`, `"zstd"`, `"lz4"`, `"compress"`, `"none"`. | -| `decompress(data: string [, filter: string]) -> string` | Decompress; the filter is auto-detected from the magic bytes if omitted. | +| `decompress(data: string [, filter: string]) -> string` | Decompress; the filter is auto-detected from the magic bytes if omitted. Naming one ENFORCES it: data in any other codec (or none) is rejected rather than decoded, so `decompress(blob, "gzip")` is a usable codec restriction for untrusted input. Names are the same as `compress` accepts. | | `archiveReadOpen(bytes: string) -> handle` | Open an archive held in memory. | | `archiveReadOpenFile(path: string) -> handle` | Open an archive streamed from a file. | | `archiveReadNext(handle) -> map \| nil` | Advance one entry: `{ name, size, mode, mtime, type[, linkname] }`; `nil` at the end. | @@ -282,7 +283,7 @@ strings (all but my-basic, whose strings are text); a my-basic port would use th its `byte*` helpers for the masked/binary frames instead of string operations. ```lua -local httpd = dofile("examples/httpd.lua") -- or paste/require the module +local httpd = load(fsRead("examples/httpd.lua"))() -- fsRead + load: dofile/require are not available local s = httpd.new() s:route("GET", "/hi", function(req) return "hello " .. req.path end) -- string body => 200 s:route("GET", "/made", function(req) return { status = 201, body = "x" } end) -- map => custom status @@ -380,7 +381,7 @@ case-insensitive, `m` multiline, `s` dot-matches-newline, `x` extended, `g` repl | `regexMatch(pattern, subject [, flags]) -> nil \| list` | First match, as `[fullMatch, group1, group2, ...]`; `nil` if no match. | | `regexSearch(pattern, subject [, flags]) -> nil \| map` | First match, as `{ match, start, end, groups: list }` (byte offsets); `nil` if no match. | | `regexReplace(pattern, subject, replacement [, flags]) -> string` | Replace matches; `replacement` uses `$1` / `${name}`. The `g` flag replaces all. | -| `regexSplit(pattern, subject [, flags]) -> list(string)` | Split `subject` on matches of `pattern`. | +| `regexSplit(pattern, subject [, flags]) -> list(string)` | Split `subject` on matches of `pattern`. An EMPTY match is not a separator -- the scan steps over it and the byte stays in the current piece -- so `regexSplit('x*', 'abc')` is `['abc']` while `regexSplit('[0-9]*', 'a1b2c')` is `['a','b','c']`. | ## ssh diff --git a/AUDIT.md b/AUDIT.md index e69abec1..8334ca60 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -2,9 +2,13 @@ Original audit: 15 scoped reviewers + 1 adversarial verifier per finding (118 raw, 4 refuted, 114 confirmed). -A **follow-on audit** is appended at the end of this file: 23 findings, IDs prefixed `S`, from the -2026-07-24/27 session. Different provenance (one reported defect and what pulling on it uncovered), -same format. It is a separate body of work -- the counts in this section cover the original 114 only. +A **follow-on audit** is appended at the end of this file: 36 findings, IDs prefixed `S`, from the +2026-07-24 to 2026-08-04 session. Same format, different provenance, and it arrived in four passes: +one reported defect and everything pulling on it uncovered (S1-S17); three open items re-examined +after being written off as accepted limitations (S18-S20); a six-lens review sweep with adversarial +verification (S21-S23); and the leads that sweep raised but never verified, plus the items its +verification confirmed (S24-S36). It is a separate body of work -- the counts in this section cover +the original 114 only. ## Remediation status @@ -1398,7 +1402,7 @@ s->ast is an _ls_create() sentinel list whose `prev` always points at the tail: --- -# Follow-on audit -- 2026-07-24/27 session (23 findings) +# Follow-on audit -- 2026-07-24 to 2026-08-04 session (36 findings) Provenance differs from the audit above, and it matters when reading these. They were not produced by a scoped reviewer sweep. They came out of **one reported defect** -- `calogExit` did not behave as @@ -1418,13 +1422,13 @@ inherits, leaving `calogEnd` a third verb for a narrow slice. See design.md sec ## Remediation status -**22 FIXED, 1 KEPT.** Verified: `make test` **959 checks / 0 failed** (with the new zig-free +**35 FIXED, 1 KEPT.** Verified: `make test` **971 checks / 0 failed** (with the new zig-free `cross-lint` running inside it); ThreadSanitizer clean on the actor core, the libraries, the JS path and my-basic; `tools/crossBuild.sh` **39 ok / 0 failed**, all ten engines built for four targets and **run** statically on musl (151 checks across 12 binaries); Windows PE and macOS x86_64/arm64 full CLIs build; every shipped example runs. -New regression tests: `tests/testExit.c` (66 checks, all ten engines), `tests/testTeardown.c` (29 +New regression tests: `tests/testExit.c` (76 checks, all ten engines), `tests/testTeardown.c` (34 checks, JavaScript on purpose -- QuickJS is the only VM that reports a stranded handle), plus cases added to `tests/testHooks.c` and `tests/testTask.c`. @@ -2012,3 +2016,308 @@ time -- it costs a handful of -O2 objects and a link, and OpenSSL was already re `bin/testNet`. --- + +## Fourth pass -- clearing the open list (13) + +Everything left open after the third pass, plus the seven leads the earlier audit had reported but +never verified (each re-verified independently and adversarially before being touched; all seven were +real, every one demonstrated by running something rather than by reading). + +--- + +### S24. [bug] libs/calogTimer.c:387, libs/calogPubsub.c:146 --- FIXED + +**Status: FIXED** + +**A timer or pubsub callback that FAILED was discarded entirely: no stderr, no error handler, no exit code -- it simply stopped working.** + +Both invoke a script callable from a thread with no caller to return a status to, and both inspected +that status only for `calogErrDeadE` (to auto-cancel a dead timer). Every other failure was dropped +on the floor. `psPublish` returns a delivery COUNT, not a status, so a broken subscriber was invisible +to everyone: the publisher saw a number, the handler's author saw nothing. Fixed by exposing the +error-posting path the actor layer already used for a failed eval (`calogPostError`) and calling it +from both, which routes the failure to the runtime's error handler on the host thread exactly like any +other script error. + +*Verifier:* A script arming a failing timer callback now reports +`calog: script error: ... timer callback blew up` and exits 1; a failing subscriber likewise. Both +produced nothing at all before. + +--- + +### S25. [bug] src/calogMain.c:279, src/context.c (contextDrainQueue) --- FIXED + +**Status: FIXED** + +**A failure could not set the exit code if the runner had not launched the context itself, and even when it could, whether it won was a race.** + +Three defects in one path. (1) `onError` only flagged contexts in `gLaunched`, so a task a script +spawned could fail, print, and leave the run reporting success. (2) The exit code was +first-writer-wins, so a task that died just before its parent's `calogExit(0)` had its failure +recorded second and thrown away -- whether the run reported the failure depended on which thread got +there first. (3) An error message a dying context posted during teardown was freed undelivered by +`contextDrainQueue`, losing both the diagnostic and the code. + +Fixed respectively by: claiming the exit code for a context the runner did not launch (without +closing it -- it belongs to whoever spawned it); replacing first-writer-wins with **first non-zero +wins, and zero stores nothing**, so ordering can no longer decide whether a failure is reported; and +dispatching a pending error at drain instead of freeing it, which is safe because every context +thread is joined by then. An aborted script is not reported as an error at all (S19), so what reaches +that drain is a genuine failure. + +*Verifier:* `taskSpawn` of a failing script followed by the parent's `calogExit()` reported **0 in +7 of 8 runs** before; **1 in 13 of 13** after, deterministically. Explicit `calogExit(3)` still +reports 3, and a clean run still reports 0. + +--- + +### S26. [bug] libs/calogRegex.c:368 --- FIXED + +**Status: FIXED** + +**`regexSplit` silently discarded subject bytes whenever the pattern could match empty.** + +One variable served as both the search position and the start of the piece being accumulated. On an +empty match the code advanced it by one byte to guarantee progress -- which also moved the piece +start, dropping that byte from the output entirely. Fixed by splitting the two roles: `searchFrom` +advances, `pieceStart` does not. + +*Verifier:* Demonstrated before the fix -- `regexSplit('x*', 'abc')` returned `['']` (the whole +subject gone), `regexSplit('[0-9]*', 'a1b2c')` returned `['', '', '']`. After: `['abc']` and +`['a','b','c']`. Three cases added to `tests/testUtil.c`, and the documented rule (an empty match is +not a separator) is now stated in API.md. + +--- + +### S27. [bug] libs/calogDb.c:749 --- FIXED + +**Status: FIXED** + +**`mysqlRowValue` wrote its NUL terminator at the connector's UNTRUNCATED length, one byte past a heap allocation.** + +Column buffers are sized from the field's `max_length`, but the connector reports the value's full +length even when it only copied `buffer_length` bytes -- and `max_length` under-reports for some +types (a zerofill `BIGINT` caps at its 19-digit display width but renders 20). The terminator then +landed outside the allocation, and `mysqlColumn` read past it as well. Fixed by clamping to the +capacity actually allocated, taken from calog's own `binds` array: the connector mutates its private +copy's `buffer_length` during conversion but never writes back to ours. + +*Verifier:* AddressSanitizer `heap-buffer-overflow ... WRITE of size 1` at `mysqlRowValue`, reproduced +against the shipped `bin/calog` through an ordinary Lua `dbQuery`, using a purpose-started local +MariaDB and a `bigint(19) unsigned zerofill` column. Reported as high, corrected to medium on the +grounds that it needs a specific column type -- though the verifier also noted a hostile or +MITM'd server can push the overflow much further via the TIME codec, and calog's default `sslmode` +does not verify certificates. + +--- + +### S28. [bug] libs/calogProc.c (env block, pipe guard) --- FIXED + +**Status: FIXED** + +**`procRun` handed the child a truncated environment and leaked the strings past the hole; separately, a failed `pipe()` leaked the descriptors already opened.** + +The environment vector was written at the pair's index, so any pair the loop skipped (a non-string +key or value, a failed malloc) left a NULL in the middle. `execve` stops at the first NULL, so one +skipped entry silently truncated -- often emptied -- the child's environment, and both cleanup loops +stopped at the same hole and leaked the rest. Fixed by writing through a compacted index. The pipe +guard was `pipe(a) != 0 || pipe(b) != 0 || pipe(c) != 0`: a short-circuit that abandoned whatever the +earlier calls had opened, so a process near its fd limit lost two descriptors per call until nothing +in it could open anything again. Fixed by staging the three calls, each failure closing its +predecessors, with the duplicated teardown factored into one helper. + +*Verifier:* Both demonstrated on the real binary -- the environment case with a numeric value in the +map (child environment empty, LeakSanitizer reporting the four leaked strings), the pipe case with an +LD_PRELOAD interposer forcing the second `pipe()` to fail. After the fix the child receives all four +valid variables and nothing leaks. + +--- + +### S29. [bug] libs/calogXml.c:435 --- FIXED + +**Status: FIXED** + +**Every failed parse of XML containing an internal entity leaked libxml2's entity table.** + +An internal entity declaration makes libxml2 build a hidden "SAX compatibility mode" document to hold +the entity table. libxml2 frees it itself only when the parse finishes cleanly; a fatal error halts +the parser first, and `xmlFreeParserCtxt` never owns it. Fixed by reclaiming `ctxt->myDoc` explicitly. + +*Verifier:* 847 bytes leaked per failed parse, perfectly linear (500 parses -> 423,500 bytes), and +~876 KB for a single document declaring 2000 entities. LeakSanitizer-clean after the fix across 200 +iterations. + +--- + +### S30. [gap] Makefile (cross-lint) --- FIXED + +**Status: FIXED** + +**The lint added to prevent an ABI-define divergence did not check the file the divergence came from.** + +`cross-lint` verified that every ABI define reached `tools/crossWinFull.sh` and +`tools/crossMacFull.sh`, but `tools/crossDeps.sh` -- which builds the cross `libmruby.a`, and is where +the original `MRB_USE_DEBUG_HOOK` mismatch (S11) actually occurred -- was never inspected. Fixed with +a separately scoped check, because folding it into the existing loops would demand that a script +compiling no calog sources contain `_SQ64` and every `CALOG_WITH_*` selector, flooding the lint with +false failures. + +*Verifier:* Teeth confirmed both ways: with `MRB_USE_DEBUG_HOOK` stripped from a copy of +`tools/crossDeps.sh` the lint now fails and names the file; restored, it passes. + +--- + +### S31. [bug] libs/calogArchive.c:278 --- FIXED + +**Status: FIXED** + +**`decompress` accepted a `filter` argument, never type-checked it, and ignored it completely.** + +The reader always sniffs every codec libarchive supports, so naming one changed nothing: a script +pinning `"gzip"` to reject other codecs on untrusted input silently accepted zstd, xz, bzip2 and plain +uncompressed bytes alike, and `decompress(data, 12345)` was accepted without complaint. Fixed by +enforcing it -- the filter actually used is compared against the caller's name after the header is +read, using libarchive's own names, which are exactly the ones `compress` accepts. Enforcing was +chosen over deleting the argument because it makes a documented feature true rather than removing a +capability scripts have reason to want. + +*Verifier:* Before: `decompress(gzipData, 'zstd')` returned the data. After: mismatched filter, bogus +name, non-string filter, and plain data named as gzip are all rejected, while the correct name and the +no-argument auto-detect still work. Four cases added to `tests/testArchive.c`. + +--- + +### S32. [bug] 13 library Register functions --- FIXED + +**Status: FIXED** + +**Thirteen libraries returned `calogOkE` while discarding the status of every `calogRegisterInline` call.** + +A failed registration (the OOM path `broker.c` already handles) left the runtime booting with a +native missing, so the failure surfaced much later and far from its cause as "no such function" -- +instead of the clean startup abort `main` is written to produce. Six libraries already checked. +Fixed by factoring the pattern rather than adding a thirteenth copy of it: a shared +`CalogNativeEntryT` table plus `calogRegisterBatch`, which stops at the first failure and returns its +status. `calogArchive` had already grown a private version of exactly this table; the shared one +replaces the need for twelve more. + +*Verifier:* Demonstrated with a `--wrap=strdup` probe failing one name allocation: +`calogCryptoRegister` returned success while `cryptoHmacSha256` was absent. Every library now returns +the failure. `make test` 966 checks / 0 failed. + +--- + +### S33. [dead-code] src/calog.h (calogLastTrace) --- FIXED + +**Status: FIXED (removed)** + +**`calogLastTrace` could not do what its own documentation described, and nothing used it.** + +Documented as formatting "the CALLING thread's current cross-context call chain" for use from inside a +native. The trace stack is thread-local and pushed by the *calling* thread before it marshals, so a +native running on the callee's thread -- which is where a script's native call actually executes -- +always sees an empty chain. Measured: a native invoked from inside a cross-context callable reported +length 0. It also had a real defect (on truncation the buffer held more text than the returned length +accounted for), zero callers, zero tests and no mention in API.md. Removed, like +`calogAbortCurrent` before it. The post-mortem chain carried in error messages -- which IS used and +tested -- remains the supported way to see a boundary. + +*Verifier:* Confirmed empirically before removing: a test written specifically to exercise it from +inside the cross-engine call path measured an empty trace, which is what turned this from "untested +API" into "API that cannot work as described". + +--- + +### S34. [bug] src/context.c (contextDrainQueue) --- FIXED + +**Status: FIXED** + +**A blocking cross-context call still queued at teardown was freed without ever waking its caller.** + +`contextDrainQueue` `messageFree`d a pending `messageCallE` without signalling its reply box, leaving +a foreign thread parked on a condition variable nothing could ever signal -- and since teardown joins +that thread, one stuck call became a process-wide hang. This is the concrete, verifiable half of a +reported three-way teardown deadlock. Fixed by replying `calogErrDeadE` instead of freeing, which +every caller of a cross-context native already handles. + +*Verifier:* The reported deadlock repro (a repeating timer whose callback calls a host-thread native, +interrupted mid-chunk) stops cleanly in ~1.0 s across 8 consecutive runs, and all TSan targets stay at +zero warnings. The broader structural claim behind that report -- that the before-contexts destroy +hooks join foreign threads while the host has stopped pumping -- is NOT closed by this and remains +open; see the note in the remaining-work list. + +--- + +### S35. [bug] src/context.c (contextReclaimCallables / calogContextUntrackFn) --- FIXED + +**Status: FIXED** + +**A foreign thread dropping a callable's last reference during a context's teardown stranded the engine handle inside an interpreter about to be destroyed -- the original QuickJS abort, from a direction the sweep did not cover.** + +`threadMain` closes the context's queue when `serveLoop` stops, then runs the per-context shutdown +hooks, and only then sweeps its callables. A last-drop landing anywhere in that gap cannot be +marshalled to the dying thread (the queue is closed), so `calogFnFinalizeForeign` runs it -- and that +path is deliberately barred from touching the interpreter, so it frees the adapter's struct WITHOUT +the engine release. The handle is then still live at `destroyInterpreter`, which is exactly what +QuickJS aborts on. + +Two distinct windows, and only one of them is what the report described. If the finalize has not yet +reached `calogContextUntrackFn`, the sweep still sees the entry -- with a refcount of zero, so +`calogFnRetainIfLive` refuses it and it was silently skipped. If the finalize gets through untrack +first, the entry is gone from the list and the sweep never sees it at all. The second is far wider +(as wide as the shutdown hooks take) and is the one a test can actually hit. + +Both are closed by the same mechanism, decided under the one lock they serialize on. `untrack` now +returns whether the finalize may proceed: while the owner has stopped serving but has not yet +reclaimed, it leaves the callable TRACKED and reports "adopted", so the finalize stops and hands its +shell over. The sweep then finds it and runs the engine release on the only thread permitted to. To +make the handoff safe in the first window too, `CalogFnT` gained a shell-holder count separate from +its reference count: the reference count reaching zero commits someone to finalizing, the holder +count says who may free the memory afterwards. The sweep takes a holder for an entry whose finalize +is still in flight, inherits the one handed across for an adopted entry, and the last party out +frees. + +*Verifier:* Reproduced DETERMINISTICALLY rather than by churn, which is what made this worth doing -- +the existing 20-round close-while-firing test never once reached the path (instrumented and counted: +0 hits in 30 runs). `tests/testDropInsideTheReclaimWindow` uses a per-context shutdown hook, which by +construction runs inside the window on the dying context's own thread, to park there until a separate +thread has dropped the host's last reference to a JavaScript closure. With the adoption branch +disabled the binary aborts on +`quickjs.c:2682: JS_FreeRuntime: Assertion 'list_empty(&rt->gc_obj_list)' failed` (SIGABRT, exit 134) +-- the same signature as S2. With it, 32 checks / 0 failed, 30 consecutive stress runs clean, and all +TSan targets at zero warnings. + +--- + +### S36. [bug] src/context.c:576 (calogDestroy) --- FIXED + +**Status: FIXED** + +**`calogDestroy` deadlocked outright when a background thread was mid-callback into a context: measured hanging on 4 runs in 5.** + +The `calogDestroyBeforeContextsE` hooks run on the host thread and stop background threads by +JOINING them. But the host, being inside `calogDestroy`, never calls `calogPump` again -- so any +host-directed CALL outstanding at that moment can never be served. With a timer callback in flight +that calls a host-thread native, the cycle closes on itself: the host waits on the timer thread, the +timer thread waits on the context serving its callback, and that context waits for a host reply +nobody is left to deliver. + +Fixed by closing host-directed CALLs and failing the ones already queued *before* the first hook +runs. The refusal is deliberately narrow -- CALLs only, since a CALL is the one kind whose sender +waits, and a fire-and-forget script error must still reach the handler that S25 restored. It is also +not a policy choice but a statement of fact: once the host thread is inside `calogDestroy` it will +never pump again, so refusing is the only honest answer to a call that would otherwise block forever. + +*Verifier:* Reproduced first (a 1 ms timer whose callback calls a non-inline native, then +`calogDestroy`): **4 of 5 runs hung**. After the fix, **0 of 10**. The regression test in +`tests/testTeardown.c` carries a watchdog so a recurrence is a named failure rather than a wedged +`make test`, and it catches the bug on **5 runs out of 5** when the fix is bypassed. + +**A process note worth more than the fix.** Several intermediate "the test does not reproduce it" +measurements in this work were wrong, and wrong in the same way: removing the fix left a static +function unused, `-Werror` failed the build, and each run silently exercised the STALE binary. Three +separate conclusions were drawn from that before the compiler error was noticed. When disabling a +fix to prove a test has teeth, the disable must keep the build green -- gate it at runtime, or check +the build output -- because a failed rebuild looks exactly like a test that passes. + +--- diff --git a/Makefile b/Makefile index fce47a99..48d4ff12 100644 --- a/Makefile +++ b/Makefile @@ -730,6 +730,7 @@ bin/testKv: obj/testKv.o obj/calogKv.o lib/libcalog.a lib/liblua.a | bin bin/testTimer: obj/testTimer.o obj/calogTimer.o lib/libcalog.a lib/liblua.a | bin $(CC) $(LDFLAGS) -pthread -o $@ $^ $(LUALIBS) + bin/testPubsub: obj/testPubsub.o obj/calogPubsub.o lib/libcalog.a lib/liblua.a | bin $(CC) $(LDFLAGS) -pthread -o $@ $^ $(LUALIBS) @@ -969,6 +970,15 @@ CROSSENV = ZIG="$(ZIGBIN)" CALOG_ZIG="$(ZIGBIN)" CROSSFULL = tools/crossWinFull.sh tools/crossMacFull.sh CROSSDEFS = _SQ64 SQUSEDOUBLE MB_DOUBLE_FLOAT MRB_USE_DEBUG_HOOK +# Defines that must ALSO reach the pinned-dep builder. tools/crossDeps.sh -- not the full-CLI +# scripts -- compiles the cross libmruby.a, and mruby records cc.defines in no generated header, so +# an adapter built with the define and an archive built without it link cleanly and then disagree +# about the layout of mrb_state at runtime. This is checked separately from CROSSDEFS because +# crossDeps.sh compiles no calog sources and knows nothing about _SQ64 or the CALOG_WITH_* selectors: +# folding it into CROSSFULL would flood the lint with false failures. +CROSSDEPFILES = tools/crossDeps.sh src/mruby/build_config.rb +CROSSDEPDEFS = MRB_USE_DEBUG_HOOK + # Zig-free, hermetic, sub-second. Every calog source the native build compiles must also be compiled # by both full-CLI cross scripts, and every ABI define + engine selector must reach them. Text-level # on purpose: it needs no toolchain, so `make test` can run it. @@ -985,6 +995,11 @@ cross-lint: grep -q -- "$$d" $$s || { echo "cross-lint: -D$$d is missing from $$s" >&2; bad=1; }; \ done; \ done; \ + for d in $(CROSSDEPDEFS); do \ + for s in $(CROSSDEPFILES); do \ + grep -q -- "$$d" $$s || { echo "cross-lint: -D$$d is missing from $$s" >&2; bad=1; }; \ + done; \ + done; \ if [ $$bad -eq 0 ]; then echo "== cross-lint ok: cross scripts cover every calog source, ABI define and engine =="; \ else echo "== cross-lint FAILED: tools/cross*Full.sh drifted from the native build; fix them or run 'make cross' ==" >&2; exit 1; fi diff --git a/design.md b/design.md index 9b59d9a8..0fd4d4dd 100644 --- a/design.md +++ b/design.md @@ -1661,10 +1661,40 @@ What ends one script is therefore `taskExit` (deferred, and the only mechanism t `tests/testTeardown.c` drives the early-context-death reclaim path through `calogCurrentRetire` -- the same call `taskExit` makes -- rather than through an API nothing else used. +### The window the sweep does not see by itself + +`threadMain` closes the queue when `serveLoop` stops, runs the per-context shutdown hooks, and only +then sweeps. A foreign last-drop landing in that gap cannot be marshalled to the dying thread, so it +finalizes on the dropping thread -- which is barred from touching the interpreter and therefore skips +the engine release, stranding the handle in a VM about to be destroyed. That is the sec 25 crash +again, reached from a direction the sweep did not cover. + +Two windows, not one. If the finalize has not yet reached `calogContextUntrackFn`, the sweep sees the +entry with a refcount of zero and used to skip it (`calogFnRetainIfLive` refuses, correctly -- see +sec 28). If the finalize gets through untrack first, the entry is gone and the sweep never sees it. +The second is as wide as the shutdown hooks take, and is the one that reproduces. + +Both close on the single lock they serialize on. `calogContextUntrackFn` now reports whether the +finalize may proceed: while the owner has stopped serving and has not yet reclaimed, it leaves the +callable TRACKED and answers no, so the finalize stops and hands the callable to the sweep -- the one +thread allowed to run that release. For the first window, where a finalize is still in flight and +will arrive later, `CalogFnT` gained a **shell-holder count separate from its reference count**: the +reference count reaching zero commits someone to finalizing, the holder count says who may free the +memory afterwards. The sweep takes a holder when a finalize is still coming, inherits the one handed +across when the callable was adopted, and whoever is last frees. Without that split there is no safe +answer -- either the sweep frees the shell while the finalize is about to read it, or the reverse. + ### Test `tests/testTeardown.c` uses JavaScript throughout, because on any other engine a stranded handle is -invisible -- there, surviving the teardown IS the assertion. It covers a subscriber, an export and a +invisible -- there, surviving the teardown IS the assertion. + +The window above is tested deterministically rather than by churn, and that distinction earned its +keep: the existing 20-round close-while-firing case never reached the path once (instrumented and +counted -- 0 hits in 30 runs). A per-context shutdown hook runs, by construction, inside the window +on the dying context's own thread, so parking there until another thread drops the host's last +reference reproduces it every time. With the adoption branch disabled the binary aborts on the +familiar `list_empty(&rt->gc_obj_list)`. It covers a subscriber, an export and a timer callback left registered at `calogDestroy`; each of those whose script instead errors out first; a script that ends itself; a closure handed to a Lua script whose owner then dies (invoking it afterwards must fail cleanly, not reach into a destroyed VM); and @@ -2065,3 +2095,106 @@ Makefile path was missed because nothing built it. So `static` is now a prerequi It costs a handful of -O2 objects and a link -- the vendored archives are shared with the rest of the tree and OpenSSL is already required by `bin/testNet` -- and it is the only thing that stops this particular rot recurring a third time. + +--- + +## 31. Failures with nowhere to go, a teardown that could not finish, and a parameter channel + +### Failures that had nowhere to go + +Three of the defects closed here were the same bug in different clothes, and the same one this whole +stretch of work started with: a failure that reaches nobody. + +- A **timer or pubsub callback** that failed was discarded outright. Both invoke a script callable + from a thread with no caller to return a status to, and both looked at that status only to notice a + dead context. `psPublish` returns a delivery COUNT, so a broken subscriber was invisible from every + direction at once. Both now route the failure through `calogPostError`, the same path a failed eval + already used. +- A **spawned task's** failure could not set the exit code at all: the runner only flagged contexts it + had launched itself, and a task belongs to the script that spawned it. +- An **error posted during teardown** was freed undelivered by `contextDrainQueue`, taking the + diagnostic and the exit code with it. + +The exit-code rule changed with them. It was "whoever asks first names it", which sounds like it +protects a failure and does not: a task that died just before its parent's `calogExit(0)` had its +failure recorded second and dropped, so whether the run reported it depended on which thread won a +race. It is now **first non-zero wins, and zero stores nothing** -- a failure cannot be masked, and +ordering no longer decides. This is safe precisely because an aborted script is not reported as an +error (sec 29), so what arrives during a teardown is genuine. + +The same drain had a second victim: a blocking cross-context CALL still queued when its target died +was freed without signalling its reply box, leaving a foreign thread parked forever on a condition +nothing could raise -- and teardown joins that thread, so one stuck call hung the process. It now +replies `calogErrDeadE`. + +### Teardown that cannot join what it is waiting on + +`calogDestroy` runs the before-contexts hooks on the host thread, and those hooks stop background +threads by JOINING them. The host is inside `calogDestroy` by then, so it never calls `calogPump` +again -- which makes every host-directed CALL outstanding at that moment unservable. With a timer +callback in flight that calls a host-thread native, the cycle closes: the host waits on the timer +thread, the timer thread waits on the context serving its callback, and that context waits for a host +reply nobody is left to deliver. Measured at 4 hangs in 5 runs. + +The fix is to close host-directed CALLs, and fail the ones already queued, before the first hook +runs. Narrow on purpose: CALLs only, because a CALL is the one message kind whose sender waits, and a +fire-and-forget script error must still reach the handler. It is less a policy than an admission -- +once the host thread enters `calogDestroy` it will never pump again, so a call arriving after that +was never going to be answered, and failing it immediately is the only honest response. + +### A parameter channel + +Sealing the engines (sec 30) removed `getenv` along with everything else, which left scripts with no +sanctioned way to receive a parameter -- every project would have invented its own convention out of +files or the kv store. `bin/calog` now takes `--` on the command line: everything before it is a +script, everything after is data, readable from any engine as `calogArgs()`. Registered inline, since +it reads immutable startup state and has nothing to serialize on the host thread. + +That the gap existed at all is the honest cost of sec 30, and worth stating plainly: removing a +capability from ten engines is only half a decision. The other half is providing the one thing the +removal actually took away, in a form the host controls. + +--- + +## 32. What the unverified leads turned out to be + +The review sweep behind sec 30 produced seventeen candidates and a verification budget of eight. That +cap was a choice, and it deferred nine findings rather than dismissing them. When the remaining seven +were finally put through the same adversarial verification, **all seven were real, and every one was +demonstrated by running something rather than by reading**. The lesson is not that the reviewers were +right -- it is that "unverified" had been quietly filed as "probably nothing", and it was not. + +None of them were in the actor core, which is the other thing worth noticing. They were in the +libraries, where a defect is quieter: nothing asserts, no sanitizer necessarily fires, and the +symptom is a leak, a truncated environment, or an argument that silently does nothing. + +- **A heap overflow reachable from an ordinary query.** `mysqlRowValue` wrote its NUL at the length + the connector reported, which is the value's UNTRUNCATED length even when only `buffer_length` + bytes were copied -- and the buffer is sized from `max_length`, which under-reports for some types + (a zerofill `BIGINT` caps at its 19-digit display width but renders 20). The clamp uses calog's own + `binds` array, not the connector's: the connector mutates its private copy's `buffer_length` during + conversion and never writes back to ours. +- **A child process with an empty environment.** `procRun` wrote its environment vector at each + pair's index, so any pair it skipped left a NULL in the middle. `execve` stops at the first NULL. + One non-string value emptied the child's environment and leaked every string past the hole. +- **Two leaks and a lost diagnostic**, each small and each permanent: libxml2's hidden entity-table + document on every failed parse, the pipe descriptors a short-circuited `pipe(a) || pipe(b)` guard + abandoned, and `regexSplit` dropping a subject byte per empty match. +- **A documented argument that did nothing.** `decompress(data, filter)` accepted a filter, never + type-checked it, and ignored it -- so a script pinning `"gzip"` against untrusted input accepted + every codec libarchive supports. This one is where the usual rule (leave it out rather than ship a + partial) pointed the other way: the capability is documented and genuinely wanted, so making it + true beat removing it. The filter is now compared against the one actually used, with libarchive's + own names, which are exactly the ones `compress` accepts. +- **The lint that did not check the file the bug came from.** `cross-lint` verified that every + ABI-visible define reached both full-CLI cross scripts, but not `tools/crossDeps.sh` -- which is + where the `MRB_USE_DEBUG_HOOK` divergence of sec 27 actually happened. It needed a separately + scoped check, since that script compiles no calog sources and folding it into the existing loops + would demand `_SQ64` and every `CALOG_WITH_*` selector be present in it. +- **Thirteen libraries reporting success on a partial registration.** Each discarded the status of + every `calogRegisterInline` call, so a failed registration surfaced much later as "no such + function" instead of the clean startup abort `main` is written to produce. The fix was to stop + writing the pattern: a shared `CalogNativeEntryT` table plus `calogRegisterBatch`, which stops at + the first failure and returns it. `calogArchive` had already grown a private version of exactly + that table -- a good sign the shared one was overdue, and that twelve more copies were the wrong + direction. diff --git a/examples/scripts/README.md b/examples/scripts/README.md index 15d51c59..edf86da5 100644 --- a/examples/scripts/README.md +++ b/examples/scripts/README.md @@ -25,6 +25,9 @@ Conventions every example follows: not return: the statement after it never runs, and the first code asked for is the one the process reports. `Ctrl-C` ends a run the same way, aborting the scripts rather than waiting for them to finish. +- **`calogArgs()`** returns the arguments given after `--` (`bin/calog build.lua -- release v2`), + the same list for every script in the run. It is the only parameter channel: the engines' own + `getenv`/`argv` are not available. - Extensions map to engines: `.lua .js .nut .bas .be .scm .wren`. ## `languages/` -- one guided tour per engine diff --git a/libs/calogArchive.c b/libs/calogArchive.c index 293d9d3c..770487b7 100644 --- a/libs/calogArchive.c +++ b/libs/calogArchive.c @@ -278,6 +278,9 @@ static int32_t arcDecompress(CalogValueT *args, int32_t argCount, CalogValueT *r if (argCount < 1 || argCount > 2 || args[0].type != calogStringE) { return calogFail(result, calogErrArgE, "decompress expects (data [, filter])"); } + if (argCount == 2 && args[1].type != calogStringE) { + return calogFail(result, calogErrArgE, "decompress: filter must be a string"); + } archive = archive_read_new(); if (archive == NULL) { return calogFail(result, calogErrOomE, "decompress: out of memory"); @@ -295,6 +298,20 @@ static int32_t arcDecompress(CalogValueT *args, int32_t argCount, CalogValueT *r archive_read_free(archive); return status; } + // A named filter is ENFORCED, not decoration. The reader always sniffs every codec libarchive + // supports, so naming one used to change nothing at all: a script pinning "gzip" to reject other + // codecs silently accepted zstd, xz, bzip2 and plain uncompressed bytes alike. The names are + // libarchive's own, so they are exactly the ones compress() accepts. + if (argCount == 2) { + const char *actual; + + actual = archive_filter_name(archive, 0); + if (actual == NULL || strcmp(actual, args[1].as.s.bytes) != 0) { + status = calogFail(result, calogErrArgE, "decompress: data is not in the named filter's format"); + archive_read_free(archive); + return status; + } + } memset(&out, 0, sizeof(out)); status = calogOkE; for (;;) { diff --git a/libs/calogCrypto.c b/libs/calogCrypto.c index 922d9d06..fce25562 100644 --- a/libs/calogCrypto.c +++ b/libs/calogCrypto.c @@ -30,19 +30,25 @@ static int32_t cryptoUuidNative(CalogValueT *args, int32_t argCount, CalogValueT // The single source of truth for lowercase hex digit rendering, shared by cryptoBytesToHex // and cryptoUuidNative. static const char cryptoHexDigits[] = "0123456789abcdef"; +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gCryptoNatives[] = { + { "cryptoHashSha256", cryptoHashSha256Native }, + { "cryptoHashSha1", cryptoHashSha1Native }, + { "cryptoHmacSha256", cryptoHmacSha256Native }, + { "cryptoRandomBytes", cryptoRandomBytesNative }, + { "cryptoBase64Encode", cryptoBase64EncodeNative }, + { "cryptoBase64Decode", cryptoBase64DecodeNative }, + { "cryptoHexEncode", cryptoHexEncodeNative }, + { "cryptoHexDecode", cryptoHexDecodeNative }, + { "cryptoUuid", cryptoUuidNative }, +}; + + int32_t calogCryptoRegister(CalogT *calog) { - calogRegisterInline(calog, "cryptoHashSha256", cryptoHashSha256Native, NULL); - calogRegisterInline(calog, "cryptoHashSha1", cryptoHashSha1Native, NULL); - calogRegisterInline(calog, "cryptoHmacSha256", cryptoHmacSha256Native, NULL); - calogRegisterInline(calog, "cryptoRandomBytes", cryptoRandomBytesNative, NULL); - calogRegisterInline(calog, "cryptoBase64Encode", cryptoBase64EncodeNative, NULL); - calogRegisterInline(calog, "cryptoBase64Decode", cryptoBase64DecodeNative, NULL); - calogRegisterInline(calog, "cryptoHexEncode", cryptoHexEncodeNative, NULL); - calogRegisterInline(calog, "cryptoHexDecode", cryptoHexDecodeNative, NULL); - calogRegisterInline(calog, "cryptoUuid", cryptoUuidNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gCryptoNatives, (int64_t)(sizeof(gCryptoNatives) / sizeof(gCryptoNatives[0])), NULL); } diff --git a/libs/calogDb.c b/libs/calogDb.c index a4a41a17..6068c204 100644 --- a/libs/calogDb.c +++ b/libs/calogDb.c @@ -382,6 +382,7 @@ typedef union MysqlScalarT { // Row context bound to mysqlRowName/mysqlRowValue for one mysqlQuery call. typedef struct MysqlRowCtxT { MYSQL_FIELD *fields; + MYSQL_BIND *binds; // OUR array: buffer_length here is exactly what was allocated char **buffers; unsigned long *lengths; my_bool *isNull; @@ -706,6 +707,7 @@ static int32_t mysqlQuery(MYSQL *conn, const CalogValueT *sqlValue, const CalogV return calogFail(result, status, "dbQuery: out of memory"); } rowCtx.fields = fields; + rowCtx.binds = binds; rowCtx.buffers = buffers; rowCtx.lengths = lengths; rowCtx.isNull = isNull; @@ -740,14 +742,25 @@ static void mysqlRowName(void *ctx, int32_t column, const char **nameOut, int64_ static int32_t mysqlRowValue(void *ctx, int32_t column, CalogValueT *out) { - MysqlRowCtxT *rowCtx; + MysqlRowCtxT *rowCtx; + unsigned long length; rowCtx = (MysqlRowCtxT *)ctx; if (rowCtx->isNull[column]) { return calogOkE; } - rowCtx->buffers[column][rowCtx->lengths[column]] = '\0'; - return mysqlColumn(rowCtx->fields[column].type, rowCtx->buffers[column], rowCtx->lengths[column], out); + // The connector reports the value's UNTRUNCATED length even when it only copied buffer_length + // bytes, and the buffer is sized from the field's max_length -- which under-reports for some + // types (a zerofill BIGINT caps max_length at its 19-digit display width but renders 20). The + // terminator then lands one byte past the allocation, and mysqlColumn reads past it too. Clamp + // to the capacity WE allocated. Our own binds array is the right source: the connector mutates + // its private copy's buffer_length during conversion, but never writes back to this one. + length = rowCtx->lengths[column]; + if (length >= rowCtx->binds[column].buffer_length) { + length = (unsigned long)(rowCtx->binds[column].buffer_length - 1); + } + rowCtx->buffers[column][length] = '\0'; + return mysqlColumn(rowCtx->fields[column].type, rowCtx->buffers[column], length, out); } #endif diff --git a/libs/calogExport.c b/libs/calogExport.c index bc5acb27..ced60ebf 100644 --- a/libs/calogExport.c +++ b/libs/calogExport.c @@ -44,17 +44,29 @@ static int32_t exportRemove(CalogValueT *args, int32_t argCount, CalogValueT *re static int32_t exportResolve(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t exportResolveByFind(CalogValueT *args, int32_t argCount, CalogValueT *result, int32_t (*find)(const char *, int64_t)); static int32_t exportResolveFold(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gExportNatives[] = { + { "calogExport", exportPublish }, + { "calogUnexport", exportRemove }, + { "calogCall", exportCall }, + { "__calogExportResolve", exportResolve }, + // Case-insensitive variant: my-basic uppercases every identifier at parse time, so its + // bare-name resolver matches an export regardless of the case it was registered with. + { "__calogExportResolveFold", exportResolveFold }, +}; + + int32_t calogExportRegister(CalogT *calog) { + int32_t status; + calogRegistryRetain(&gInitMutex, &gRefCount); - calogRegisterInline(calog, "calogExport", exportPublish, NULL); - calogRegisterInline(calog, "calogUnexport", exportRemove, NULL); - calogRegisterInline(calog, "calogCall", exportCall, NULL); - calogRegisterInline(calog, "__calogExportResolve", exportResolve, NULL); - // Case-insensitive variant: my-basic uppercases every identifier at parse time, so its - // bare-name resolver matches an export regardless of the case it was registered with. - calogRegisterInline(calog, "__calogExportResolveFold", exportResolveFold, NULL); + status = calogRegisterBatch(calog, gExportNatives, (int64_t)(sizeof(gExportNatives) / sizeof(gExportNatives[0])), NULL); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogExportShutdown, calogDestroyAfterContextsE); } diff --git a/libs/calogFs.c b/libs/calogFs.c index ea209fdc..31e5ca2b 100644 --- a/libs/calogFs.c +++ b/libs/calogFs.c @@ -55,18 +55,24 @@ static int32_t fsReadNative(CalogValueT *args, int32_t argCount, CalogValueT *re static int32_t fsRemoveNative(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t fsStatNative(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t fsWriteNative(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gFsNatives[] = { + { "fsRead", fsReadNative }, + { "fsWrite", fsWriteNative }, + { "fsAppend", fsAppendNative }, + { "fsExists", fsExistsNative }, + { "fsRemove", fsRemoveNative }, + { "fsMkdir", fsMkdirNative }, + { "fsList", fsListNative }, + { "fsStat", fsStatNative }, +}; + + int32_t calogFsRegister(CalogT *calog) { - calogRegisterInline(calog, "fsRead", fsReadNative, NULL); - calogRegisterInline(calog, "fsWrite", fsWriteNative, NULL); - calogRegisterInline(calog, "fsAppend", fsAppendNative, NULL); - calogRegisterInline(calog, "fsExists", fsExistsNative, NULL); - calogRegisterInline(calog, "fsRemove", fsRemoveNative, NULL); - calogRegisterInline(calog, "fsMkdir", fsMkdirNative, NULL); - calogRegisterInline(calog, "fsList", fsListNative, NULL); - calogRegisterInline(calog, "fsStat", fsStatNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gFsNatives, (int64_t)(sizeof(gFsNatives) / sizeof(gFsNatives[0])), NULL); } diff --git a/libs/calogHttp.c b/libs/calogHttp.c index 51a75c18..e3380d05 100644 --- a/libs/calogHttp.c +++ b/libs/calogHttp.c @@ -141,6 +141,14 @@ static bool httpSameOrigin(const HttpUrlT *a, const HttpUrlT *b); static void httpSetTimeouts(CalogSocketT fd, int timeoutSec); static int32_t httpTlsHandshake(HttpConnT *conn, const HttpUrlT *url, CalogValueT *result); static int httpWriteAuthority(char *out, size_t cap, const char *scheme, const HttpUrlT *base); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gHttpNatives[] = { + { "httpGet", httpGetNative }, + { "httpRequest", httpRequestNative }, +}; + + int32_t calogHttpRegister(CalogT *calog) { @@ -156,9 +164,7 @@ int32_t calogHttpRegister(CalogT *calog) { // Windows has no SIGPIPE (a reset/closed peer surfaces as a normal send/recv error instead). signal(SIGPIPE, SIG_IGN); #endif - calogRegisterInline(calog, "httpGet", httpGetNative, NULL); - calogRegisterInline(calog, "httpRequest", httpRequestNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gHttpNatives, (int64_t)(sizeof(gHttpNatives) / sizeof(gHttpNatives[0])), NULL); } diff --git a/libs/calogJson.c b/libs/calogJson.c index a407b79b..cf312b9c 100644 --- a/libs/calogJson.c +++ b/libs/calogJson.c @@ -50,12 +50,18 @@ static int32_t jsonParseValue(JsonParseT *p, CalogValueT *out, int32_t depth); static int32_t jsonPutUtf8(JsonBufT *buf, uint32_t cp); static void jsonSkipWs(JsonParseT *p); static int32_t jsonStringifyNative(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gJsonNatives[] = { + { "jsonParse", jsonParseNative }, + { "jsonStringify", jsonStringifyNative }, +}; + + int32_t calogJsonRegister(CalogT *calog) { - calogRegisterInline(calog, "jsonParse", jsonParseNative, NULL); - calogRegisterInline(calog, "jsonStringify", jsonStringifyNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gJsonNatives, (int64_t)(sizeof(gJsonNatives) / sizeof(gJsonNatives[0])), NULL); } diff --git a/libs/calogKv.c b/libs/calogKv.c index a39aae05..3dc618ea 100644 --- a/libs/calogKv.c +++ b/libs/calogKv.c @@ -47,15 +47,27 @@ static int32_t kvGet(CalogValueT *args, int32_t argCount, CalogValueT *result, v static int32_t kvHas(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t kvKeys(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t kvSet(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gKvNatives[] = { + { "kvSet", kvSet }, + { "kvGet", kvGet }, + { "kvHas", kvHas }, + { "kvDelete", kvDelete }, + { "kvKeys", kvKeys }, +}; + + int32_t calogKvRegister(CalogT *calog) { + int32_t status; + calogRegistryRetain(&gInitMutex, &gRefCount); - calogRegisterInline(calog, "kvSet", kvSet, NULL); - calogRegisterInline(calog, "kvGet", kvGet, NULL); - calogRegisterInline(calog, "kvHas", kvHas, NULL); - calogRegisterInline(calog, "kvDelete", kvDelete, NULL); - calogRegisterInline(calog, "kvKeys", kvKeys, NULL); + status = calogRegisterBatch(calog, gKvNatives, (int64_t)(sizeof(gKvNatives) / sizeof(gKvNatives[0])), NULL); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogKvShutdown, calogDestroyAfterContextsE); } diff --git a/libs/calogProc.c b/libs/calogProc.c index 9715000c..fce0d179 100644 --- a/libs/calogProc.c +++ b/libs/calogProc.c @@ -48,6 +48,9 @@ typedef struct ProcWinStdinT { #endif static int32_t procBufAppend(ProcBufT *buffer, const void *bytes, size_t length); +#ifndef _WIN32 +static void procFreeSpawn(char **argv, char **builtEnv); +#endif static CalogValueT *procOpt(CalogAggT *opts, const char *name); static int32_t procResult(CalogValueT *result, int32_t exitCode, ProcBufT *out, ProcBufT *err); static int32_t procRun(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); @@ -83,6 +86,26 @@ static int32_t procBufAppend(ProcBufT *buffer, const void *bytes, size_t length) } +#ifndef _WIN32 + +// Release the argv vector and, when procRun built one, the replacement environment block. The +// terminating NULL is the vector's own, so this is only correct because the build loop compacts -- +// a hole would stop it early and leak the tail. +static void procFreeSpawn(char **argv, char **builtEnv) { + int64_t index; + + free(argv); + if (builtEnv != NULL) { + for (index = 0; builtEnv[index] != NULL; index++) { + free(builtEnv[index]); + } + free(builtEnv); + } +} + +#endif + + static CalogValueT *procOpt(CalogAggT *opts, const char *name) { CalogValueT key; CalogValueT *field; @@ -514,9 +537,11 @@ static int32_t procRun(CalogValueT *args, int32_t argCount, CalogValueT *result, if (field != NULL && field->type == calogAggE && calogAggIsKeyed(field->as.agg)) { CalogAggT *envMap; int64_t envCount; + int64_t envUsed; int64_t e; envMap = field->as.agg; envCount = envMap->pairCount; + envUsed = 0; builtEnv = (char **)calloc((size_t)envCount + 1, sizeof(char *)); if (builtEnv == NULL) { free(argv); @@ -534,26 +559,43 @@ static int32_t procRun(CalogValueT *args, int32_t argCount, CalogValueT *result, v = envMap->pairs[e].value.as.s.bytes; klen = (size_t)envMap->pairs[e].key.as.s.length; vlen = (size_t)envMap->pairs[e].value.as.s.length; - builtEnv[e] = (char *)malloc(klen + vlen + 2); - if (builtEnv[e] == NULL) { + // Written through envUsed, NOT e: a pair this loop skips (a non-string key or + // value, or a failed malloc) must not leave a NULL hole in the middle of the + // vector. execve stops at the first NULL, so one skipped entry silently handed the + // child a truncated -- often empty -- environment, and the cleanup loops below stop + // at the same hole and leaked everything past it. + builtEnv[envUsed] = (char *)malloc(klen + vlen + 2); + if (builtEnv[envUsed] == NULL) { continue; } - memcpy(builtEnv[e], k, klen); - builtEnv[e][klen] = '='; - memcpy(builtEnv[e] + klen + 1, v, vlen); - builtEnv[e][klen + 1 + vlen] = '\0'; + memcpy(builtEnv[envUsed], k, klen); + builtEnv[envUsed][klen] = '='; + memcpy(builtEnv[envUsed] + klen + 1, v, vlen); + builtEnv[envUsed][klen + 1 + vlen] = '\0'; + envUsed++; } envp = builtEnv; } } - if (pipe(inPipe) != 0 || pipe(outPipe) != 0 || pipe(errPipe) != 0) { - free(argv); - if (builtEnv != NULL) { - for (i = 0; builtEnv[i] != NULL; i++) { - free(builtEnv[i]); - } - free(builtEnv); - } + // Staged, not short-circuited: `pipe(a) || pipe(b) || pipe(c)` leaked whatever the earlier + // calls had already opened, so a process at its fd limit lost two descriptors per procRun until + // nothing in the process could open anything again. + if (pipe(inPipe) != 0) { + procFreeSpawn(argv, builtEnv); + return calogFail(result, calogErrArgE, "procRun: could not create pipes"); + } + if (pipe(outPipe) != 0) { + close(inPipe[0]); + close(inPipe[1]); + procFreeSpawn(argv, builtEnv); + return calogFail(result, calogErrArgE, "procRun: could not create pipes"); + } + if (pipe(errPipe) != 0) { + close(inPipe[0]); + close(inPipe[1]); + close(outPipe[0]); + close(outPipe[1]); + procFreeSpawn(argv, builtEnv); return calogFail(result, calogErrArgE, "procRun: could not create pipes"); } posix_spawn_file_actions_init(&actions); @@ -574,13 +616,7 @@ static int32_t procRun(CalogValueT *args, int32_t argCount, CalogValueT *result, close(inPipe[0]); close(outPipe[1]); close(errPipe[1]); - free(argv); - if (builtEnv != NULL) { - for (i = 0; builtEnv[i] != NULL; i++) { - free(builtEnv[i]); - } - free(builtEnv); - } + procFreeSpawn(argv, builtEnv); if (spawnRc != 0) { close(inPipe[1]); close(outPipe[0]); diff --git a/libs/calogPubsub.c b/libs/calogPubsub.c index e674aea7..47fe5cdd 100644 --- a/libs/calogPubsub.c +++ b/libs/calogPubsub.c @@ -52,13 +52,25 @@ static void pubsubFreeAll(void); static int32_t pubsubPublish(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t pubsubSubscribe(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t pubsubUnsubscribe(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gPubsubNatives[] = { + { "psSubscribe", pubsubSubscribe }, + { "psUnsubscribe", pubsubUnsubscribe }, + { "psPublish", pubsubPublish }, +}; + + int32_t calogPubsubRegister(CalogT *calog) { + int32_t status; + calogRegistryRetain(&gInitMutex, &gRefCount); - calogRegisterInline(calog, "psSubscribe", pubsubSubscribe, NULL); - calogRegisterInline(calog, "psUnsubscribe", pubsubUnsubscribe, NULL); - calogRegisterInline(calog, "psPublish", pubsubPublish, NULL); + status = calogRegisterBatch(calog, gPubsubNatives, (int64_t)(sizeof(gPubsubNatives) / sizeof(gPubsubNatives[0])), NULL); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogPubsubShutdown, calogDestroyAfterContextsE); } @@ -144,6 +156,12 @@ static int32_t pubsubPublish(CalogValueT *args, int32_t argCount, CalogValueT *r continue; } status = calogFnInvoke(collected[index], &messageCopy, 1, &callResult); + // A subscriber that FAILED is reported. publish returns a delivery COUNT, not a status, so + // without this a broken handler was invisible to everyone: the publisher saw a number, the + // handler's own author saw nothing at all. Read the owner before the release below. + if (status != calogOkE && status != calogErrDeadE && status != calogErrNotFoundE) { + calogPostError(calogFnRuntime(collected[index]), calogFnOwner(collected[index]), &callResult); + } calogValueFree(&callResult); calogValueFree(&messageCopy); calogFnRelease(collected[index]); diff --git a/libs/calogRegex.c b/libs/calogRegex.c index bd06f2cb..edb3dcf9 100644 --- a/libs/calogRegex.c +++ b/libs/calogRegex.c @@ -315,7 +315,8 @@ static int32_t regexSplit(CalogValueT *args, int32_t argCount, CalogValueT *resu const char *flags; int64_t subjectLen; int64_t flagsLen; - PCRE2_SIZE offset; + PCRE2_SIZE pieceStart; + PCRE2_SIZE searchFrom; bool global; int32_t status; @@ -346,14 +347,20 @@ static int32_t regexSplit(CalogValueT *args, int32_t argCount, CalogValueT *resu } subject = args[1].as.s.bytes; subjectLen = args[1].as.s.length; - offset = 0; + // Two distinct positions, which used to be one variable and was the bug: pieceStart is where the + // piece being accumulated begins, searchFrom is where the next match is looked for. They differ + // only after an empty match, where the search must advance to make progress but the piece must + // NOT -- sharing them threw away one subject byte per empty match, so any pattern that can match + // empty (`x*`, `[0-9]*`) silently lost most of the subject. + pieceStart = 0; + searchFrom = 0; while (status == calogOkE) { PCRE2_SIZE *ovector; PCRE2_SIZE matchStart; PCRE2_SIZE matchEnd; CalogValueT piece; int rc; - rc = pcre2_match(code, (PCRE2_SPTR)subject, (PCRE2_SIZE)subjectLen, offset, 0, matchData, NULL); + rc = pcre2_match(code, (PCRE2_SPTR)subject, (PCRE2_SIZE)subjectLen, searchFrom, 0, matchData, NULL); if (rc < 0) { break; } @@ -361,25 +368,27 @@ static int32_t regexSplit(CalogValueT *args, int32_t argCount, CalogValueT *resu matchStart = ovector[0]; matchEnd = ovector[1]; if (matchEnd == matchStart) { - // Empty match: advance one byte so the split terminates rather than looping. + // An empty match is not a separator: advance the SEARCH one byte so the loop terminates, + // and leave the piece where it started so the byte we stepped over is still part of it. if (matchStart >= (PCRE2_SIZE)subjectLen) { break; } - offset = matchStart + 1; + searchFrom = matchStart + 1; continue; } - status = calogValueString(&piece, subject + offset, (int64_t)(matchStart - offset)); + status = calogValueString(&piece, subject + pieceStart, (int64_t)(matchStart - pieceStart)); if (status == calogOkE) { status = calogAggPush(pieces, &piece); if (status != calogOkE) { calogValueFree(&piece); } } - offset = matchEnd; + pieceStart = matchEnd; + searchFrom = matchEnd; } if (status == calogOkE) { CalogValueT tail; - status = calogValueString(&tail, subject + offset, (int64_t)(subjectLen - (int64_t)offset)); + status = calogValueString(&tail, subject + pieceStart, (int64_t)(subjectLen - (int64_t)pieceStart)); if (status == calogOkE) { status = calogAggPush(pieces, &tail); if (status != calogOkE) { diff --git a/libs/calogSsh.c b/libs/calogSsh.c index b9dfe28b..a76cdbb1 100644 --- a/libs/calogSsh.c +++ b/libs/calogSsh.c @@ -94,9 +94,28 @@ static ssize_t sshReadSftpFile(void *handle, int streamId, char *buffer, size_t static int32_t sshResolve(SshLibT *lib, int64_t handle, CalogValueT *result, SshConnT **out); static void sshSessionDestroy(LIBSSH2_SESSION *session, CalogSocketT sock, const char *reason); static void sshWaitSocket(CalogSocketT sock, LIBSSH2_SESSION *session); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gSshNatives[] = { + { "sshConnect", sshConnect }, + { "sshAuthPassword", sshAuthPassword }, + { "sshAuthKey", sshAuthKey }, + { "sshExec", sshExec }, + { "sshClose", sshClose }, + { "sftpGet", sftpGet }, + { "sftpPut", sftpPut }, + { "sftpList", sftpList }, + { "sftpStat", sftpStat }, + { "sftpRemove", sftpRemove }, + { "sftpMkdir", sftpMkdir }, +}; + + int32_t calogSshRegister(CalogT *calog) { + int32_t status; + pthread_mutex_lock(&gSshLibMutex); if (gSshLib == NULL) { SshLibT *lib; @@ -130,17 +149,10 @@ int32_t calogSshRegister(CalogT *calog) { } gSshLib->refCount++; pthread_mutex_unlock(&gSshLibMutex); - calogRegisterInline(calog, "sshConnect", sshConnect, gSshLib); - calogRegisterInline(calog, "sshAuthPassword", sshAuthPassword, gSshLib); - calogRegisterInline(calog, "sshAuthKey", sshAuthKey, gSshLib); - calogRegisterInline(calog, "sshExec", sshExec, gSshLib); - calogRegisterInline(calog, "sshClose", sshClose, gSshLib); - calogRegisterInline(calog, "sftpGet", sftpGet, gSshLib); - calogRegisterInline(calog, "sftpPut", sftpPut, gSshLib); - calogRegisterInline(calog, "sftpList", sftpList, gSshLib); - calogRegisterInline(calog, "sftpStat", sftpStat, gSshLib); - calogRegisterInline(calog, "sftpRemove", sftpRemove, gSshLib); - calogRegisterInline(calog, "sftpMkdir", sftpMkdir, gSshLib); + status = calogRegisterBatch(calog, gSshNatives, (int64_t)(sizeof(gSshNatives) / sizeof(gSshNatives[0])), gSshLib); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogSshShutdown, calogDestroyAfterContextsE); } diff --git a/libs/calogTask.c b/libs/calogTask.c index f1a9966a..69d9b052 100644 --- a/libs/calogTask.c +++ b/libs/calogTask.c @@ -97,9 +97,25 @@ static int32_t taskLoad(CalogValueT *args, int32_t argCount, CalogVa static int32_t taskSelf(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t taskSpawn(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int64_t taskWrapContext(TaskLibT *lib, CalogContextT *context, CalogValueT *result, const char *label); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gTaskNatives[] = { + { "taskSpawn", taskSpawn }, + { "taskLoad", taskLoad }, + { "taskEval", taskEval }, + { "taskClose", taskClose }, + { "taskActive", taskActive }, + { "taskExit", taskExit }, + { "taskSelf", taskSelf }, + { "taskCount", taskCount }, +}; + + int32_t calogTaskRegister(CalogT *calog) { + int32_t status; + pthread_mutex_lock(&gTaskLibMutex); if (gTaskLib == NULL) { TaskLibT *lib; @@ -118,14 +134,10 @@ int32_t calogTaskRegister(CalogT *calog) { } gTaskLib->refCount++; pthread_mutex_unlock(&gTaskLibMutex); - calogRegisterInline(calog, "taskSpawn", taskSpawn, gTaskLib); - calogRegisterInline(calog, "taskLoad", taskLoad, gTaskLib); - calogRegisterInline(calog, "taskEval", taskEval, gTaskLib); - calogRegisterInline(calog, "taskClose", taskClose, gTaskLib); - calogRegisterInline(calog, "taskActive", taskActive, gTaskLib); - calogRegisterInline(calog, "taskExit", taskExit, gTaskLib); - calogRegisterInline(calog, "taskSelf", taskSelf, gTaskLib); - calogRegisterInline(calog, "taskCount", taskCount, gTaskLib); + status = calogRegisterBatch(calog, gTaskNatives, (int64_t)(sizeof(gTaskNatives) / sizeof(gTaskNatives[0])), gTaskLib); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogTaskShutdown, calogDestroyAfterContextsE); } diff --git a/libs/calogTime.c b/libs/calogTime.c index c850f25b..3eea480c 100644 --- a/libs/calogTime.c +++ b/libs/calogTime.c @@ -7,6 +7,8 @@ #include "calogTime.h" +#include "calogInternal.h" + #include #include @@ -20,11 +22,19 @@ static int32_t timeSleepNative(CalogValueT *args, int32_t argCount, CalogValueT static double timeToSeconds(const struct timespec *ts); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gTimeNatives[] = { + { "timeNow", timeNowNative }, + { "timeMonotonic", timeMonotonicNative }, + { "timeSleep", timeSleepNative }, +}; + + + + int32_t calogTimeRegister(CalogT *calog) { - calogRegisterInline(calog, "timeNow", timeNowNative, NULL); - calogRegisterInline(calog, "timeMonotonic", timeMonotonicNative, NULL); - calogRegisterInline(calog, "timeSleep", timeSleepNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gTimeNatives, (int64_t)(sizeof(gTimeNatives) / sizeof(gTimeNatives[0])), NULL); } diff --git a/libs/calogTimer.c b/libs/calogTimer.c index 370d2b5e..54565d92 100644 --- a/libs/calogTimer.c +++ b/libs/calogTimer.c @@ -76,13 +76,25 @@ static int64_t timerNowNs(void); static void timerPruneLocked(void); static int32_t timerSchedule(CalogValueT *args, int32_t argCount, CalogValueT *result, bool periodic); static void *timerThreadMain(void *arg); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gTimerNatives[] = { + { "timerAfter", timerAfterNative }, + { "timerEvery", timerEveryNative }, + { "timerCancel", timerCancelNative }, +}; + + int32_t calogTimerRegister(CalogT *calog) { + int32_t status; + calogRegistryRetain(&gInitMutex, &gRefCount); - calogRegisterInline(calog, "timerAfter", timerAfterNative, NULL); - calogRegisterInline(calog, "timerEvery", timerEveryNative, NULL); - calogRegisterInline(calog, "timerCancel", timerCancelNative, NULL); + status = calogRegisterBatch(calog, gTimerNatives, (int64_t)(sizeof(gTimerNatives) / sizeof(gTimerNatives[0])), NULL); + if (status != calogOkE) { + return status; + } return calogAtDestroy(calog, calogTimerShutdown, calogDestroyBeforeContextsE); } @@ -385,6 +397,13 @@ static void *timerThreadMain(void *arg) { pthread_mutex_unlock(&gTimerMutex); status = calogFnInvoke(cb, NULL, 0, &res); + // A callback that FAILED is reported. Nothing else observes this thread, so dropping it + // here meant a broken timer callback produced no stderr, no error handler call and no + // failing exit code -- it simply stopped working. Read the owner before the release + // below, which may free the callable. + if (status != calogOkE && status != calogErrDeadE) { + calogPostError(calogFnRuntime(cb), calogFnOwner(cb), &res); + } calogValueFree(&res); if (status == calogErrDeadE) { // The owning context is gone: auto-cancel the timer (find it again by id, since diff --git a/libs/calogXml.c b/libs/calogXml.c index 41ded87b..f7a50cd0 100644 --- a/libs/calogXml.c +++ b/libs/calogXml.c @@ -65,15 +65,21 @@ static int32_t xmlSetStr(CalogAggT *map, const char *key, const char *bytes, in static void xmlStartElement(void *userData, const xmlChar *name, const xmlChar **atts); static void xmlStop(XmlParseT *state, int32_t status); static int32_t xmlStringifyNative(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); +// Every inline native this library exposes. One table so registration is a single +// checked call (calogRegisterBatch) rather than a run of calls whose status was dropped. +static const CalogNativeEntryT gXmlNatives[] = { + { "xmlParse", xmlParseNative }, + { "xmlStringify", xmlStringifyNative }, +}; + + int32_t calogXmlRegister(CalogT *calog) { // libxml2 self-initializes, thread-safely (mutex-guarded), when a parser context is created, so // there is no explicit init to do and nothing process-global to tear down: its per-thread state // is released when each context thread exits. - calogRegisterInline(calog, "xmlParse", xmlParseNative, NULL); - calogRegisterInline(calog, "xmlStringify", xmlStringifyNative, NULL); - return calogOkE; + return calogRegisterBatch(calog, gXmlNatives, (int64_t)(sizeof(gXmlNatives) / sizeof(gXmlNatives[0])), NULL); } @@ -433,6 +439,14 @@ static int32_t xmlParseNative(CalogValueT *args, int32_t argCount, CalogValueT * // XXE-safe. NOCDATA delivers CDATA content through the characters callback. xmlCtxtUseOptions(ctxt, XML_PARSE_NONET | XML_PARSE_NOCDATA | XML_PARSE_NOENT | XML_PARSE_NO_XXE); xmlParseChunk(ctxt, args[0].as.s.bytes, (int)args[0].as.s.length, 1); + // An internal entity declaration makes libxml2 build a hidden "SAX compatibility mode" document + // to hold the entity table. libxml2 frees that itself only when the parse finishes cleanly; a + // fatal error halts the parser first, and xmlFreeParserCtxt never owns it -- so every failed + // parse of a document with an internal entity leaked the whole entity table. + if (ctxt->myDoc != NULL) { + xmlFreeDoc(ctxt->myDoc); + ctxt->myDoc = NULL; + } xmlFreeParserCtxt(ctxt); if (state.status == calogOkE && !state.haveRoot) { diff --git a/src/broker.c b/src/broker.c index a0b92470..b8390202 100644 --- a/src/broker.c +++ b/src/broker.c @@ -55,6 +55,20 @@ int32_t calogCall(CalogT *broker, const char *name, CalogValueT *args, int32_t a } +int32_t calogRegisterBatch(CalogT *broker, const CalogNativeEntryT *entries, int64_t count, void *userData) { + int64_t index; + int32_t status; + + for (index = 0; index < count; index++) { + status = calogRegisterInline(broker, entries[index].name, entries[index].fn, userData); + if (status != calogOkE) { + return status; + } + } + return calogOkE; +} + + CalogT *calogBrokerCreate(void) { CalogT *broker; diff --git a/src/calog.h b/src/calog.h index 1909bfa6..b6833490 100644 --- a/src/calog.h +++ b/src/calog.h @@ -169,12 +169,6 @@ int32_t calogAtDestroy(CalogT *calog, CalogDestroyHookFnT fn, CalogDestroyPhaseE // Register per-context hooks; either may be NULL. userData is passed through to both. Setup-only. int32_t calogAtContext(CalogT *calog, CalogContextHookFnT init, CalogContextHookFnT shutdown, void *userData); -// Format the CALLING thread's current cross-context call chain (newest first, e.g. -// "lua ctx 2 <- janet ctx 1") into buffer, returning the bytes written. Call it from within a -// native to see how a nested cross-engine call reached it. (Post-mortem, a failed cross-context -// call's error message already carries the same chain as "[engine ctx N] ..." tags -- see above.) -int32_t calogLastTrace(char *buffer, size_t size); - // ---- natives ---- // A name beginning with "__" is INTERNAL: it is callable via calogCall but is NOT exposed // into any engine's global namespace, so scripts can neither see nor shadow it. diff --git a/src/calogInternal.h b/src/calogInternal.h index c4b28345..aefd3c53 100644 --- a/src/calogInternal.h +++ b/src/calogInternal.h @@ -154,6 +154,18 @@ CalogEntryT *calogLookup(CalogT *calog, const char *name); // Safe only while the registry is frozen -- i.e. before any context starts. void calogForEach(CalogT *calog, void (*visit)(const CalogEntryT *entry, void *ud), void *ud); +// One inline native to register: the script-visible name and the C function behind it. +typedef struct CalogNativeEntryT { + const char *name; + CalogNativeFnT fn; +} CalogNativeEntryT; + +// Register every entry as an inline native, stopping at the FIRST failure and returning its status. +// A library's Register must not report success on a partial registration: the runtime then boots +// with a native missing and the script gets "no such function" at run time, far from the cause. One +// userData for the whole batch, which is what every library actually wants. +int32_t calogRegisterBatch(CalogT *calog, const CalogNativeEntryT *entries, int64_t count, void *userData); + // ---- abort bookkeeping (calogAbortAll is the public half; see calog.h) ---- // Mark the calling context: the abort error is being raised into its VM right now. Call it wherever // the abort is injected into running script code. @@ -163,6 +175,12 @@ void calogAbortRaise(void); // error, say) still gets its diagnostic while some other script is tearing the runtime down. bool calogAbortRaised(CalogT *calog); +// Report a script failure to the runtime's error handler, delivered on the host thread. A library +// that invokes a script callback from its OWN thread (the timer thread, a publish) uses this: that +// thread has no caller to return a status to, so without it a callback that failed vanishes -- no +// stderr, no handler, and under bin/calog no failing exit code either. +void calogPostError(CalogT *calog, uint64_t contextId, const CalogValueT *result); + // ---- callable lifecycle (driven by the engine adapters) ---- int32_t calogFnCreate(CalogFnT **out, CalogT *runtime, CalogNativeFnT fn, void *userData, CalogReleaseFnT release, uint64_t ownerCtxId); void calogFnFinalize(CalogFnT *fn); @@ -181,6 +199,16 @@ void calogFnMarkDead(CalogFnT *fn); // Take a reference only if the callable has not already dropped to zero (a finalize in flight). // Call with the owner context's queueMutex held; see the definition for why that is required. bool calogFnRetainIfLive(CalogFnT *fn); +// Shell lifetime, independent of the reference count: see CalogFnT in value.c. Only the owner's +// reclaim sweep takes a second holder, and only under the owner's queueMutex. +void calogFnShellHold(CalogFnT *fn); +void calogFnShellDrop(CalogFnT *fn); +// Engine release for a callable whose refcount already hit zero on another thread, run on the +// owner's thread by its reclaim sweep. Drops the sweep's shell holder. +void calogFnReclaimOrphan(CalogFnT *fn); +// Whether the owner's teardown adopted this callable rather than letting its finalize complete. +bool calogFnAdopted(const CalogFnT *fn); +void calogFnMarkAdopted(CalogFnT *fn); uint64_t calogFnOwner(const CalogFnT *fn); CalogT *calogFnRuntime(const CalogFnT *fn); void *calogFnUserData(const CalogFnT *fn); @@ -198,7 +226,10 @@ bool calogContextRegistered(CalogT *runtime, uint64_t ctxId); // that can no longer be resolved. Track failing (OOM) fails the create: an untracked callable would // be exactly the leak this exists to prevent. int32_t calogContextTrackFn(CalogT *runtime, uint64_t ownerCtxId, CalogFnT *fn); -void calogContextUntrackFn(CalogT *runtime, uint64_t ownerCtxId, CalogFnT *fn); +// Remove a callable from its owner's list. Returns false when the owner's teardown ADOPTED it +// instead -- the callable stays tracked and the owner's reclaim sweep takes over both the engine +// release and the shell -- in which case the caller must stop and touch it no further. +bool calogContextUntrackFn(CalogT *runtime, uint64_t ownerCtxId, CalogFnT *fn); // ---- per-context resource limits (sandboxing) ---- // The mutable state a limited context enforces on its OWN thread. memUsed is charged by the engine's diff --git a/src/calogMain.c b/src/calogMain.c index 621c93f7..d5caed46 100644 --- a/src/calogMain.c +++ b/src/calogMain.c @@ -83,6 +83,7 @@ static BOOL WINAPI consoleHandler(DWORD ctrlType); #endif static const CalogEngineT *engineForExtension(const char *ext); static const char *extensionOf(const char *arg); +static int32_t nativeCalogArgs(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t nativeCalogExit(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static int32_t nativeCalogPrint(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData); static void noteExitCode(int32_t code); @@ -94,12 +95,10 @@ static char *readFile(const char *path); static void requestShutdown(int32_t code); static bool resolveArg(const char *arg, const CalogEngineT **outEngine, char **outSource); -// Requested by a script (calogExit), a signal, or the last live context erroring out; the host -// pump loop watches gShutdown and exits with gExitCode. gExitRequested latches the first request of -// any kind (see noteExitCode) so nothing that follows can rewrite the code it named. -static _Atomic bool gShutdown = false; -static _Atomic bool gExitRequested = false; -static _Atomic int32_t gExitCode = 0; +// Requested by a script (calogExit), a signal, or a context erroring out; the host pump loop watches +// gShutdown and exits with gExitCode. See noteExitCode for how the code is arbitrated. +static _Atomic bool gShutdown = false; +static _Atomic int32_t gExitCode = 0; // Set by the signal handler, consumed by main: a signal means STOP, so the runtime is latched // aborting exactly as calogExit latches it. The handler cannot do that itself -- calogAbortAll is // not async-signal-safe and the handler has no runtime pointer -- so it sets this and main acts on @@ -108,6 +107,13 @@ static _Atomic int32_t gExitCode = 0; // good and ready. static _Atomic bool gAbortRequested = false; +// The arguments after "--" on the command line: the sanctioned way to parameterize a run. Written +// once before any context starts and never mutated, so calogArgs reads them without a lock. Scripts +// have no other channel -- the engines' own getenv/argv are removed (see API.md) -- and a documented +// one beats every project inventing its own convention out of files or the kv store. +static char **gScriptArgs = NULL; +static int32_t gScriptArgCount = 0; + // One entry per script context the runner launched. onError flags `failed` for a context whose // script errors; the pump loop then closes that context and drops the live count. When the live // count reaches zero -- every launched context has errored out -- the runner exits. @@ -234,6 +240,42 @@ static const char *extensionOf(const char *arg) { } +// calogArgs() -> the list of strings given after "--" on the command line, the same list for every +// script in the run, empty when none were given. Registered inline: it reads immutable startup data, +// so there is nothing to serialize on the host thread. +static int32_t nativeCalogArgs(CalogValueT *args, int32_t argCount, CalogValueT *result, void *userData) { + CalogAggT *agg; + int32_t index; + int32_t status; + + (void)args; + (void)argCount; + (void)userData; + calogValueNil(result); + status = calogAggCreate(&agg, calogListE); + if (status != calogOkE) { + return calogFail(result, status, "calogArgs: out of memory"); + } + for (index = 0; index < gScriptArgCount; index++) { + CalogValueT item; + + status = calogValueString(&item, gScriptArgs[index], (int64_t)strlen(gScriptArgs[index])); + if (status != calogOkE) { + calogAggFree(agg); + return calogFail(result, status, "calogArgs: out of memory"); + } + status = calogAggPush(agg, &item); + if (status != calogOkE) { + calogValueFree(&item); + calogAggFree(agg); + return calogFail(result, status, "calogArgs: out of memory"); + } + } + calogValueAgg(result, agg); + return calogOkE; +} + + // calogExit([code]) -- a script asks the runner to tear everything down and exit. Registered inline // so it runs on the CALLING script's thread: that is what lets calogAbortAll unwind this very script // (an error raised on the host thread could not), and everything it touches is thread-safe anyway. @@ -284,29 +326,52 @@ static int32_t nativeCalogPrint(CalogValueT *args, int32_t argCount, CalogValueT } -// Record what the process will report. Whoever asks FIRST names it -- a script's calogExit, a signal, -// or a script erroring out -- and everything after is ignored. That is the only rule under which a -// failure cannot be masked: neither by a sibling's later calogExit(0), nor by a context erroring out -// while the runtime tears down after a deliberate exit. +// Record what the process will report. The rule is that a FAILURE can never be masked, and that +// ordering must not be what decides whether it is: +// +// - the first NON-ZERO code wins outright and cannot be displaced, so one failure's status is +// never rewritten by a second failure, by a sibling's calogExit(0), or by a signal arriving +// during the teardown that failure started; +// - zero stores nothing. It is already the default, so it stands exactly when nothing failed. +// +// The earlier rule was "whoever asks first names it", which sounds equivalent and is not: a task +// that died just before its parent's calogExit(0) had its failure recorded second and thrown away, +// so whether the run reported the failure depended on which thread got there first. An aborted +// script is not reported as an error at all (see calogAbortRaised), so what reaches here during a +// teardown is a genuine failure and deserves to be reported. static void noteExitCode(int32_t code) { - if (!atomic_exchange(&gExitRequested, true)) { - atomic_store(&gExitCode, code); + int32_t expected; + + if (code == 0) { + return; } + expected = 0; + atomic_compare_exchange_strong(&gExitCode, &expected, code); } static void onError(uint64_t contextId, const char *message, void *userData) { int32_t index; + bool launched; (void)userData; fprintf(stderr, "calog: script error: %s\n", (message != NULL) ? message : "(unknown)"); // Flag the failing context so the pump loop retires it (a script error closes its context). + launched = false; for (index = 0; index < gLaunchedCount; index++) { if (gLaunched[index].id == contextId) { atomic_store(&gLaunched[index].failed, true); + launched = true; break; } } + // A context the RUNNER did not launch -- a task the script spawned. The pump loop only reaps + // what it launched, so nothing else would ever record this failure and the run would report + // success with an error printed above it. Its context belongs to whoever spawned it, so it is + // not closed here; only the exit code is claimed, and first-writer-wins keeps it honest. + if (!launched) { + noteExitCode(1); + } } @@ -336,9 +401,10 @@ static BOOL WINAPI consoleHandler(DWORD ctrlType) { static void printUsage(FILE *stream, const char *program) { size_t index; - fprintf(stream, "usage: %s