docs: n-gram edge coverage implementation plan - #5
Conversation
Co-authored-by: daedalus <115175+daedalus@users.noreply.github.com>
Reviewer's GuideAdds a detailed implementation plan for configurable n-gram edge coverage, covering AFL shim state and hashing, ELF/Python integration, map sizing, ptrace behavior, compatibility and resume handling, affected files, and validation strategy. Sequence diagram for n-gram coverage reset and edge mappingsequenceDiagram
participant Target
participant AFLShim
participant SharedMap
participant ForkServer
ForkServer->>AFLShim: initialize ring state
AFLShim->>AFLShim: zero __afl_prev_locs and reset __afl_prev_idx
Target->>AFLShim: __afl_map_edge(cur_loc)
AFLShim->>AFLShim: FNV-1a mix ring slots and cur_loc
AFLShim->>SharedMap: update edge_id bucket
AFLShim->>AFLShim: append cur_loc >> 1 and advance ring
AFLShim->>AFLShim: __afl_guarded_reset()
AFLShim->>AFLShim: zero ring and reset index
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/ngram_coverage_plan.md" line_range="39-48" />
<code_context>
+uint32_t __afl_prev_loc = 0;
+
+// After
+#ifndef __AFL_NGRAM_K
+#define __AFL_NGRAM_K 2 /* default: retain current 2-node behaviour */
+#endif
+
+#if __AFL_NGRAM_K > 1
</code_context>
<issue_to_address>
**issue (broader_impact):** The documented default `__AFL_NGRAM_K=2` does not retain the existing layout or edge IDs: `#if __AFL_NGRAM_K > 1` selects the new FNV ring path, while the purported original implementation is in the `#else` branch, which is reached only for k <= 1. A default build therefore stops exporting `__afl_prev_loc` and changes every edge ID, breaking the stated k=2 ABI and corpus/resume compatibility.
**Triggers:** When the shim is rebuilt with the documented default configuration.
**Suggested fix:** Use a dedicated `__AFL_NGRAM_K == 2` compatibility branch that preserves `__afl_prev_loc` and the existing XOR hash, and reserve the ring/FNV path for k > 2.
</issue_to_address>
### Comment 2
<location path="docs/ngram_coverage_plan.md" line_range="168-173" />
<code_context>
+The ptrace path simulates `(rel ^ self.prev_location) % map_size`.
+Extend with a ring:
+```python
+# prev_locations: collections.deque(maxlen=k-1), initially all-zero
+edge_id = rel
+for p in self.prev_locations:
+ edge_id ^= p # simple XOR chain is fine for ptrace (no perf path)
+bucket = edge_id % self.map_size
+self.prev_locations.appendleft(rel >> 1)
+```
+Note: the ptrace path has no `caller_ctx` and never will; this is an
</code_context>
<issue_to_address>
**issue (bug_risk):** The ptrace design replaces `prev_location` with `prev_locations` but does not specify updating `reset_edge_map()` to clear or recreate the deque. After one execution, the next execution hashes its first breakpoint against the previous execution's history, so identical inputs produce different coverage across iterations.
**Triggers:** When ptrace coverage runs more than one target execution on the same `PtraceCoverage` instance.
**Suggested fix:** Clear and reinitialize `prev_locations` in `reset_edge_map()` alongside the existing edge-map reset.
</issue_to_address>
### Comment 3
<location path="docs/ngram_coverage_plan.md" line_range="151" />
<code_context>
+ (drop-rate exceeds 1 % at load > 0.75, see `afl_shim.c:336–339`).
+
+4. **`MapSizeEstimate` (`elf.py:1670`)**: add `ngram_k: int` field alongside
+ `ctx_bits: int`.
+
+### `shm.py` — no layout change
</code_context>
<issue_to_address>
**issue (bug_risk):** Adding `ngram_k` to the `MapSizeEstimate` NamedTuple requires updating every positional constructor, but the complete file-change list only calls out the field addition. The existing `MapSizeEstimate(MAP_SIZE_DEFAULT, 0, "default", ctx_bits, False)` and five-argument success constructor will raise `TypeError` once the tuple has six fields.
**Triggers:** When the planned `MapSizeEstimate` field is implemented without updating all constructors and tests.
**Suggested fix:** Update every `MapSizeEstimate(...)` construction and all positional consumers to carry `ngram_k`, preferably using named arguments for this metadata tuple.
</issue_to_address>
### Comment 4
<location path="docs/ngram_coverage_plan.md" line_range="131-133" />
<code_context>
+
+### `elf.py` — detection and map-size estimation
+
+1. **`detect_ngram_k(target: str) -> int`** (new, mirrors `detect_ctx_bits`
+ at `elf.py:1587`): scan the ELF symtab for `__afl_ngram_k_N`; return N.
+ Return 2 (current default) when the symbol is absent.
+
+2. **`ngram_inflation_factor(k: int) -> float`**: k-gram cardinality grows
</code_context>
<issue_to_address>
**issue (bug_risk):** The proposed detector returns a single unspecified `N` even though a shared object can contain multiple instrumented translation units and therefore multiple `__afl_ngram_k_N` symbols. Without the existing detector's explicit widest-value policy or a validation error for disagreement, map sizing can be based on a smaller k than one of the loaded TUs uses.
**Triggers:** When a target or shared library contains TUs built with different n-gram settings.
**Suggested fix:** Scan all marker symbols, reject conflicting values or deliberately select the maximum, and document that policy as `detect_ctx_bits` does.
```suggestion
1. **`detect_ngram_k(target: str) -> int`** (new, mirrors `detect_ctx_bits`
at `elf.py:1587`): scan all ELF symtab markers matching `__afl_ngram_k_N`;
if values disagree, select the maximum so the map is sized for the widest
instrumented translation unit, matching `detect_ctx_bits`'s policy. Return 2
(current default) when the symbol is absent.
```
</issue_to_address>
### Comment 5
<location path="docs/ngram_coverage_plan.md" line_range="44-45" />
<code_context>
+#endif
+
+#if __AFL_NGRAM_K > 1
+static uint32_t __afl_prev_locs[__AFL_NGRAM_K - 1];
+static uint8_t __afl_prev_idx = 0;
+#else
+uint32_t __afl_prev_loc = 0; /* k=1: unchanged ABI */
</code_context>
<issue_to_address>
**issue (bug_risk):** The ring index is declared as `uint8_t`, so it cannot represent the full index range when configurable `__AFL_NGRAM_K - 1` exceeds 255. For k > 256 the index wraps at 256 rather than at the ring length, causing out-of-range accesses or incorrect history selection.
**Triggers:** When a build sets `__AFL_NGRAM_K` above 256.
**Suggested fix:** Use an index type wide enough for the configured ring or enforce a compile-time maximum and static assertion for `__AFL_NGRAM_K`.
</issue_to_address>Sourcery assessment
Approval pending. 5 findings to address first.
Blocking findings: docs/ngram_coverage_plan.md:48, docs/ngram_coverage_plan.md:173, docs/ngram_coverage_plan.md:151, docs/ngram_coverage_plan.md:133, docs/ngram_coverage_plan.md:45
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #ifndef __AFL_NGRAM_K | ||
| #define __AFL_NGRAM_K 2 /* default: retain current 2-node behaviour */ | ||
| #endif | ||
|
|
||
| #if __AFL_NGRAM_K > 1 | ||
| static uint32_t __afl_prev_locs[__AFL_NGRAM_K - 1]; | ||
| static uint8_t __afl_prev_idx = 0; | ||
| #else | ||
| uint32_t __afl_prev_loc = 0; /* k=1: unchanged ABI */ | ||
| #endif |
There was a problem hiding this comment.
issue (broader_impact): The documented default __AFL_NGRAM_K=2 does not retain the existing layout or edge IDs: #if __AFL_NGRAM_K > 1 selects the new FNV ring path, while the purported original implementation is in the #else branch, which is reached only for k <= 1. A default build therefore stops exporting __afl_prev_loc and changes every edge ID, breaking the stated k=2 ABI and corpus/resume compatibility.
Triggers: When the shim is rebuilt with the documented default configuration.
Suggested fix: Use a dedicated __AFL_NGRAM_K == 2 compatibility branch that preserves __afl_prev_loc and the existing XOR hash, and reserve the ring/FNV path for k > 2.
| # prev_locations: collections.deque(maxlen=k-1), initially all-zero | ||
| edge_id = rel | ||
| for p in self.prev_locations: | ||
| edge_id ^= p # simple XOR chain is fine for ptrace (no perf path) | ||
| bucket = edge_id % self.map_size | ||
| self.prev_locations.appendleft(rel >> 1) |
There was a problem hiding this comment.
issue (bug_risk): The ptrace design replaces prev_location with prev_locations but does not specify updating reset_edge_map() to clear or recreate the deque. After one execution, the next execution hashes its first breakpoint against the previous execution's history, so identical inputs produce different coverage across iterations.
Triggers: When ptrace coverage runs more than one target execution on the same PtraceCoverage instance.
Suggested fix: Clear and reinitialize prev_locations in reset_edge_map() alongside the existing edge-map reset.
| (drop-rate exceeds 1 % at load > 0.75, see `afl_shim.c:336–339`). | ||
|
|
||
| 4. **`MapSizeEstimate` (`elf.py:1670`)**: add `ngram_k: int` field alongside | ||
| `ctx_bits: int`. |
There was a problem hiding this comment.
issue (bug_risk): Adding ngram_k to the MapSizeEstimate NamedTuple requires updating every positional constructor, but the complete file-change list only calls out the field addition. The existing MapSizeEstimate(MAP_SIZE_DEFAULT, 0, "default", ctx_bits, False) and five-argument success constructor will raise TypeError once the tuple has six fields.
Triggers: When the planned MapSizeEstimate field is implemented without updating all constructors and tests.
Suggested fix: Update every MapSizeEstimate(...) construction and all positional consumers to carry ngram_k, preferably using named arguments for this metadata tuple.
| 1. **`detect_ngram_k(target: str) -> int`** (new, mirrors `detect_ctx_bits` | ||
| at `elf.py:1587`): scan the ELF symtab for `__afl_ngram_k_N`; return N. | ||
| Return 2 (current default) when the symbol is absent. |
There was a problem hiding this comment.
issue (bug_risk): The proposed detector returns a single unspecified N even though a shared object can contain multiple instrumented translation units and therefore multiple __afl_ngram_k_N symbols. Without the existing detector's explicit widest-value policy or a validation error for disagreement, map sizing can be based on a smaller k than one of the loaded TUs uses.
Triggers: When a target or shared library contains TUs built with different n-gram settings.
Suggested fix: Scan all marker symbols, reject conflicting values or deliberately select the maximum, and document that policy as detect_ctx_bits does.
| 1. **`detect_ngram_k(target: str) -> int`** (new, mirrors `detect_ctx_bits` | |
| at `elf.py:1587`): scan the ELF symtab for `__afl_ngram_k_N`; return N. | |
| Return 2 (current default) when the symbol is absent. | |
| 1. **`detect_ngram_k(target: str) -> int`** (new, mirrors `detect_ctx_bits` | |
| at `elf.py:1587`): scan all ELF symtab markers matching `__afl_ngram_k_N`; | |
| if values disagree, select the maximum so the map is sized for the widest | |
| instrumented translation unit, matching `detect_ctx_bits`'s policy. Return 2 | |
| (current default) when the symbol is absent. |
| static uint32_t __afl_prev_locs[__AFL_NGRAM_K - 1]; | ||
| static uint8_t __afl_prev_idx = 0; |
There was a problem hiding this comment.
issue (bug_risk): The ring index is declared as uint8_t, so it cannot represent the full index range when configurable __AFL_NGRAM_K - 1 exceeds 255. For k > 256 the index wraps at 256 rather than at the ring length, causing out-of-range accesses or incorrect history selection.
Triggers: When a build sets __AFL_NGRAM_K above 256.
Suggested fix: Use an index type wide enough for the configured ring or enforce a compile-time maximum and static assertion for __AFL_NGRAM_K.
There was a problem hiding this comment.
🟡 Changes recommended
The new plan document contains internal inconsistencies/inaccurate references (notably around the k-gating/ABI rationale) that should be corrected before merging to avoid misleading future implementation work.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a design/implementation plan document for introducing configurable n-gram edge coverage to the fuzzer’s AFL shim and Python integration, focusing on hashing, reset semantics, sizing, and compatibility.
Changes:
- Add
docs/ngram_coverage_plan.mddescribing proposed AFL shim ring-buffer + n-gram hashing approach. - Document expected Python-side detection/sizing changes and a compatibility + test plan.
File summaries
| File | Description |
|---|---|
| docs/ngram_coverage_plan.md | New implementation plan for configurable n-gram edge coverage, including shim/Python touchpoints, compatibility notes, and test strategy. |
Review details
Suppressed comments (1)
docs/ngram_coverage_plan.md:193
- The compatibility table claims
__afl_prev_locis “scanned by fuzzer.py:308”, butsrc/fuzzer_tool/services/fuzzer.pyline ~308 defines_AFL_SYMS = ("__afl_area", "__afl_map_shm", "__sanitizer_cov")and does not reference__afl_prev_loc. Please correct/remove this reference (or point to the actual scanning site if it exists) so the ABI concern is accurately justified.
| **Corpus portability** | edge_ids change when k changes; existing `_seen_edge_ids`, `EdgeTracker`, `state.json` are incompatible | Add `ngram_k` to `state.json`; refuse resume if k mismatches |
| **`__afl_prev_loc` ABI** | Symbol is non-static (`afl_shim.c:283`), scanned by `fuzzer.py:308` | Keep symbol at k=2 default; the ring is introduced only at k > 2, emitting a new symbol `__afl_ngram_k_N` |
| **cmplog/perf shims** | Both use `-include afl_shim.c`; new statics duplicate into each TU | No change — `static` globals are already per-TU; ring follows the same pattern |
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // After | ||
| #ifndef __AFL_NGRAM_K | ||
| #define __AFL_NGRAM_K 2 /* default: retain current 2-node behaviour */ | ||
| #endif | ||
|
|
||
| #if __AFL_NGRAM_K > 1 | ||
| static uint32_t __afl_prev_locs[__AFL_NGRAM_K - 1]; | ||
| static uint8_t __afl_prev_idx = 0; | ||
| #else | ||
| uint32_t __afl_prev_loc = 0; /* k=1: unchanged ABI */ | ||
| #endif | ||
| ``` | ||
|
|
||
| - `__AFL_NGRAM_K = 2` (default) keeps the existing single-word layout so no | ||
| existing binaries need recompilation. | ||
| - The ring holds exactly k−1 entries; index wraps modulo k−1. |
| The ptrace path simulates `(rel ^ self.prev_location) % map_size`. | ||
| Extend with a ring: | ||
| ```python | ||
| # prev_locations: collections.deque(maxlen=k-1), initially all-zero | ||
| edge_id = rel | ||
| for p in self.prev_locations: | ||
| edge_id ^= p # simple XOR chain is fine for ptrace (no perf path) | ||
| bucket = edge_id % self.map_size | ||
| self.prev_locations.appendleft(rel >> 1) | ||
| ``` | ||
| Note: the ptrace path has no `caller_ctx` and never will; this is an | ||
| acknowledged coverage-mode gap (`ptrace_coverage.py:430` comment). |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Adds
docs/ngram_coverage_plan.md— the implementation plan produced by the prior research session analysingafl_shim.cand its Python integration surface.Covers:
__afl_prev_loc)elf.py,ptrace_coverage.py,edge_tracker.py)Summary by Sourcery
Document the implementation plan for adding configurable n-gram edge coverage while preserving compatibility and reliable reset behavior.
Enhancements:
Documentation:
Tests: