Every wrapper outside the list and tree code was untested. Six new test
files close that, following the plan already written in TODO.md 1.2-1.6:
test_stream.c fopen/fread/fwrite/fclose -- happy paths, the round
trip, AKERR_EOF on a short read, AKERR_IO on a stream
opened in the wrong mode, ENOENT, and the NULL guards
test_format.c printf/fprintf/sprintf -- text *and* count asserted
(stdout is pointed at a temp file to check aksl_printf),
all eight NULL guards, EBADF on a read-only stream, and
512 variadic calls in a loop as sanitizer cover for the
missing va_end
test_convert.c ato{i,l,ll,f} happy paths, negatives, leading
whitespace, NULL guards
test_path.c realpath on a file and on a symlink, both compared
against realpath(3) since TMPDIR may itself be a link;
ENOENT, ENOTDIR, NULL path
test_strhash.c djb2 known-answer vectors, len == 0, embedded NUL,
stability, NULL guards
test_convert_strict.c
known-failing (2.1.5): the AKERR_VALUE / ERANGE
contract the ato* family cannot express today
test_tree.c gains the BFS AKERR_NOT_IMPLEMENTED contract, NULL arguments,
and a callback error that is not AKERR_ITERATOR_BREAK propagating out.
Tests deliberately say nothing about behaviour TODO.md records as
defective -- unchecked ptr/mode/resolved_path, short transfers reported as
success, *count left at -1, the djb2 sign extension -- so the eventual fix
does not have to come with a test rewrite. Each failure case in
test_path.c passes a zeroed buffer, because the wrapper's own error path
formats resolved_path with %s (2.1.6).
aksl_capture.h gains aksl_temp_file() with an atexit unlink backstop.
Without it every test that fails before its own unlink leaves temp files
behind -- which is the normal case for a known-failing test, and happens
173 times over in a mutation run.
Coverage on src/stdlib.c: 52.0% -> 99.0% of lines (200/202), 23.6% ->
51.0% of branches, 8/21 -> 21/21 functions. The two uncovered lines are
both `} HANDLE(e, AKERR_ITERATOR_BREAK) {`, where the macro starts with
the `break;` of PROCESS's `case 0:` arm -- reachable only via a non-NULL
error context whose status is zero, the pathology 2.2.1 exists to remove.
Mutation score on src/stdlib.c: 46.8% -> 89.6% (155/173 killed). CI, the
pre-push hook and the docs ratchet from 40 to 80 accordingly, and the 18
survivors are grouped by cause in TODO.md and README.md. A new CI
coverage job gates at 90% lines / 45% branches.
Verified:
ctest --test-dir build # 12/12
ctest --test-dir build-asan # 12/12 under ASan + UBSan
ctest --test-dir build-coverage # 14/14, report attached
ctest --test-dir build -j8 --repeat until-fail:3
gcc -Wall -Wextra -c on all nine test files # no warnings
python3 scripts/mutation_test.py --target src/stdlib.c # 89.6%
No temp files left in /tmp after any of the above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
576 lines
34 KiB
Markdown
576 lines
34 KiB
Markdown
# TODO
|
||
|
||
Working notes for `libakstdlib` — the akerror-wrapped libc surface.
|
||
|
||
Scope of the current library (`include/akstdlib.h`, `src/stdlib.c`): 16 libc wrappers
|
||
(`fopen`, `fread`, `fwrite`, `fclose`, `malloc`, `free`, `memset`, `memcpy`, `printf`,
|
||
`fprintf`, `sprintf`, `atoi`, `atol`, `atoll`, `atof`, `realpath`), one hash helper
|
||
(`aksl_strhash_djb2`), a doubly-linked list (`append`/`pop`/`iterate`) and a binary tree
|
||
iterator (`aksl_tree_iterate`).
|
||
|
||
Items marked **[CONFIRMED]** were reproduced by building the library and running a probe
|
||
program against it, not just read off the source.
|
||
|
||
---
|
||
|
||
## 1. Unit tests that should be written
|
||
|
||
### 1.0 Test-harness prerequisites — **DONE** (branch `feature/test-harness`)
|
||
|
||
`ctest` is green: 5/5, no *Not Run* entries. Everything in §1.1–1.9 can now be written
|
||
against `tests/aksl_capture.h` and added to the `AKSL_TESTS` list.
|
||
|
||
- [x] **The current tests cannot fail.** `tests/test_linkedlist.c` asserted nothing at all;
|
||
it printed node names and returned `0` unconditionally, so any of the list bugs in
|
||
§2.1 would pass it. Rewritten as 15 assertion-based cases covering the append, iterate
|
||
and pop behaviour that is correct today.
|
||
- [x] **`tests/test_tree.c` was red.** `ctest` reported `tree (Failed)`, exit 136, because
|
||
`parms.steps` was never reset between the three searches, so the second assertion
|
||
compared an accumulated `14` against `7`. Each search now builds its own tree and
|
||
params. The file records that its step counts do not actually distinguish the three
|
||
traversal orders — real order assertions remain §1.8.
|
||
- [x] **Adopt libakerror's test conventions.** `tests/aksl_capture.h` provides `AKSL_CHECK()`
|
||
(an `NDEBUG`-proof assert), `AKSL_CHECK_STATUS()`/`AKSL_CHECK_OK()` which run an
|
||
akerror-returning call, assert on its status and release the context,
|
||
`AKSL_CHECK_MSG_CONTAINS()`, a capturing `akerr_log_method`, `aksl_slots_in_use()` for
|
||
pool-leak checks, and an `AKSL_RUN()` driver that additionally fails any test which
|
||
leaks an error-pool slot.
|
||
- [x] **One test source per behaviour, registered with CTest via a list variable.**
|
||
`tests/test_<name>.c` → executable `test_<name>` → CTest test `<name>`, driven by
|
||
`AKSL_TESTS` in `CMakeLists.txt`.
|
||
- [x] **Add a `WILL_FAIL` category.** Two lists: `AKSL_WILL_FAIL_TESTS` for tests that abort
|
||
by design (empty for now) and `AKSL_KNOWN_FAILING_TESTS` for the confirmed defects in
|
||
§2.1 — see the note at the head of §2.1.
|
||
- [x] **Fix the four permanently-failing submodule tests.** `deps/libakerror` is added
|
||
`EXCLUDE_FROM_ALL`, so akerror's `add_test` entries registered but its test binaries
|
||
were never built, and `ctest` reported `err_catch`, `err_cleanup`, `err_trace`,
|
||
`err_improper_closure` as *Not Run* → failed on every run. Neither the pinned commit
|
||
nor upstream `main` guards that registration, CMake has no way to un-register a test,
|
||
and `set_tests_properties` cannot reach across directory scopes — so `add_test` and
|
||
`set_tests_properties` are shadowed for the duration of the `add_subdirectory` call.
|
||
The dependency has its own CI; this suite now contains only this project's tests.
|
||
- [x] **Wire in a memory-error build.** `cmake -S . -B build-asan -DAKSL_SANITIZE=ON` adds
|
||
`-fsanitize=address,undefined -fno-sanitize-recover=all` to the library, the tests and
|
||
the vendored libakerror. Currently clean, and it is the intended way to test the items
|
||
in §2 that only misbehave under instrumentation (uninitialised `%s` in
|
||
`aksl_realpath`, unbounded `vsprintf`, missing `va_end`).
|
||
- [x] **Port `scripts/mutation_test.py` from libakerror.** Retargeted at `src/stdlib.c` and
|
||
`include/akstdlib.h` (178 mutants) and exposed as
|
||
`cmake --build build --target mutation`. CI runs the narrower `src/stdlib.c` set (173
|
||
mutants) as its own `mutation_test` job with `--threshold 80` and a JUnit report.
|
||
**Current score: 89.6%** — 155 killed, 18 survived. It was 46.8% (81 killed, 92
|
||
survived) before §1.2–1.6 were written; the remaining survivors are:
|
||
|
||
| surviving mutants | where | why |
|
||
|---|---|---|
|
||
| 5 | deleted statements whose absence nothing observes (`free(ptr)`, `obj->next = NULL`, three `SUCCEED_RETURN`s) | needs an assertion on the side effect, not on the status |
|
||
| 4 | `aksl_list_append`'s cycle/tail walk | the function is broken here (§2.1.1); its known-failing test cannot pin the internals yet |
|
||
| 4 | `lalloc`/`lfree` defaulting in `aksl_tree_iterate` | dead parameters (§2.2.8) — defaulted, then never called |
|
||
| 3 | `*count == -1` in the `printf` family | `*count` on the error path is itself a contract gap (§1.3) |
|
||
| 2 | `feof` in `aksl_fwrite`, `break` after the BFS `FAIL_RETURN` | unreachable in practice |
|
||
|
||
The threshold is a regression ratchet, not a quality bar; raise it as those survivors
|
||
become assertions. Each one is a concrete missing test — `mutation-junit.xml` lists
|
||
them with `file:line` and the exact edit.
|
||
- [x] **Cap every test with a CTest `TIMEOUT`.** Found while measuring the above: a mutant
|
||
that deletes `fast = fast->next->next` turns `aksl_list_iterate` into an infinite
|
||
loop, so `ctest` waited forever, the mutation harness killed `ctest` at its own
|
||
120 s timeout, and the orphaned test binary kept spinning at 100% CPU while holding
|
||
the pipe open — the run wedged and had to be killed by hand. `TIMEOUT 30` on every
|
||
test makes `ctest` reap its own child, so those mutants are now killed in 30 s and
|
||
leave nothing behind. It also guards the real suite against the same class of bug.
|
||
- [x] **CI could not have run any of this.** `actions/checkout@v4` was not fetching
|
||
submodules, so `add_subdirectory(deps/libakerror)` had nothing to descend into and the
|
||
configure step failed before ever reaching the tests. Added `submodules: recursive`,
|
||
and swapped `cmake --build build --target test` for `ctest --output-on-failure` so a
|
||
failure is diagnosable from the job log. The deeper problem — CI installing
|
||
`libakerror@main` while the build actually compiles the pinned submodule — is §2.3.
|
||
|
||
### 1.1 Memory wrappers
|
||
|
||
- [x] `aksl_malloc` — happy path returns non-NULL and writes through `dst`.
|
||
- [x] `aksl_malloc(n, NULL)` → `AKERR_NULLPOINTER`, message content asserted.
|
||
- [ ] `aksl_malloc(0, &p)` — pin down the contract. Today the result depends on whether the
|
||
platform's `malloc(0)` returns NULL; if it does, the wrapper reports `errno`, which may
|
||
be `0`. See §2.2.1.
|
||
- [ ] `aksl_malloc(SIZE_MAX, &p)` → allocation failure surfaces `ENOMEM`, and `*dst` is left
|
||
NULL rather than garbage.
|
||
- [x] `aksl_free(NULL)` → `AKERR_NULLPOINTER` (assert the documented behaviour, since
|
||
`free(NULL)` is legal C and callers will be surprised).
|
||
- [x] `aksl_free` happy path, and a malloc→free round trip under ASan.
|
||
- [x] `aksl_memset` — happy path fills the buffer; `n == 0` is a no-op; `s == NULL` →
|
||
`AKERR_NULLPOINTER`.
|
||
- [x] `aksl_memcpy` — happy path copies; `d == NULL` and `s == NULL` each →
|
||
`AKERR_NULLPOINTER`; `n == 0` with valid pointers succeeds.
|
||
- [ ] `aksl_memcpy` with overlapping regions — decide and test whether this is rejected
|
||
(`AKERR_VALUE`) or documented as UB like `memcpy`.
|
||
|
||
### 1.2 File I/O
|
||
|
||
Covered by `tests/test_stream.c`.
|
||
|
||
- [x] `aksl_fopen` happy path on a temp file; `*fp` is written.
|
||
- [x] `aksl_fopen("/nonexistent/path", "r", &fp)` → `ENOENT` propagated as the status, with
|
||
the pathname in the message.
|
||
- [ ] `aksl_fopen` on a mode-denied path (e.g. `/proc/1/mem`, or a `chmod 000` temp file) →
|
||
`EACCES`.
|
||
- [x] `aksl_fopen(path, mode, NULL)` → `AKERR_NULLPOINTER`.
|
||
- [ ] `aksl_fopen(NULL, "r", &fp)` and `aksl_fopen(path, NULL, &fp)` → currently unchecked,
|
||
see §2.2.2. Test once the guards exist.
|
||
- [x] `aksl_fread` full read; short read at EOF → `AKERR_EOF`; read from a write-only stream
|
||
→ `AKERR_IO`; `fp == NULL` → `AKERR_NULLPOINTER`. `ptr == NULL` is still unchecked and
|
||
untested (see §2.2.3).
|
||
- [ ] `aksl_fread` **partial read that is neither EOF nor error** — assert whatever the fixed
|
||
contract is; today this silently returns success and the caller cannot tell how many
|
||
members were read.
|
||
- [x] `aksl_fwrite` happy path; write to a read-only stream → `AKERR_IO`; `fp == NULL` →
|
||
`AKERR_NULLPOINTER`. The full-device (`/dev/full`) case is still open.
|
||
- [x] `aksl_fclose` happy path; `NULL` → `AKERR_NULLPOINTER`. Double-close is still
|
||
undecided, so it is neither caught nor tested.
|
||
- [ ] `aksl_fclose` on a stream whose buffered flush fails (`/dev/full`) → non-zero `fclose`
|
||
surfaces `errno`.
|
||
- [x] Round-trip test: `fopen` → `fwrite` → `fclose` → `fopen` → `fread` → compare bytes.
|
||
|
||
### 1.3 Formatted output
|
||
|
||
Covered by `tests/test_format.c`.
|
||
|
||
- [x] `aksl_printf` / `aksl_fprintf` / `aksl_sprintf` happy paths, asserting both the byte
|
||
count written through `count` and the produced text. (`aksl_printf` is checked by
|
||
pointing `stdout` at a temp file for the duration of the call.)
|
||
- [x] Each of the three with every pointer argument NULL in turn → `AKERR_NULLPOINTER`.
|
||
- [x] `aksl_fprintf` to a read-only stream → the `errno` it saw (`EBADF`) with "Short write"
|
||
in the message. `*count` is still left holding `-1`; the test asserts the status only,
|
||
so it will not have to change when that is fixed.
|
||
- [ ] `aksl_sprintf` with a format that overflows the destination — currently unbounded
|
||
(§2.2.4). Test the `aksl_snprintf` replacement once it exists.
|
||
- [x] Regression test for the missing `va_end` (§2.1.4) — 512 variadic calls in a loop, which
|
||
the sanitizer build also runs.
|
||
- [ ] Format-string/arg mismatch is caught at compile time once
|
||
`__attribute__((format(printf, ...)))` is added (§2.2.5) — a negative compile test.
|
||
|
||
### 1.4 String → number
|
||
|
||
Happy paths and NULL guards are covered by `tests/test_convert.c`; the strict-conversion
|
||
contract is asserted by `tests/test_convert_strict.c`, which is registered as a known
|
||
failure until §2.1.5 is fixed.
|
||
|
||
- [x] `aksl_atoi` / `atol` / `atoll` / `atof` happy paths, including negative values and
|
||
leading whitespace.
|
||
- [x] NULL `nptr` and NULL `dest` for each → `AKERR_NULLPOINTER`.
|
||
- [x] **Non-numeric input** (`"not a number"`) — **[CONFIRMED]** currently returns *success*
|
||
with `*dest == 0`. `AKERR_VALUE` is asserted by the known-failing test.
|
||
- [x] **Overflow** (`"99999999999999999999"`) — **[CONFIRMED]** currently returns success with
|
||
a garbage value (`-1` on this box). `ERANGE` is asserted by the known-failing test.
|
||
- [x] Empty string, `" "` and `"12abc"` (trailing junk) → `AKERR_VALUE`, in the known-failing
|
||
test. `"0x10"` and `"inf"`/`"nan"` for `atof` are still open.
|
||
- [ ] `LONG_MIN`/`LONG_MAX`/`LLONG_MIN`/`LLONG_MAX` boundary strings round-trip exactly.
|
||
|
||
### 1.5 `aksl_realpath`
|
||
|
||
Covered by `tests/test_path.c`. Every failure case there passes a zeroed `resolved_path`,
|
||
because the wrapper's own error path formats that buffer with `%s` (§2.1.6).
|
||
|
||
- [x] Happy path on an existing file and on a symlink chain; result matches `realpath(3)`.
|
||
- [x] Non-existent path → `ENOENT`.
|
||
- [x] A path component that is not a directory → `ENOTDIR`.
|
||
- [ ] Symlink loop → `ELOOP`.
|
||
- [x] `path == NULL` → `AKERR_NULLPOINTER`.
|
||
- [ ] `resolved_path == NULL` — currently unchecked and leaks (§2.1.6). Test once fixed.
|
||
- [ ] A failure case where `resolved_path` is an *uninitialised* buffer — this is the crash
|
||
case in §2.1.6; run it under ASan/MSan.
|
||
|
||
### 1.6 `aksl_strhash_djb2`
|
||
|
||
Covered by `tests/test_strhash.c`.
|
||
|
||
- [x] Known-answer vectors: `djb2("")` == 5381; a handful of fixed strings with their
|
||
pre-computed 32-bit values.
|
||
- [x] `len == 0` returns 5381 regardless of `str` contents.
|
||
- [x] NULL `str` and NULL `hashval` → `AKERR_NULLPOINTER`.
|
||
- [ ] **High-bit bytes** (`"\xff\xfe"`) — pins down the sign-extension bug in §2.2.6; the
|
||
expected value must be the `unsigned char` one (5868578; the wrapper returns the
|
||
sign-extended 5859874 today). Every vector in the test file is 7-bit ASCII precisely so
|
||
that it says nothing about this case either way.
|
||
- [x] Embedded NUL bytes are hashed (the function is length-driven, not NUL-driven).
|
||
- [x] Same input → same output across two calls (no hidden state).
|
||
|
||
### 1.7 Linked list
|
||
|
||
- [ ] **`aksl_list_append` builds a correct chain of N nodes** — **[CONFIRMED BROKEN]**, see
|
||
§2.1.1. Appending `n1..n4` to `n0` yields the chain `n0 -> n4`; `n1`, `n2`, `n3` are
|
||
silently dropped. This is the single most important test to add.
|
||
- [ ] `append` sets `obj->prev` to the real tail and `obj->next` to NULL.
|
||
- [ ] `append` onto an empty (single, zeroed) node.
|
||
- [ ] `append` NULL list / NULL obj → `AKERR_NULLPOINTER`.
|
||
- [ ] `append` onto a list containing a cycle → `AKERR_CIRCULAR_REFERENCE`, for a self-loop,
|
||
a 2-node cycle, and a cycle that does not include the head.
|
||
- [ ] `append` a node that is already in the list (aliasing) — define and test the contract.
|
||
- [ ] **`aksl_list_iterate` visits every node exactly once, starting at the head** —
|
||
**[CONFIRMED BROKEN]**, see §2.1.2: it starts iterating from the *midpoint* left behind
|
||
by the cycle detector, so the head is never visited. Assert both the visit count and
|
||
the visit order.
|
||
- [ ] `iterate` over a single-node list visits exactly one node.
|
||
- [ ] `iterate` NULL list / NULL iter → `AKERR_NULLPOINTER`.
|
||
- [ ] `iterate` over a cyclic list → `AKERR_CIRCULAR_REFERENCE` (self-loop, 2-node,
|
||
tail-to-middle).
|
||
- [ ] `iterate` where the callback raises `AKERR_ITERATOR_BREAK` → iteration stops at that
|
||
node, the wrapper returns success, and the visit count proves the early exit.
|
||
- [ ] `iterate` where the callback raises some *other* error → that error propagates out
|
||
unchanged, with the callback's message intact.
|
||
- [ ] `aksl_list_pop` on a middle node relinks `prev`/`next` correctly and clears the popped
|
||
node's pointers.
|
||
- [ ] `pop` on the head node, on the tail node, and on a single-node list.
|
||
- [ ] `pop(NULL)` → `AKERR_NULLPOINTER`.
|
||
- [ ] `pop` then `iterate` — the list is still traversable and the popped node is gone.
|
||
- [ ] Pool-accounting test: after a long sequence of list operations including failures,
|
||
`akerr_slots_in_use() == 0`.
|
||
|
||
### 1.8 Tree
|
||
|
||
- [ ] Visit **order** assertions, not just counts, for `DFS_PREORDER`, `DFS_INORDER` and
|
||
`DFS_POSTORDER` over the 7-node tree — record the visited node pointers into an array
|
||
and compare against the expected sequence. The current test only counts steps, which
|
||
cannot distinguish the three orders (all three visit 7 nodes).
|
||
- [ ] **`AKERR_ITERATOR_BREAK` aborts the whole traversal** — **[CONFIRMED BROKEN]**, see
|
||
§2.1.3. The pre-order case exists as `tests/test_tree_iterate_break.c`: pre-order
|
||
visits 0, 1, 3, so breaking at `tree[3]` must stop the walk at **3** visits, and today
|
||
it runs on to all 7. Add the equivalent for in-order (3, 1, 4, 0, 5, 2, 6 → breaking at
|
||
`tree[4]` gives 3) and post-order (3, 4, 1, 5, 6, 2, 0 → breaking at `tree[5]` gives 4).
|
||
- [x] `AKSL_TREE_SEARCH_BFS` and `AKSL_TREE_SEARCH_BFS_RIGHT` → `AKERR_NOT_IMPLEMENTED`
|
||
today, asserted in `tests/test_tree.c` along with the callback never being called;
|
||
replace with real order assertions once implemented (§3 / §2.2.9).
|
||
- [x] `aksl_tree_iterate` NULL `root` / NULL `iter` → `AKERR_NULLPOINTER`, and a callback error
|
||
that is *not* `AKERR_ITERATOR_BREAK` propagates out to the caller.
|
||
- [ ] **Unknown `searchmode`** (e.g. `99`) — **[CONFIRMED]** currently returns *success*
|
||
having visited nothing. Should be `AKERR_VALUE`.
|
||
- [ ] **`AKSL_TREE_SEARCH_VISIT`** (defined in the header, `5`) — **[CONFIRMED]** falls into
|
||
the same silent-success hole. Either implement it or reject it.
|
||
- [ ] `root == NULL` / `iter == NULL` → `AKERR_NULLPOINTER`.
|
||
- [ ] Single-node tree (no children) visits exactly once, in all three orders.
|
||
- [ ] Left-only and right-only degenerate chains.
|
||
- [ ] Callback raising a non-`ITERATOR_BREAK` error propagates out of the recursion with the
|
||
original status and message.
|
||
- [ ] Custom `lalloc`/`lfree` are actually invoked — **currently they are stored and never
|
||
called** (§2.2.8), so this test will fail until BFS lands.
|
||
- [ ] Deep/degenerate tree (e.g. 100k-node left chain) — documents the recursion-depth limit
|
||
(§2.2.7).
|
||
- [ ] A tree containing a cycle (child pointing back at an ancestor) — currently infinite
|
||
recursion; test for `AKERR_CIRCULAR_REFERENCE` once guarded.
|
||
|
||
### 1.9 Cross-cutting
|
||
|
||
- [ ] **Error-pool accounting**: for *every* wrapper, a test that drives the failure path
|
||
`AKERR_MAX_ARRAY_ERROR + 10` times and asserts the pool does not leak
|
||
(`akerr_slots_in_use() == 0` after each handled error).
|
||
- [ ] **Stack-trace content**: assert that the file/function/line recorded by each wrapper's
|
||
`FAIL` points at `src/stdlib.c` and the right function name.
|
||
- [ ] **`AKERR_NOIGNORE` is effective**: a negative compile test where a wrapper's return is
|
||
discarded and `-Werror=unused-result` fires.
|
||
- [ ] **Thread safety**: libakerror's `AKERR_ARRAY_ERROR` is a process-global array with no
|
||
locking. If `libakstdlib` is meant to be callable from threads, add a concurrent
|
||
smoke test under TSan — and if it is not, say so in the README.
|
||
|
||
---
|
||
|
||
## 2. Corner cases in existing functionality that should be resolved
|
||
|
||
### 2.1 Confirmed defects (reproduced against the built library)
|
||
|
||
Four of these now have a failing test apiece, registered in `AKSL_KNOWN_FAILING_TESTS`
|
||
and therefore marked `WILL_FAIL` so the suite stays green while the gap stays visible:
|
||
`tests/test_list_append_chain.c`, `tests/test_list_iterate_head.c`,
|
||
`tests/test_tree_iterate_break.c` and `tests/test_convert_strict.c`. When one of these
|
||
defects is fixed, CTest reports that test as failed with *unexpectedly passed* — that is the
|
||
cue to move it into `AKSL_TESTS`.
|
||
|
||
1. **`aksl_list_append` does not find the tail — it silently truncates the list.**
|
||
`src/stdlib.c:194`. The function conflates Floyd cycle detection with tail-finding:
|
||
`tail` is set to `slow` *before* `slow` advances, so it tracks the node *behind the
|
||
midpoint*, not the tail. Appending `n1`, `n2`, `n3`, `n4` to `n0` produces the chain
|
||
`n0 -> n4`; `n1`–`n3` are unlinked and lost. The fix is to keep the cycle check but walk
|
||
a separate cursor to the real tail (`while (tail->next) tail = tail->next;`), or run
|
||
Floyd first and then walk to the end.
|
||
|
||
2. **`aksl_list_iterate` skips the first half of the list.** `src/stdlib.c:324`. After the
|
||
cycle-detection loop, `slow` is left at the list midpoint, and the visiting loop then
|
||
starts from `slow` instead of from `list`. The head node is never passed to the callback.
|
||
Fix: iterate from `list`, not from `slow`.
|
||
|
||
3. **`AKERR_ITERATOR_BREAK` does not stop a tree traversal.** `src/stdlib.c:251`. The
|
||
recursive frame in which the callback raises the break handles it in its own
|
||
`PROCESS`/`HANDLE(e, AKERR_ITERATOR_BREAK)` block and returns *success*; the parent's
|
||
`PASS` therefore sees no error and continues on to the sibling subtree. Breaking at
|
||
`tree[3]` in pre-order still visits all 7 nodes. Fix by splitting the recursion: an
|
||
internal helper that propagates `AKERR_ITERATOR_BREAK` unhandled, and a public entry point
|
||
that swallows it exactly once at the top. `tests/test_tree.c` currently passes only
|
||
because the search target is the last node in all three orders.
|
||
|
||
4. **`va_end` is never called.** `src/stdlib.c:96`, `:109`, `:122` each call `va_start` with
|
||
no matching `va_end` on any path — happy or error. This is undefined behaviour per the C
|
||
standard and leaks register-save state on some ABIs.
|
||
|
||
5. **`aksl_atoi`/`atol`/`atoll`/`atof` cannot report a conversion failure.**
|
||
`src/stdlib.c:135`–`169`. `atoi("not a number")` returns success with `0`;
|
||
`atoi("99999999999999999999")` returns success with a wrapped value. Since the entire
|
||
point of this library is turning silent libc failures into error contexts, these should be
|
||
reimplemented over `strtol`/`strtoll`/`strtod` with `errno = 0` before the call, an
|
||
`endptr` check for "no digits consumed" and "trailing junk", and a range check —
|
||
raising `AKERR_VALUE` and `ERANGE` respectively. Keep the `atoi`-compatible names but
|
||
document the stricter contract, or add `aksl_strtol`-family wrappers alongside.
|
||
`tests/test_convert_strict.c` asserts that contract and is registered as a known failure.
|
||
|
||
6. **`aksl_realpath` mishandles `resolved_path`.** `src/stdlib.c:171`.
|
||
- `resolved_path` is never NULL-checked. `realpath(path, NULL)` is valid and mallocs a
|
||
buffer, but the wrapper discards `result`, so the caller gets nothing and the buffer
|
||
leaks.
|
||
- On failure the message formats `resolved_path` with `%s` while `realpath` leaves it
|
||
unspecified — for the normal caller who passed an uninitialised stack buffer, the error
|
||
path itself reads uninitialised memory and can crash.
|
||
- There is no way to express the buffer's size, so callers must know to supply `PATH_MAX`
|
||
bytes. Consider `aksl_realpath(const char *path, char *buf, size_t buflen)`, or an
|
||
allocating variant that returns the malloc'd pointer through an out-param.
|
||
|
||
### 2.2 Latent issues and API contract gaps
|
||
|
||
1. **`errno` is used as an error status where it may be stale.** `aksl_malloc` reports
|
||
`errno` when `malloc` returns NULL, but `malloc(0)` may legitimately return NULL without
|
||
setting `errno`, producing a `FAIL` with `status == 0` — an error context that every
|
||
`DETECT`/`CATCH` will read as *success* while still holding a pool slot. Guard every
|
||
`errno`-sourced status with a fallback (`errno ? errno : AKERR_IO`) and set `errno = 0`
|
||
before the call being wrapped.
|
||
|
||
2. **`aksl_fopen` does not validate `pathname` or `mode`.** `fopen(NULL, ...)` is UB. Add
|
||
`AKERR_NULLPOINTER` guards.
|
||
|
||
3. **`aksl_fread`/`aksl_fwrite` lose the transfer count and hide short transfers.**
|
||
- `ptr` is never NULL-checked in either function.
|
||
- Neither has an out-param for the number of members actually transferred, so a caller who
|
||
gets `AKERR_EOF` cannot tell how much data arrived. Add `size_t *nmemb_out`.
|
||
- If `nmemr != nmemb` but neither `feof` nor `ferror` is set, both functions fall through
|
||
to `SUCCEED_RETURN` — a short transfer reported as complete success.
|
||
- `aksl_fwrite`'s error message reads `"Error reading file"` (`src/stdlib.c:83`), and
|
||
checking `feof()` on a write path is meaningless.
|
||
|
||
4. **`aksl_sprintf` wraps `vsprintf`, which is unbounded.** There is no way for a caller to
|
||
bound the destination. Add `aksl_snprintf`/`aksl_vsnprintf` and consider deprecating the
|
||
`sprintf` wrapper, or reject it outright — an "error-handling" wrapper around an
|
||
unbounded write is a sharp edge the library exists to remove.
|
||
|
||
5. **No `format` attribute on the variadic wrappers.** Adding
|
||
`__attribute__((format(printf, 2, 3)))` (and the equivalents) restores the compile-time
|
||
format/argument checking that callers lose by going through the wrapper.
|
||
|
||
6. **`aksl_strhash_djb2` sign-extends bytes ≥ 0x80.** `src/stdlib.c:181` iterates a
|
||
`char *`, which is signed on x86/ARM Linux, so high bytes contribute a sign-extended
|
||
negative value and the hash differs from the canonical djb2 — and differs across
|
||
platforms where `char` is unsigned. Iterate a `const unsigned char *`. Also take
|
||
`const char *` in the signature so callers need not cast away constness.
|
||
|
||
7. **`aksl_tree_iterate` recursion is unbounded and cycle-blind.** A deep or degenerate tree
|
||
overflows the stack, and a child pointer that loops back to an ancestor recurses forever.
|
||
Add a depth limit (raising `AKERR_OUTOFBOUNDS`) and/or a visited check raising
|
||
`AKERR_CIRCULAR_REFERENCE`, consistent with what the list functions already do.
|
||
|
||
8. **`lalloc`, `lfree` and `queue` are dead parameters.** `lalloc`/`lfree` are defaulted and
|
||
then never called; `queue` is entirely unused (it is the only `-Wunused-parameter` warning
|
||
in the file). The doc comment even tells the caller to "pass NULL here", which is a sign
|
||
the queue belongs in an internal helper rather than the public signature. Either
|
||
implement BFS so they are used, or drop them from the public API until it is.
|
||
|
||
9. **Unknown `searchmode` values return success.** The `switch` in `aksl_tree_iterate` has no
|
||
`default:`; `searchmode == 99` and the header-defined `AKSL_TREE_SEARCH_VISIT` (5) both
|
||
fall straight through to `SUCCEED_RETURN` having visited nothing. Add
|
||
`default: FAIL_RETURN(e, AKERR_VALUE, ...)`.
|
||
|
||
10. **`AKSL_TREE_SEARCH_BFS`/`BFS_RIGHT` are unimplemented** (`AKERR_NOT_IMPLEMENTED`), and
|
||
`AKSL_TREE_SEARCH_VISIT` is declared in the header with a documented meaning but no
|
||
implementation anywhere.
|
||
|
||
11. **`aksl_memset`'s and `aksl_memcpy`'s inner checks are dead code.** `memset` returns `s`
|
||
and `memcpy` returns `d`, both unconditionally; neither can fail. The
|
||
`FAIL_ZERO_RETURN(e, memset(...), errno, ...)` and `(memcpy(...) == d)` checks can never
|
||
fire and only obscure the intent. Also, `memcpy` with overlapping ranges is UB — either
|
||
document that or dispatch to `memmove`.
|
||
|
||
12. **`aksl_list_pop` cannot tell the caller the new head.** Popping the head leaves the
|
||
caller's head pointer dangling at a now-detached node. Add an out-param for the new head,
|
||
or a `aksl_list_pop(aksl_ListNode **head, aksl_ListNode *node)` form.
|
||
|
||
13. **`aksl_free` does not clear the caller's pointer**, so double-free remains easy. Consider
|
||
an `aksl_freep(void **ptr)` that frees and NULLs.
|
||
|
||
14. **No initialisers for the public structs.** Every caller must remember to `memset` an
|
||
`aksl_ListNode`/`aksl_TreeNode` to zero before use (both existing tests do). Add
|
||
`aksl_list_node_init`/`aksl_tree_node_init` or `AKSL_LIST_NODE_INIT` macros.
|
||
|
||
15. **`aksl_TreeNode.parent` is declared but never set or read** by any library function.
|
||
|
||
16. **Header hygiene**: no `extern "C" { }` guard for C++ consumers; no version macros
|
||
(`AKSL_VERSION_MAJOR`…); `akstdlib.h` pulls in `stdio.h`/`stdlib.h`/`string.h`/`stdint.h`
|
||
into every consumer's namespace.
|
||
|
||
17. **Doxygen coverage is 2 functions out of 20.** A `Doxyfile` exists but only
|
||
`aksl_tree_iterate` and `aksl_list_iterate` have doc comments. Every public function
|
||
needs `@param`/`@throws`/`@return`, especially the ones whose contract deviates from libc
|
||
(`aksl_free(NULL)` is an error; `aksl_atoi` will become strict).
|
||
|
||
### 2.3 Build, CI and repository
|
||
|
||
- [ ] **`CMakeLists.txt:4-6` sets `CMAKE_CXX_FLAGS` in a C-only project**, so `-g -ggdb -pg`
|
||
never reach the C compiler. Use `CMAKE_C_FLAGS` — or better, drop `-pg` from the
|
||
default build entirely (it is what produces the stray `gmon.out` sitting untracked in
|
||
the repo root) and let `CMAKE_BUILD_TYPE` control debug info.
|
||
- [ ] **No warning flags.** Add `-Wall -Wextra` (and ideally `-Werror` in CI). The only
|
||
current warning is the unused `queue` parameter, so the cost of turning them on is low.
|
||
- [ ] `src/stdlib.c` triggers **"ISO C99 requires at least one argument for the `...`"** on
|
||
roughly 20 lines under `-Wpedantic`, because `FAIL_*` is called with a bare message and
|
||
no varargs. Either always pass an argument, or add a zero-arg-safe form in libakerror.
|
||
- [ ] **The `deps/libakerror` submodule is pinned 11 commits behind `main`** (pinned at
|
||
`4fad0ce "Add gitea workflow"`; upstream is at `4212ff0`). The pin predates the
|
||
refcount-leak fix, the stack-trace buffer-overflow fix, the `AKERR_MAX_ERR_VALUE`
|
||
correction and the format-string fix. Bump it.
|
||
- [ ] **CI does not build against the submodule it pins.** `.gitea/workflows/ci.yaml` clones
|
||
`libakerror@main` and installs it, while the build it then runs is top-level and so
|
||
compiles `deps/libakerror` at the pinned commit — two different libakerror versions
|
||
depending on where you build, and the installed one is never actually linked. Pick
|
||
one. (The submodule is at least *present* in CI now: §1.0 added
|
||
`submodules: recursive` to the checkout, without which configure failed outright.)
|
||
- [x] **`ctest` was red** (`tree` failing plus four *Not Run* submodule tests) — fixed in
|
||
§1.0. The build badge in `README.md` means something again.
|
||
- [ ] **Untracked build litter** in the repo root: `build/`, `gmon.out`,
|
||
`CMakeLists.txt-akstdlib`, and a dozen `*~` editor backups. Add a `.gitignore`
|
||
(libakerror has one; this repo does not).
|
||
- [ ] **`README.md` is a build badge and nothing else.** It needs at minimum: what the
|
||
library is, the akerror wrapper contract, the list of wrapped functions, and the
|
||
deviations from libc semantics.
|
||
- [ ] **Indentation is inconsistent** — `src/stdlib.c` mixes hard tabs and 4-space indents
|
||
within the same functions. libakerror standardised on Stroustrup style
|
||
(commit `e5f7616`); apply the same here, ideally with a checked-in `.clang-format`.
|
||
|
||
---
|
||
|
||
## 3. libc functions not yet wrapped
|
||
|
||
Ordered roughly by how much a caller of this library would miss them. Each entry means "add
|
||
an `aksl_`-prefixed wrapper that turns the documented failure modes into an
|
||
`akerr_ErrorContext`".
|
||
|
||
### 3.1 High priority — gaps in areas the library already covers
|
||
|
||
**Memory (`stdlib.h`, `string.h`)**
|
||
- [ ] `calloc`, `realloc`, `reallocarray` — `realloc`'s "returns NULL and the old pointer is
|
||
still valid" trap is exactly what this library should be hiding.
|
||
- [ ] `aligned_alloc`, `posix_memalign`
|
||
- [ ] `memmove`, `memcmp`, `memchr`
|
||
|
||
**Bounded string formatting (`stdio.h`)**
|
||
- [ ] `snprintf`, `vsnprintf` — needed to make §2.2.4 fixable.
|
||
- [ ] `vprintf`, `vfprintf`, `vsprintf` — the `va_list` forms, so consumers can build their
|
||
own variadic wrappers on top.
|
||
- [ ] `asprintf` / `vasprintf` (GNU) as an allocating alternative.
|
||
|
||
**String → number (`stdlib.h`)**
|
||
- [ ] `strtol`, `strtoll`, `strtoul`, `strtoull`, `strtod`, `strtof`, `strtold` — the correct
|
||
foundation for §2.1.5.
|
||
|
||
**Strings (`string.h`)**
|
||
- [ ] `strlen`, `strnlen`
|
||
- [ ] `strcpy`, `strncpy`, `strcat`, `strncat` — with truncation reported as an error rather
|
||
than silently accepted, which is the whole value proposition here.
|
||
- [ ] `strdup`, `strndup`
|
||
- [ ] `strcmp`, `strncmp`, `strcasecmp`, `strncasecmp`, `strcoll`
|
||
- [ ] `strchr`, `strrchr`, `strstr`, `strcasestr`, `strpbrk`, `strspn`, `strcspn`
|
||
- [ ] `strtok_r`, `strsep`
|
||
- [ ] `strerror_r`
|
||
|
||
**Stream I/O (`stdio.h`)**
|
||
- [ ] `fseek`, `ftell`, `rewind`, `fgetpos`, `fsetpos`, `fseeko`, `ftello`
|
||
- [ ] `fflush`
|
||
- [ ] `fgets`, `fputs`, `fgetc`/`getc`/`getchar`, `fputc`/`putc`/`putchar`, `ungetc`
|
||
- [ ] `getline`, `getdelim`
|
||
- [ ] `freopen`, `fdopen`, `fileno`
|
||
- [ ] `setvbuf`, `setbuf`
|
||
- [ ] `clearerr`, `feof`, `ferror` — thin, but worth exposing so callers never touch raw
|
||
`FILE *` internals.
|
||
- [ ] `sscanf`, `fscanf`, `scanf` — the return-value semantics (items matched vs `EOF`) are a
|
||
classic silent-failure source.
|
||
- [ ] `remove`, `rename`, `tmpfile`, `mkstemp`, `mkdtemp`
|
||
- [ ] `perror` — or an akerror-native equivalent.
|
||
|
||
### 3.2 POSIX file and process API (likely the next major surface)
|
||
|
||
**`unistd.h` / `fcntl.h`**
|
||
- [ ] `open`, `close`, `read`, `write`, `pread`, `pwrite`, `lseek`
|
||
- [ ] `readv`, `writev`
|
||
- [ ] `dup`, `dup2`, `pipe`, `fcntl`
|
||
- [ ] `fsync`, `fdatasync`, `truncate`, `ftruncate`
|
||
- [ ] `unlink`, `link`, `symlink`, `readlink`, `rmdir`, `mkdir`
|
||
- [ ] `access`, `faccessat`, `chmod`, `fchmod`, `chown`, `fchown`, `umask`
|
||
- [ ] `chdir`, `fchdir`, `getcwd`
|
||
- [ ] `isatty`, `ttyname_r`
|
||
- [ ] `sysconf`, `pathconf`
|
||
- [ ] `sleep`, `usleep`, `nanosleep`
|
||
|
||
**`sys/stat.h`**
|
||
- [ ] `stat`, `fstat`, `lstat`, `fstatat`
|
||
- [ ] `statvfs`, `fstatvfs`
|
||
|
||
**`dirent.h`**
|
||
- [ ] `opendir`, `fdopendir`, `readdir`, `readdir_r`, `closedir`, `rewinddir`, `scandir`
|
||
|
||
**Process control**
|
||
- [ ] `fork`, `execve` / `execvp` / `execl` family, `waitpid`, `wait`
|
||
- [ ] `posix_spawn`
|
||
- [ ] `system`, `popen`, `pclose`
|
||
- [ ] `getpid`, `getppid`, `getuid`, `geteuid`, `setuid`, `setgid`
|
||
- [ ] `atexit`, `exit`, `_exit`, `abort` — mostly to give akerror a hook on shutdown.
|
||
- [ ] `getenv`, `setenv`, `unsetenv`, `putenv`, `clearenv`
|
||
|
||
**`sys/mman.h`**
|
||
- [ ] `mmap`, `munmap`, `mprotect`, `msync`, `madvise`
|
||
|
||
### 3.3 Time
|
||
|
||
- [ ] `time`, `clock_gettime`, `clock_getres`, `gettimeofday`
|
||
- [ ] `localtime_r`, `gmtime_r`, `mktime`, `timegm`, `difftime`
|
||
- [ ] `strftime`, `strptime`
|
||
- [ ] `clock`, `times`
|
||
|
||
### 3.4 Sorting, searching and misc `stdlib.h`
|
||
|
||
- [ ] `qsort`, `qsort_r`, `bsearch` — comparator errors currently have nowhere to go; an
|
||
akerror-aware comparator signature would be a genuine improvement.
|
||
- [ ] `abs`, `labs`, `llabs`, `div`, `ldiv`, `lldiv`
|
||
- [ ] `rand`, `srand`, `random`, `srandom`, `getrandom`/`arc4random`
|
||
|
||
### 3.5 Lower priority / larger projects
|
||
|
||
- [ ] **Sockets**: `socket`, `bind`, `listen`, `accept`, `connect`, `send`/`sendto`/`sendmsg`,
|
||
`recv`/`recvfrom`/`recvmsg`, `shutdown`, `setsockopt`/`getsockopt`, `getaddrinfo`/
|
||
`freeaddrinfo`/`gai_strerror`, `inet_ntop`/`inet_pton`
|
||
- [ ] **Multiplexing**: `select`, `poll`, `ppoll`, `epoll_create1`/`epoll_ctl`/`epoll_wait`
|
||
- [ ] **Signals**: `sigaction`, `sigprocmask`, `sigemptyset`/`sigaddset`, `kill`, `raise`,
|
||
`signalfd`
|
||
- [ ] **Threads**: `pthread_create`/`join`/`detach`, `pthread_mutex_*`, `pthread_cond_*`,
|
||
`pthread_rwlock_*`, `sem_*` — blocked on resolving the thread-safety question in §1.9
|
||
(libakerror's error pool is an unlocked process-global array).
|
||
- [ ] **Dynamic loading**: `dlopen`, `dlsym`, `dlclose`, `dlerror`
|
||
- [ ] **Locale / wide chars**: `setlocale`, `mbstowcs`, `wcstombs`, `iconv_*`
|
||
- [ ] **Math**: the `math.h` functions that set `errno`/raise FP exceptions (`sqrt`, `log`,
|
||
`pow`, `acos`, …) — probably better served by a dedicated `libakmath`.
|
||
|
||
### 3.6 Non-libc additions the current data structures imply
|
||
|
||
Not libc wrappers, but the existing list/tree API is visibly incomplete:
|
||
|
||
- [ ] List: `prepend`, `insert_after`/`insert_before`, `length`, `find`, `reverse`,
|
||
`concat`, `free_all`, reverse iteration, and a head/tail-tracking container type so
|
||
`append` is O(1) instead of O(n).
|
||
- [ ] Tree: `insert`, `remove`, `find`, `height`, `count`, `free_all`, and the BFS traversal
|
||
the `lalloc`/`lfree`/`queue` parameters were designed for.
|
||
- [ ] Hash map built on `aksl_strhash_djb2`, plus additional hashes (FNV-1a, xxHash) and a
|
||
NUL-terminated `aksl_strhash_djb2_str` convenience form.
|
||
- [ ] Growable buffer / string-builder type, to make the `snprintf` and `strcat` wrappers
|
||
pleasant to use.
|