Andrew Kesterson
5ff87908e7
Four things in src/error.c did by hand what the macros already do, or
skipped checks the library would have caught for a consumer.
akerr_copy_string() returned void and validated only its capacity, while
writing through a caller-supplied pointer for a caller-supplied length.
It is now __akerr_copy_string() and raises: AKERR_NULLPOINTER for a NULL
destination or source, AKERR_VALUE for a capacity with no room for a
terminator. Both call sites PASS it, and the owner copy in
akerr_reserve_status_range() now gates the commit, so a failed copy
cannot leave a range claimed under an empty owner. It is exported under
the internal prefix rather than static so tests/err_copy_string.c can
drive those guards; nothing else can reach them.
__akerr_name_library_status() and the band reservation in akerr_init()
hand-rolled the log/handler/release sequence. Both now use
ATTEMPT/CATCH/PROCESS/FINISH_NORETURN. PASS does not fit: both sites are
void and have no caller to propagate to, so the terminal form of the same
idiom is the right one -- an unhandled failure prints its stack trace and
goes to akerr_handler_unhandled_error, which terminates, exactly as
before but without the bespoke plumbing. The legacy set path in
akerr_name_for_status() had the same shape and now handles its refusal
with HANDLE_DEFAULT, converting it to the "Unknown Error" sentinel.
Every remaining `if (x) { FAIL_RETURN }` in the registry is now
FAIL_ZERO_RETURN or FAIL_NONZERO_RETURN, and akerr_register_status_name()
checks both owner and name before passing either down --
akerr_store_status_name() reads a NULL owner as "caller did not identify
itself" for the legacy path, so a NULL arriving through the owned entry
point would have skipped the ownership check entirely.
New tests: err_copy_string (the guards above), err_library_status_fatal
(WILL_FAIL -- proves a refused library-status registration terminates).
Tests: ctest 31/31, mutation 80.7% (was 77.5%), line coverage 98.9%.
Branch coverage on src/error.c drops 64.5% -> 50.4%, just over its gate:
each FAIL_* site carries ~6 branch outcomes of error-construction
machinery that only run when that failure fires, and each PASS around a
call that cannot fail carries ~25, so added validation lowers the ratio
by construction. Recorded in TODO.md item 7.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
94 lines
4.6 KiB
Markdown
94 lines
4.6 KiB
Markdown
# TODO
|
|
|
|
Working notes for `libakerror`. Outstanding items only.
|
|
|
|
## 1. The test suite has no sanitizer run
|
|
|
|
Mutation testing caught an out-of-bounds probe in the status-name hash table
|
|
that the suite could not: the failure mode was a write into adjacent BSS, which
|
|
does not crash, so every test still passed. Sharpening one test closed that
|
|
instance, but ASan would have caught the whole class directly and independently
|
|
of how sharp the assertions are.
|
|
|
|
Add a `-fsanitize=address,undefined` build to `.gitea/workflows/ci.yaml`, or a
|
|
CMake option alongside `AKERR_COVERAGE`. This is the highest-value item here: it
|
|
covers the whole library, not just the registry, and the library's fixed pools
|
|
and manual buffer arithmetic are exactly what it is good at.
|
|
|
|
## 2. `HANDLE`-level status aliasing is still undetectable
|
|
|
|
Two components can compile the same integer into a `case` label without ever
|
|
reserving a range or registering a name, and nothing sees it. Ownership
|
|
enforcement covers *naming*, which is the part the library mediates; the `case`
|
|
label never reaches it.
|
|
|
|
Closing this needs the `if`/`else if` handler ladder — rewriting
|
|
`PROCESS`/`HANDLE`/`HANDLE_GROUP`/`HANDLE_DEFAULT`/`FINISH` so status matching
|
|
is not restricted to integer constant expressions. That would also allow
|
|
matching on ranges or predicates, and would let a handler resolve a code through
|
|
its owner. It touches the most load-bearing code in the library and every
|
|
consumer's error handling at once, so it wants its own change.
|
|
|
|
Note it would *not* by itself fix the "don't use `CATCH` or `FAIL_*_BREAK`
|
|
inside a loop" hazard: that comes from exiting via `break`, not from `switch`.
|
|
|
|
## 3. No registry introspection
|
|
|
|
There is no way to ask who owns a status, or to enumerate reservations. The
|
|
"coordinate ranges at the dependency-stack level" advice in UPGRADING.md
|
|
therefore has no tooling behind it.
|
|
|
|
A read-only accessor plus a dump through `akerr_log_method` would let a startup
|
|
self-check or a CI job print the whole map for a linked stack. Cheap, additive,
|
|
and the natural next step for multi-component adoption.
|
|
|
|
## 4. No way to release a reservation
|
|
|
|
A plugin host that `dlopen`s many distinct plugins over a process lifetime
|
|
accumulates ranges until the table fills. Reloading the *same* plugin is fine —
|
|
an identical repeat by the same owner is idempotent.
|
|
|
|
## 5. The registry is not thread safe
|
|
|
|
Global mutable state, no locking, and `akerr_status_name_count++` is not atomic.
|
|
Currently documented as an initialization-time-only API rather than enforced. If
|
|
components start initializing on separate threads this needs either a lock or a
|
|
documented once-per-process init barrier.
|
|
|
|
## 6. Deprecate the two-argument name-registration path
|
|
|
|
`akerr_name_for_status(status, name)` cannot identify its caller, so it can only
|
|
check that *some* reservation covers the status, not that the caller owns it. It
|
|
exists for migration. Once consumers have moved to
|
|
`akerr_register_status_name()`, make the set path a no-op or remove it and leave
|
|
`akerr_name_for_status()` as pure lookup.
|
|
|
|
## 7. `akerr_init()`'s own reservation failure is untested
|
|
|
|
`tests/err_library_status_fatal.c` covers the terminal path in
|
|
`__akerr_name_library_status()` by naming a status the library does not own. The
|
|
band reservation in `akerr_init()` has no such handle: it can only fail in a
|
|
build whose tables are too small for the library's own entries, and both sizes
|
|
are `PRIVATE` to the library target, so a test executable cannot set them.
|
|
|
|
Closing it means a second library target built with tiny tables plus a
|
|
`WILL_FAIL` test linked against it — worth doing when the CMake gains a
|
|
sanitizer variant (item 1), since that adds the same machinery.
|
|
|
|
Related: branch coverage on `src/error.c` now sits just above its 50% gate.
|
|
Every `FAIL_*` site carries about six branch outcomes of error-construction
|
|
machinery (`ENSURE_ERROR_READY`, `AKERR_STACKTRACE_APPEND`) that only run when
|
|
that specific failure fires, and every `PASS` site around a call that cannot
|
|
fail carries about twenty-five. Validating more inputs therefore lowers the
|
|
ratio by construction. Before adding defensive checks, expect to add a test that
|
|
drives them, as `tests/err_copy_string.c` does.
|
|
|
|
## Unrelated pre-existing issues
|
|
|
|
- The `AKERR_USE_STDLIB=OFF` build does not compile at all: `bool`, `PATH_MAX`
|
|
and `NULL` are used unconditionally but only included under the stdlib branch.
|
|
The README's dependency list states what a replacement must provide, but the
|
|
header still needs its includes untangled for that configuration to work.
|
|
- `CMakeLists.txt` sets `main_lib_dest` from `MY_LIBRARY_VERSION`, which is never
|
|
defined and never read. Dead line.
|