fix: fix(perf): a NULL cache crashes eb_cache_get, and an over-budget put never returns - #24
Draft
srpatcha wants to merge 2 commits into
Draft
fix: fix(perf): a NULL cache crashes eb_cache_get, and an over-budget put never returns#24srpatcha wants to merge 2 commits into
srpatcha wants to merge 2 commits into
Conversation
… put never returns Opened by the scheduled autoreview pipeline after review of open PRs. Reviewed against the EmbeddedOS Master Design v2.0. Files: src/perf/perf.c tests/CMakeLists.txt tests/test_perf_cache.c
Performance Regression Check ✅ All metrics within thresholds
Full resultsCommit: de46757 |
srpatcha
commented
Sep 6, 2026
srpatcha
left a comment
srpatcha
left a comment
Member
Author
There was a problem hiding this comment.
Review — eBrowser#24 "fix(perf): a NULL cache crashes eb_cache_get, and an over-budget put never returns"
head: f6ccb4a author: srpatcha ci: fail
Verdict: Both guard repairs in src/perf/perf.c are correct and the new tests genuinely fail against origin/master, but the eviction fix trades the hang for a rejected eb_cache_put() that first empties the entire cache — I reproduced that. CI is red for a repo-wide reason unrelated to this diff.
This PR was opened by this same automated pipeline. It carries no approval from me and needs a human decision before merge.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium (P2) | src/perf/perf.c:167-181 |
A put that will be refused evicts the whole cache first. The loop runs before anything establishes that sz can ever fit, so an over-budget value drains every live entry and then returns false. Verified by execution against this branch: a cache holding a and b (count=2, bytes=40, budget 64) has count=0, bytes=0 and both keys gone after eb_cache_put(&c,"big",…,256,0) returns false. Before this PR the call hung, so the eviction was never observable; it is now. |
Reject impossible entries before evicting anything. At the top of the eviction block: if (c->max_entries < 1) return false; and if (c->max_bytes && sz > c->max_bytes) return false;. The existing loop and post-loop guards then only ever run on entries that can actually fit. Add a case asserting a pre-existing entry survives a refused oversized put — test_put_larger_than_byte_budget_is_refused (tests/test_perf_cache.c:94) starts from an empty cache, so it cannot see this. |
| 2 | High (P1) | .github/workflows/ci.yml:24-33 |
All three required Test (Python 3.x) checks fail, and mergeStateStatus is BLOCKED. Not caused by this diff. Root cause confirmed from the job steps (job 101555640536): Set up Python 3.10 is the failing step and every later step is skipped. actions/setup-python@v5 is configured with cache: pip, which requires a dependency file, and the repo contains no requirements.txt, pyproject.toml, setup.py, setup.cfg or Pipfile. The next step, pip install -r requirements.txt, points at the same missing file. |
Out of scope for this PR, but it is what blocks it. Either drop cache: pip and the -r requirements.txt install and make the matrix install only the tools it uses, or add the dependency file the workflow already assumes. No open PR in this repo covers it (#21/#22/#23 are dependabot action bumps). Worth a tracked issue rather than folding into this change. |
| 3 | High (P1) | src/perf/perf.c:127, src/network/cache.c:7 |
Out of diff, disclosed in the PR body, confirmed here with the link evidence the body does not have. eb_cache_init, eb_cache_put and eb_cache_evict_lru are each defined twice with incompatible signatures — eb_lru_cache_t in eb_perf, eb_cache_t in eBrowser_network. tests/CMakeLists.txt:86-90 (benchmark) and :108-111 (perf_regression) link both archives. Static archives resolve first-match, so which implementation a call binds to depends on link order, and the two structs have different layouts. The build does not error on this, which is what makes it dangerous. |
Agreed this is not a guard fix. It needs an owning issue: rename one family (eb_lru_* for the perf one is the smaller blast radius, since eb_cache_get is only defined in perf.c). Do not fix it inside this PR. |
| 4 | Low (P3) | tests/test_perf_cache.c:76-77 |
On a failed assertion RUN() still prints PASS and increments s_pass. Verified by forcing two assertions to fail: output shows FAIL: … immediately followed by PASS, and the summary reads 6 passed, 2 failed. The exit code is still 1, so ctest does catch it — the damage is a misleading log and a wrong pass count, not a missed failure. Inherited verbatim from tests/test_bookmark.c:8-9, so this is house style rather than something this PR invented. |
Smallest fix, local to the new file: have ASSERT set a per-test flag and let RUN print PASS/FAIL from it. Fixing it repo-wide is a separate cleanup — say so rather than quietly copying the macro forward. |
Not a finding, checked and cleared:
- The PR body's verification table is backed.
state/verify/eBrowser__perf-cache-guards.ctest.*.logshows 21/21 tests passing includingtest_perf_cache; the0sin the table is whole-second rounding ofTotal Test time (real) = 0.37 sec. The firstcppcheckattempt exited 127 (binary not on PATH) and was re-run through the toolchain wrapper, where it passed — the log keeps both, as it should. #include "eBrowser/perf.h"resolves:include/eBrowseris a tracked symlink toinclude/ebrowser, not a second copy of the headers.- The post-loop guard
c->count >= c->max_entriesdoes not reject entries that fit.eb_cache_init()clampsmaxtoEB_CACHE_MAX_ENTRIES, and a negativemaxnow returnsfalseinstead of hanging.
Architecture conformance
Conforms. eBrowser is Tier 5 — Applications (§21), and §20.1 places it as a reference application rather than a peer of the kernel. The diff is confined to src/perf/ and tests/ inside that repo. No #include, target_link_libraries entry or manifest field points up a tier: the new test links eb_perf, an in-repo target, and includes only in-repo headers plus libc. No public header, struct layout or serialized format changes, so §23.2 raises nothing. §21.1 is not engaged — no repository or subsystem boundary moves.
The only behaviour change is the one finding 1 describes: an input that previously never returned now returns false and has a side effect on the way out.
Proposed changes
- Refuse impossible entries before evicting (finding 1), in
eb_cache_put()immediately before the eviction loop:
/* Nothing can be evicted into existence — refuse before touching the cache. */
if (c->max_entries < 1) return false;
if (c->max_bytes && sz > c->max_bytes) return false;With that in place the existing loop is only entered when the entry can fit, and the two post-loop guards become the belt-and-braces they read as.
-
Extend
test_put_larger_than_byte_budget_is_refusedto seed one entry first and assert it is still retrievable after the refused put. That is the case that currently has no coverage. -
Fix the
PASS-after-FAILreporting in the new file only (finding 4). Leave the other test files alone in this PR. -
Leave findings 2 and 3 to their own issues. Both are pre-existing and neither is small enough to be provable inside this change.
Not checked
- I did not run this repo's CMake build or ctest myself. My evidence comes from two places: the pipeline's own recorded logs under
state/verify/, and a standalone harness I compiled from this branch'ssrc/perf/perf.cplus the repo headers withgcc -fsanitize=address,undefined. Under that harness the PR's six cases pass clean; that is not the same as the repo's real build configuration, which I did not reproduce. - I did not verify the pre-fix SIGSEGV or the two hangs the PR body reports. I verified the post-fix behaviour and reproduced the new eviction side effect; the before-state I am taking from the PR body.
eb_cache_remove()'s array-shift-versus-linked-list corruption is acknowledged in the PR body and I did not analyse it. Note thateb_cache_evict_lru()calls straight into it, so the eviction loop this PR modifies runs on top of that defect — the newif (c->count >= before) break;will stop a corrupted list from spinning, but I have not established what it evicts in that state.- No fuzz or concurrency testing.
eb_cache_*has no internal locking and I did not check whether any caller reaches it from more than one thread. - CI: I read the step conclusions through the API. The full job logs were not retrievable — the run was still in progress when I queried, so
gh run view --log-failedreturned nothing. The root cause in finding 2 is inferred from the failing step name plus the confirmed absence of every dependency file the step needs, not from the log text.
Automated architecture review of f6ccb4a3d2d8 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Performance Regression Check ✅ All metrics within thresholds
Full resultsCommit: 114af0a |
srpatcha
commented
Sep 10, 2026
srpatcha
left a comment
srpatcha
left a comment
Member
Author
There was a problem hiding this comment.
Review — eBrowser#24 "fix(perf): a NULL cache crashes eb_cache_get, and an over-budget put never returns"
head: b7a4fe6 author: srpatcha ci: fail (3 × Test (Python 3.x)) draft: true
Verdict: Follow-up. b7a4fe6 fixes the eviction side effect correctly — I re-ran it and a refused oversized put now leaves the cache intact. But the same commit repurposed one of the two regression tests, so the zeroentries case in the PR body's own reproduction table no longer has coverage.
This PR was opened by this automated pipeline. It carries no approval and still needs a human decision before merge.
Prior findings
| Prior # | Status | Evidence |
|---|---|---|
| 1 — a put that will be refused evicts the whole cache first | Resolved in b7a4fe6 |
The early rejection now sits at src/perf/perf.c:151, before both the dedup loop and the eviction loop. Re-verified by execution: a cache with max_entries=8, max_bytes=64 holding keep=9 bytes, then eb_cache_put(&c,"big",…,256,0) → returns false, count=1, current_bytes=9, eb_cache_get(&c,"keep",NULL) still hits. Previously this drained the cache. |
2 — all three Test (Python 3.x) checks fail; setup-python cache: pip with no dependency file |
Untouched — still open, still blocking | Re-verified on origin/master: .github/workflows/ci.yml:26 sets cache: pip and :32 runs pip install -r requirements.txt, while the repository contains no requirements.txt, pyproject.toml, setup.py, setup.cfg or Pipfile. Three checks red, mergeStateStatus: BLOCKED. |
3 — eb_cache_init/eb_cache_put/eb_cache_evict_lru defined twice with incompatible signatures |
Untouched | Still present at head: src/network/cache.c:7,31,73 (eb_cache_t) versus src/perf/perf.c:127,148,231 (eb_lru_cache_t). tests/CMakeLists.txt:97,103,112 still link eb_perf alongside the network archive. Correctly out of scope for a guard fix; still needs its own issue. |
4 — RUN() prints PASS and increments s_pass even after a failed assertion |
Resolved in b7a4fe6, in the new file only |
tests/test_perf_cache.c:15-21 now snapshots s_fail and only prints PASS/increments when it is unchanged. The repo-wide instance in tests/test_bookmark.c:8-9 is untouched, which is the right scope. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium (P2) | tests/test_perf_cache.c:52-60 |
The fix commit removed coverage of one of the two regressions this PR exists to fix. test_put_into_zero_entry_cache_is_refused used to call eb_cache_init(&c, 0, 0) — exactly the zeroentries row in the PR body's reproduction table, and one of the two inputs the body says used to hang. It now calls eb_cache_init(&c, 1, 0), inserts an entry, and reaches the guarded state by assigning c.max_entries = 0 directly. No test in the file constructs a zero-entry cache through the API any longer: the five eb_cache_init calls use 8, 8, 1, 4, 8. The behaviour is still correct — I checked it through the public API and eb_cache_init(&c,0,0) followed by a put returns false with count=0 — but it is now correct-and-untested, and a future edit to the max_entries <= 0 clause at perf.c:151 would go unnoticed. Separately, poking c.max_entries reaches into the struct and manufactures a state eb_cache_init cannot produce (max_entries=0 with count=1), which couples the test to the layout. |
Keep both cases rather than trading one for the other. Restore the original as test_put_into_zero_entry_cache_is_refused (eb_cache_init(&c, 0, 0), put refused, count == 0), and get the survives-a-refusal assertion from the API instead of the struct: eb_cache_init(&c, 1, 0), put keep, then a second put with a different key must be refused with keep still retrievable. That exercises the same count >= max_entries guard without touching internals. |
| 2 | Medium (P2) | src/perf/perf.c:152-167 |
The update branch bypasses the byte budget entirely, and the new guard reads as though it does not. When the key already exists, the function does current_bytes -= n->value_size … current_bytes += sz and returns true — no eviction, no budget check. Reproduced: a cache with max_bytes=64 holding a=40 and k=8 (current_bytes=48), then eb_cache_put(&c,"k",<64 bytes>,64,0) returns true and leaves current_bytes=104, 62% over budget, with nothing evicted. This is pre-existing — I ran the identical harness against origin/master and got the same 104, so it is not a regression from this PR. It belongs here because the new line 151 is inserted immediately above this loop and its comment says "Reject entries that can never fit before evicting usable data", which a reader will take as covering the update path. It bounds one value against the budget; it says nothing about the total. |
Not this PR's to fix — the guard repairs were deliberately scoped. But say so in the body rather than leaving the new comment overclaiming, and open an issue: the update branch should run the same evict-then-check sequence as the insert path, using sz - n->value_size as the delta. Note the same branch also leaks state on malloc failure — current_bytes is already decremented and the old buffer already freed when n->value = malloc(sz) returns NULL, leaving the node with value == NULL and a stale value_size, so a later eb_cache_remove double-subtracts. |
| 3 | Low (P3) | src/perf/perf.c:174, 181 |
The overflow-safe rewrite c->current_bytes > c->max_bytes - sz is correct only because of the guard at line 151. max_bytes and sz are both size_t (include/ebrowser/perf.h:35), so if sz > max_bytes ever reached these lines the subtraction would wrap to a huge value and the over-budget entry would be accepted — the exact opposite of the intent. Today line 151 makes that unreachable, and I confirmed the boundary behaves: sz == max_bytes on an empty cache is accepted (put=1, count=1, bytes=64), and the evict-to-fit path still works (a evicted, b cached). The dependency between the three lines is not recorded anywhere. |
One comment on line 174: /* Safe only because line 151 guarantees sz <= max_bytes; both are size_t. */. Cheaper than the bug it prevents. |
| 4 | Low (P3) | .github/workflows/ci.yml:36-41 |
Lint (ruff) and Type check (mypy) both carry continue-on-error: true, so neither can fail the job. Pre-existing and not touched by this PR, noted because it is in the same job as prior finding 2 and will still be true after that is fixed — the matrix will go green while two of its five steps remain incapable of reporting anything. |
Fold into whatever fixes prior finding 2: once the job can run at all, decide whether ruff and mypy are gates or advisory, and if advisory, move them to a separate non-required job rather than neutering them in place. |
Verified clean this round, by execution against head b7a4fe6:
eb_cache_get(NULL, "k", NULL)returnsNULLwithout dereferencing — the original crash is gone (perf.c:197,if (!c) return NULL;before themisses++).eb_cache_init(&c, 0, 0)+ put returnsfalse,count=0, no hang. The behaviour finding 1 is about is correct; only its test is missing.sz == max_byteson an empty cache is accepted rather than rejected off-by-one —put=1, count=1, current_bytes=64with the value retrievable.- The normal eviction path still makes room: two 40-byte values into a 64-byte budget leaves
count=1, current_bytes=40,aevicted,bcached. max_bytes == 0still means unbounded rather than "nothing fits" —put=1, count=1.- Everything above ran under
-fsanitize=address,undefinedwith no sanitizer diagnostics in the cache paths.
Architecture conformance
Conforms, unchanged from the previous review. eBrowser is Tier 5 — Applications (§21), and §20.1 is explicit that it is a reference application "proving platform capability, not a peer pillar of the kernel" — so a cache-guard fix inside src/perf/ is squarely where it belongs and carries no wider obligation. The diff touches only src/perf/ and tests/ within that repo. §5.1 is not engaged: the new test links eb_perf, an in-repo target, and includes only in-repo headers plus libc; no #include, target_link_libraries entry or manifest field points up a tier. No public header, struct layout or serialized format changes, so §23.2's compatibility contracts and brief §8 raise nothing — eb_lru_cache_t is untouched, and the only behavioural change remains a non-terminating input becoming false.
§28's evidence policy is where prior finding 2 sits, and it is worth naming plainly: this repository's Python test matrix has never run on this PR, so the Implemented-level evidence §28 requires ("code and functional tests") is unavailable for anything here except through the C harness the pipeline runs separately. No proposal appended — nothing here is a gap in the master design's wording.
Proposed changes
- Restore the
eb_cache_init(&c, 0, 0)case and get the survival assertion through the API instead ofc.max_entries = 0(finding 1). Both cases, six lines. - Add the one-line comment tying
perf.c:174/:181to the guard at:151(finding 3). - Note in the body that the update branch is out of scope and unbudgeted, so the new comment does not overclaim (finding 2), and open the issue.
- Prior finding 2 is what actually blocks this. It is a four-line workflow fix — drop
cache: pip, droppip install -r requirements.txt, keep the explicit tool install that follows it — and it unblocks every PR in this repository, not just this one. It should be its own PR; no open PR here covers it (#21/#22/#23 are Dependabot action bumps). - Prior finding 3 (the duplicate
eb_cache_*symbol families) still needs an owning issue. Renaming the perf family toeb_lru_*remains the smaller blast radius.
Not checked
- I did not run this repository's CMake build or its
ctestsuite. The verification above comes from a standalone harness compiled from this branch'ssrc/perf/perf.cplus the repo headers undergcc -I include -fsanitize=address,undefined, in a temporary extraction. That is not the repo's real build configuration, and in particular it links onlyperf.c, so it cannot exhibit the symbol collision in prior finding 3. - The pre-fix SIGSEGV and the two hangs are still taken from the PR body. I verified post-fix behaviour and reproduced finding 2 against both
origin/masterand this head; I did not reconstruct the original crash. - I did not read the failing CI job logs. Prior finding 2's root cause is re-confirmed from the workflow file plus the confirmed absence of every dependency file it references, not from log text. The three checks are red; that they are red for this reason remains an inference, though a well-supported one.
eb_cache_remove()'s array-shift-versus-linked-list interaction is still unanalysed, andeb_cache_evict_lru()calls straight into it, so the eviction loop this PR modifies still runs on top of it. Theif (c->count >= before) break;bounds the loop; what it evicts in a corrupted state I have not established.- No fuzz and no concurrency testing.
eb_cache_*has no internal locking and I have not checked whether any caller reaches it from more than one thread. - The
Performance Checkjob passes and its posted table shows all 12 metrics within thresholds, but none of those metrics exerciseeb_cache_*— I did not measure whether the added guard costs anything on the put path, and I would not expect it to. - Nothing was pushed and no merge was attempted. The clone was read through
git show/git archiveagainstFETCH_HEAD; its working tree is unchanged.
Automated architecture review of b7a4fe6a9661 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two guard clauses in the LRU cache (
src/perf/perf.c) are wrong. One crashes,one hangs. Both are reachable from the public API in
include/ebrowser/perf.h.1.
eb_cache_get()dereferences the NULL cache it is rejecting —src/perf/perf.c:185When
cis NULL the guard takes the branch and then writes throughc. Thecall segfaults instead of returning NULL. Surfaced by the maintenance health
scan (
state/maint/20260906T133057/eBrowser.md, section C — unused functionsand dead code):
src/perf/perf.c:185: warning: Either the condition '!c' is redundant or there is possible null pointer dereference: c.2.
eb_cache_put()spins forever when eviction cannot help —src/perf/perf.c:167eb_cache_evict_lru()returns immediately oncec->tailis NULL, so on anempty cache the loop body changes nothing while the condition stays true. Two
ordinary inputs reach that state:
fit, keep evicting nothing;
eb_cache_init(&c, 0, ...)—count >= max_entriesis0 >= 0, true before a single entry exists.Root cause
Both are the same mistake in different clothes: a guard written for the common
case and never checked against the boundary. The first assumes the pointer it
just tested is usable; the second assumes eviction always makes progress.
Reproduction
A three-case harness compiled against unmodified
src/perf/perf.catorigin/masterbb37c5b (gcc -I include harness.c src/perf/perf.c -lpthread),each case run under
timeout 5:nullgeteb_cache_get(NULL, "k", NULL)overbudgeteb_cache_put(&c, "big", 256 bytes, budget 64)zeroentrieseb_cache_putoneb_cache_init(&c, 0, 0)The fix
eb_cache_get(): split the guard so a NULL cache returns before any memberaccess. A NULL-key call on a real cache still counts a miss, as before.
eb_cache_put(): break out of the eviction loop whencountstops falling,then refuse the entry if it still does not fit. The only behaviour that
changes is a hang becoming
false— the previous code never returned fromthese inputs, so no caller can be relying on the old result.
Files changed
src/perf/perf.c— the two guards.tests/test_perf_cache.c— new. Six cases: the three regressions above, aNULL-key miss, a put/get/remove roundtrip, and an eviction that does make
room, so the loop's normal path stays covered.
tests/CMakeLists.txt— registerstest_perf_cachewith ctest, withTIMEOUT 30. Both regressions were hangs rather than wrong answers; withouta timeout property a reintroduction would stall the suite instead of failing
it.
eb_perfhad no ctest target before this. The three executables that link it(
benchmark,http2_server,load_test_combined) are build targets with noadd_test(), so nothing insrc/perf/was executed by the test suite — whichis why a segfaulting cache lookup survived in the tree.
Impact and risk
Local to
src/perf/. The eviction loop gained one comparison per iteration;the cache path is otherwise untouched. No public header, ABI or manifest
changes. Nothing outside
src/perf/perf.cis modified.Two things this deliberately does not touch, both recorded in the
maintenance backlog for a maintainer rather than fixed here:
eb_cache_remove()compactsc->entries[]with an element shift whilec->head,c->tailand each node'sprev/nextpoint into that samearray, so a removal from the middle leaves the LRU list pointing at the wrong
slots. Repairing that is a data-structure change, not a guard fix.
eb_cache_init,eb_cache_put,eb_cache_getandeb_cache_evict_lruareeach defined twice with incompatible signatures — once in
src/perf/perf.cagainst
eb_lru_cache_t, once insrc/network/cache.cagainsteb_cache_t.tests/benchmarklinks both archives today. Resolving it means renaming apublic API.
Surfaced by the scheduled maintenance sweep, 2026-09-06.
Verification
Executed in an isolated worktree branched from
origin/master:buildcmake --build build/host --parallel 4cmakecmake -B build/host -G Ninja -DBUILD_TESTING=ONcppcheck/home/srpatcha/eos/.ai/toolchains/run-in-toolchain.sh /home/srpatcha/eos/.ai/autoreview/state/worktrees/eBrowser__perf-cache-guards -- cppcheck --enable=warning,portability --inline-suppr --error-exitcode=1 -I include src/perf/perf.c tests/test_perf_cache.cctestctest --test-dir build/host --output-on-failure --no-tests=error --parallel 4Opened by the scheduled autoreview pipeline (model
claude-opus-5), branched fromorigin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.Fixes #27