Validate instrumented MSan runtime together with the CI fixes - #573
Open
Krilliac wants to merge 6 commits into
Open
Validate instrumented MSan runtime together with the CI fixes#573Krilliac wants to merge 6 commits into
Krilliac wants to merge 6 commits into
Conversation
build-linux-msan compiled the engine with -fsanitize=memory but linked the distribution's libc++, which is not instrumented, so every std::string written inside libc++.so.1 read as uninitialized: 732 reports in the first 25 tests, a 16 MiB sanitizer log that hit the wrapper's file-size limit, SparkTests killed by SIGXFSZ with no completion evidence, and a lane red on Working for months at 18-26 runner minutes per push. The job now fetches the LLVM 18.1.3 source release matching the runner's clang-18 (size, SHA-256 and SHA-512 pinned), builds libc++/libc++abi with LLVM_USE_SANITIZER=MemoryWithOrigins from the checked-in recipe .github/msan/libcxx-runtime.cmake, caches the install prefix through split actions/cache restore/save keyed by recipe hash, LLVM tag and compiler package version, and verifies the prefix on every run before it is used or saved. SparkTests is compiled with -nostdinc++/-isystem and linked with -L/-rpath/-lc++abi against that prefix; libc++-dev is no longer installed so a lost flag fails the link. A verification step walks every ELF under build/bin and build/lib and checks which libc++ it resolves, its RUNPATH, its direct libc++abi dependency and that it is MSan-linked (executables define the runtime symbols, shared objects import them). Tests/TestMSanCanary.cpp proves at run time that this translation unit is instrumented and that libc++ stores update shadow memory. The shared sanitizer wrapper ignores SIGXFSZ so an over-cap write truncates the log instead of killing the suite (harness fixture added); the lane runs with halt_on_error=1 so each run yields one deduplicated signature; a new step labels pre-test infrastructure failures so they are never mistaken for findings; ENABLE_VULKAN=OFF removes the last uninstrumented library the tests could reach; the ignorelist loses dead patterns and the /usr/* entry. Parity expects the extra configure record, inventory regenerated last, readiness status, the reproducible recipe and both copies of the build/CI and validation skills updated. The lane stays continue-on-error and outside the required gate until a run classifies clean. Static and repository validators only (workflow shape, exact gate, privilege boundaries, sanitizer evidence and pipeline harness, parity, inventory, docs contract, wiki, site data, registration): all pass. The SIGXFSZ fixture and the canary's RED side need Linux; the first CI run of this branch is their proof. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rumentation as skipped
…, and canary fixes
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. |
Contributor
⏳ CodeQL Report PendingThe pull-request head changed. Results from earlier commits do not apply while the new head is being scanned. Current PR head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c332e9c5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
The latest #569 MSan run still uses the system libc++ and fails with uninitialized std::string data before completing. The earlier instrumented-libc++ run in #568 verified its runtime linkage and then exposed the ReplicatedEntity initialization bug fixed in #569.
This integration preserves #568 and #570 history and combines them with #569: instrumented libc++/libc++abi, bounded sanitizer failure reporting, initialized replication ownership, logger/world isolation, and honest canary skips. Generated merge conflicts were regenerated. Cache save additionally requires successful cache restore to avoid masking earlier infrastructure failures.
Validation: 129 sanitizer wrapper tests, 9 sanitizer evidence tests, 47 workflow tests, source census, full documentation generation, and independent review passed. Parity suite passed all checks except its nonmutation snapshot raced concurrent documentation generation; that single check passed when rerun after generation stopped. Local full SparkTests build passed and the 58 affected logger/wiring/network tests passed with shuffle123. Full local suite cannot run here because Unix sockets return EPERM.
Hosted combined MSan validation and canary negative proof remain pending. MSan remains advisory and no release gate is weakened. This is an integration candidate stacked on #569, not release certification.