fix(c): invalidate cache for .include and .incbin directives - #2795
fix(c): invalidate cache for .include and .incbin directives#2795bleggett wants to merge 2 commits into
Conversation
bleggett
changed the title
|
This also should fix #2700 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
==========================================
+ Coverage 72.90% 73.25% +0.35%
==========================================
Files 72 72
Lines 37275 37819 +544
==========================================
+ Hits 27176 27705 +529
- Misses 10099 10114 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @bleggett , Looks like you have conflicts. Please, rebase your changes |
9e62ec3 to
0bb81ee
Compare
Rebased, thanks! |
|
Thanks for the fixes, @bleggett Is it ok for you, @sylvestre ? |
| fn find_asm_dependencies(bytes: &[u8]) -> Vec<AsmDependency> { | ||
| let mut dependencies = Vec::new(); | ||
| let mut offset = 0; | ||
| while offset < bytes.len() { |
There was a problem hiding this comment.
.include/.incbin matching seems to be too loose (c.rs:870) no line-start requirement, no ?
| } else if skipped_whitespace { | ||
| // gas requires a quoted file name. Not finding one means the operand was | ||
| // produced some other way, so we cannot tell what this depends on. | ||
| return Some((FileOperand::Unparsable, 0)); |
There was a problem hiding this comment.
Since the caller matches .include/.incbin anywhere in the buffer with no line-start check, this bails out to Unparsable on ordinary C like opts.include = 1; or a ".include %s\n" string literal, making the whole TU non-cacheable
the match needs a statement/line-boundary check before reaching here.
4 participants
This fixes a really nasty bug with sccache for C code where
.incbinand.includedirectives weren't fully checked for cache consistency before being returned in the build cache.This led to a case where a Linux kernel build that used
sccacheended up with kmods signed with the wrong signing key, because the kernel'scerts/system_certificates.Sembeds the cert via assembler .incbin, andsccachewould mistakenly return a stale signing key from a previous build 🙃Ex:
certs/system_certificates.S embeds the cert via assembler .incbin:
The sizes come from label arithmetic, not from the source text. So the preprocessed text of that file is byte-identical on every build regardless of which key is in signing_key.x509.
sccachehashes the preprocessed output, gets a hit, and hands back an object containing the old certificate.ccacherefuses to cache any translation unit containing .incbin precisely because of this (ccache/ccache#136, documented in the ccache manual https://ccache.dev/manual/4.13.6.html).sccachehas the detector but only wires it into preprocessor cache mode, which isn't enough.Tested with a kernel build using this SHA, mismatch issue goes away. Also added some more unit tests.
I also did a pass to see if there are other similarly-shaped problems where the cache hashing/keying might be broken and return the wrong thing and found a few more minor ones, mostly around plugins/specs/struct layout randomization, those are fixed in the second commit.