Repository navigation
Check every public routine for all inputs with no_panic - #15
Merged
Merged
Conversation
The no_panic driver called each routine with particular values. After inlining, the compiler specialized each routine to those values, so a panic that depends on the input could be optimized away and missed: a panic in write_f32_le on negative values passed the check, because the driver only passed non-negative floats. Take the address of each routine instead. That forces a standalone copy that has to handle any arguments, and that copy is what the linker checks. The panic above is now detected. Replace the text-matching coverage test with one that parses the source with syn, so it is not affected by formatting or attribute order and matches routines by type.
no_panic relies on a linker trick. If a toolchain change stopped it from detecting panics, every check would pass silently. The canary is a function that can panic; CI requires its link to fail with the expected no_panic error.
The scan only read the top level of src/, so a public method in a nested module directory (e.g. src/foo/mod.rs) escaped the check.
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.
Problem
The no_panic check from #14 could miss panics that depend on the input. The driver called each routine with particular values, and after inlining, the compiler specialized each routine to those values. To confirm it, I added
if value < 0.0 { panic!() }towrite_f32_le, and the check still passed, because the driver only passed non-negative floats.The README on
mainalso says the check "covers every input", which wasn't true.Fix
tests/no_panic.rsnow takes the address of every public routine (black_box::<fn(_) -> _>(ReadCursor::read_u8)) instead of calling it. That forces a standalone copy that has to handle any arguments, and that copy is what the linker checks. The negative-float panic is now detected.transactionandat_posare checked with a callback that can't panic, since they run the caller's callback.tests/no_panic_coverage.rsparsessrc/withsyninstead of matching text. Every public method must have the attribute and appear in the driver, matched by type. It's deliberately simple: it guards against honest mistakes in our own code, not adversarial input.read_arraysizes, and build configuration.Verification
synchecker: