Repository navigation
Keep timer time monotonic and qualify immutable coverage campaigns - #10
Merged
Merged
Conversation
Reviewer's GuideThe PR makes logical timer time monotonic across fallback-to-PIT transition using a scoped, saturating clock; adds immutable, evidence-preserving coverage campaigns with a new two-owner handle-table target; and integrates the corresponding CI checks, harnesses, documentation, and roadmap qualification updates. Sequence diagram for monotonic timer transitionsequenceDiagram
participant Coordinator
participant Interrupts
participant Clock
participant PIT
participant Scheduler
Coordinator->>Interrupts: poll_fallback_tick()
Interrupts->>Clock: poll_fallback()
Clock-->>Interrupts: logical ticks
Interrupts-->>Coordinator: monotonic ticks
PIT->>Interrupts: genos_irq0_rust(frame)
Interrupts->>Interrupts: TICKS.fetch_add()
Interrupts->>Clock: hardware_tick()
Clock-->>Interrupts: advanced logical ticks
Interrupts->>Scheduler: timer_preempt(frame)
Interrupts-->>PIT: complete IRQ handling
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tools/test_coverage_fuzz.py" line_range="120-121" />
<code_context>
compiler = run(["clang++", "--version"]).splitlines()[0]
- fingerprint = hashlib.sha256((json.dumps(lock, sort_keys=True) + compiler).encode()).hexdigest()
+ recipe = ["clang++", "-std=c++17", "-O2", "-g", "-fno-omit-frame-pointer"]
+ fingerprint = hashlib.sha256(json.dumps({"lock": lock, "compiler": compiler,
+ "recipe": recipe, "archiver": ["ar", "rcs"]}, sort_keys=True).encode()).hexdigest()
marker = runtime / "archive.json"
if archive.is_file() and marker.is_file():
</code_context>
<issue_to_address>
**Runtime archive reused across architectures**
When a coverage cache is reused on a different host architecture whose Clang reports the same first version line, `build_runtime` can accept an archive compiled for a different host because its cache fingerprint omits the compiler target, while the C++ objects are compiled for Clang’s default target. The later fuzz executable then links against the wrong-architecture archive and the campaign fails.
Include the actual Clang target triple and Rust host target in the archive fingerprint so a host change forces a rebuild.
</issue_to_address>
### Comment 2
<location path="tools/test_coverage_fuzz.py" line_range="68-71" />
<code_context>
-def verified_source(*, offline: bool) -> tuple[Path, dict]:
- lock = json.loads(LOCK.read_text())
+def check_source_files(source: Path, lock: dict) -> None:
+ expected = {entry["name"] for entry in lock["files"]}
+ if any(path.name not in expected or not path.is_file() or path.is_symlink()
+ for path in source.iterdir()):
+ raise ValueError("unexpected or symlinked file in pinned runtime source cache")
+
+
</code_context>
<issue_to_address>
**Updated locks strand campaigns**
When a later campaign changes the locked runtime file set after an earlier campaign populated `build/coverage-fuzz/source`, `verified_source` calls `check_source_files` before reconciling the shared cache, so a leftover file causes every campaign using the updated lock to raise `ValueError`, including online campaigns. The cache must be manually cleaned.
Reconcile or refresh the cache against the new lock before checking its files.
</issue_to_address>
### Comment 3
<location path="tools/test_coverage_fuzz.py" line_range="138" />
<code_context>
archive.unlink(missing_ok=True)
- run(["ar", "rcs", str(archive), *objects], log=runtime / "archive.log")
+ run(["ar", "rcs", str(archive), *objects], log=evidence / "runtime/archive.log")
marker.write_text(json.dumps({"lock": fingerprint,
"archive_sha256": hashlib.sha256(archive.read_bytes()).hexdigest()},
indent=2) + "\n")
</code_context>
<issue_to_address>
**Interrupted marker write blocks campaigns**
When a campaign is stopped while writing `build/coverage-fuzz/runtime/archive.json`, `build_runtime` loads the cache marker with `json.loads` and propagates a decoding error instead of treating the cache as invalid, so a process interruption during `marker.write_text` leaves subsequent campaigns unable to rebuild the runtime until the marker is removed.
Write the marker to a temporary file and atomically replace it, and treat an invalid marker as a cache miss.
</issue_to_address>
### Comment 4
<location path="tools/test_coverage_fuzz.py" line_range="119-122" />
<code_context>
archive = runtime / "libFuzzer.a"
compiler = run(["clang++", "--version"]).splitlines()[0]
- fingerprint = hashlib.sha256((json.dumps(lock, sort_keys=True) + compiler).encode()).hexdigest()
+ recipe = ["clang++", "-std=c++17", "-O2", "-g", "-fno-omit-frame-pointer"]
+ fingerprint = hashlib.sha256(json.dumps({"lock": lock, "compiler": compiler,
+ "recipe": recipe, "archiver": ["ar", "rcs"]}, sort_keys=True).encode()).hexdigest()
marker = runtime / "archive.json"
if archive.is_file() and marker.is_file():
record = json.loads(marker.read_text())
</code_context>
<issue_to_address>
**Runtime cache misstates compiler provenance**
When a cached runtime was built by a different clang or archiver whose identity is not distinguished by the fingerprint, `build_runtime` fingerprints only the first line of `clang++ --version` and the literal `ar` command, so toolchains with the same displayed version but different builds can reuse an archive produced by another toolchain while the manifest records the current toolchain as its provenance.
Bind the cache key to complete compiler and archiver identities (or executable hashes) and record those identities with the cached archive.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: tools/test_coverage_fuzz.py:121, tools/test_coverage_fuzz.py:71, tools/test_coverage_fuzz.py:138, tools/test_coverage_fuzz.py:122
Comment on lines
+120
to
+121
| fingerprint = hashlib.sha256(json.dumps({"lock": lock, "compiler": compiler, | ||
| "recipe": recipe, "archiver": ["ar", "rcs"]}, sort_keys=True).encode()).hexdigest() |
There was a problem hiding this comment.
🟡 Medium · Runtime archive reused across architectures
When a coverage cache is reused on a different host architecture whose Clang reports the same first version line, build_runtime can accept an archive compiled for a different host because its cache fingerprint omits the compiler target, while the C++ objects are compiled for Clang’s default target. The later fuzz executable then links against the wrong-architecture archive and the campaign fails.
Include the actual Clang target triple and Rust host target in the archive fingerprint so a host change forces a rebuild.
Prompt for AI agents
In `tools/test_coverage_fuzz.py` at lines 120-121:
**Runtime archive reused across architectures**
When a coverage cache is reused on a different host architecture whose Clang reports the same first version line, `build_runtime` can accept an archive compiled for a different host because its cache fingerprint omits the compiler target, while the C++ objects are compiled for Clang’s default target. The later fuzz executable then links against the wrong-architecture archive and the campaign fails.
Include the actual Clang target triple and Rust host target in the archive fingerprint so a host change forces a rebuild.
Comment on lines
+68
to
+71
| expected = {entry["name"] for entry in lock["files"]} | ||
| if any(path.name not in expected or not path.is_file() or path.is_symlink() | ||
| for path in source.iterdir()): | ||
| raise ValueError("unexpected or symlinked file in pinned runtime source cache") |
There was a problem hiding this comment.
🟡 Medium · Updated locks strand campaigns
When a later campaign changes the locked runtime file set after an earlier campaign populated build/coverage-fuzz/source, verified_source calls check_source_files before reconciling the shared cache, so a leftover file causes every campaign using the updated lock to raise ValueError, including online campaigns. The cache must be manually cleaned.
Reconcile or refresh the cache against the new lock before checking its files.
Prompt for AI agents
In `tools/test_coverage_fuzz.py` at lines 68-71:
**Updated locks strand campaigns**
When a later campaign changes the locked runtime file set after an earlier campaign populated `build/coverage-fuzz/source`, `verified_source` calls `check_source_files` before reconciling the shared cache, so a leftover file causes every campaign using the updated lock to raise `ValueError`, including online campaigns. The cache must be manually cleaned.
Reconcile or refresh the cache against the new lock before checking its files.| archive.unlink(missing_ok=True) | ||
| run(["ar", "rcs", str(archive), *objects], log=runtime / "archive.log") | ||
| run(["ar", "rcs", str(archive), *objects], log=evidence / "runtime/archive.log") | ||
| marker.write_text(json.dumps({"lock": fingerprint, |
There was a problem hiding this comment.
🟡 Medium · Interrupted marker write blocks campaigns
When a campaign is stopped while writing build/coverage-fuzz/runtime/archive.json, build_runtime loads the cache marker with json.loads and propagates a decoding error instead of treating the cache as invalid, so a process interruption during marker.write_text leaves subsequent campaigns unable to rebuild the runtime until the marker is removed.
Write the marker to a temporary file and atomically replace it, and treat an invalid marker as a cache miss.
Prompt for AI agents
In `tools/test_coverage_fuzz.py` at line 138:
**Interrupted marker write blocks campaigns**
When a campaign is stopped while writing `build/coverage-fuzz/runtime/archive.json`, `build_runtime` loads the cache marker with `json.loads` and propagates a decoding error instead of treating the cache as invalid, so a process interruption during `marker.write_text` leaves subsequent campaigns unable to rebuild the runtime until the marker is removed.
Write the marker to a temporary file and atomically replace it, and treat an invalid marker as a cache miss.
Comment on lines
+119
to
122
| recipe = ["clang++", "-std=c++17", "-O2", "-g", "-fno-omit-frame-pointer"] | ||
| fingerprint = hashlib.sha256(json.dumps({"lock": lock, "compiler": compiler, | ||
| "recipe": recipe, "archiver": ["ar", "rcs"]}, sort_keys=True).encode()).hexdigest() | ||
| marker = runtime / "archive.json" |
There was a problem hiding this comment.
🟡 Medium · Runtime cache misstates compiler provenance
When a cached runtime was built by a different clang or archiver whose identity is not distinguished by the fingerprint, build_runtime fingerprints only the first line of clang++ --version and the literal ar command, so toolchains with the same displayed version but different builds can reuse an archive produced by another toolchain while the manifest records the current toolchain as its provenance.
Bind the cache key to complete compiler and archiver identities (or executable hashes) and record those identities with the cached archive.
Prompt for AI agents
In `tools/test_coverage_fuzz.py` at lines 119-122:
**Runtime cache misstates compiler provenance**
When a cached runtime was built by a different clang or archiver whose identity is not distinguished by the fingerprint, `build_runtime` fingerprints only the first line of `clang++ --version` and the literal `ar` command, so toolchains with the same displayed version but different builds can reuse an archive produced by another toolchain while the manifest records the current toolchain as its provenance.
Bind the cache key to complete compiler and archiver identities (or executable hashes) and record those identities with the cached archive.
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
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.
The logical clock could move backward when fallback polling had advanced time before the first PIT interrupt: a retained VM fixture observes 3 → 1 on prior main. One scoped clock now preserves fallback time and advances it on hardware delivery. IRQ-masked, nonwaiting access retires its borrow before scheduling and preserves caller IF. Raw interrupt counts remain separate diagnostics.
Coverage campaigns now compile only SHA-256-locked runtime sources, reject unexpected cached files, build from a retained clean source archive, isolate runtime/executable/corpus artifacts per run, and preserve partial logs and failure manifests. A new bounded handle-table target compares both owners with independent models across type/rights checks, exhaustion, revocation and clear. Fresh CI coverage runs require nonzero coverage, exact run counts and new corpus discovery.
Validation on clean 0da9e7c: five pure clock tests; retained old-main VM regression and corrected real-PIT proof (3 → 4 → 6); 10,000 handle cases with 272 new corpus entries; workspace and seven parser harness tests; 173 Python tests; strict kernel/harness lint; reviewed unsafe inventory remains 407 sites with no new unsafe construct. Pinned strict-provenance Miri passes all 136 selected tests plus native allocator stress. All 83 required remote checks pass on implementation head 0da9e7c; a live policy/success audit confirms 83 executed and zero failed. The new timer and two coverage contexts are enforced, with every unrelated protection field preserved. Publication head 8334b5f contains documentation only and requires its own checks before integration.
The PR9 merge is recorded as F0.1 completion, bringing the unchanged console checklist denominator to 52/100. F1 boot/CPU obligations, remaining F4/F5 ownership, other F6 targets and sustained qualification remain open. GenOS remains Experimental.