fix(storage): Avoid use of uninitialised memory#1224
fix(storage): Avoid use of uninitialised memory#1224jayvdb wants to merge 1 commit intofoyer-rs:mainfrom
Conversation
jayvdb
changed the title
jayvdb
force-pushed
the
storage-buffer-ub
branch
2 times, most recently
from
f9ec4ba to
b24e0a0
Compare
Signed-off-by: John Vandenberg <jayvdb@gmail.com>
jayvdb
force-pushed
the
storage-buffer-ub
branch
from
b24e0a0 to
f08b96d
Compare
|
@MrCroxx , can you approve these workflows? |
MrCroxx
left a comment
MrCroxx
left a comment
There was a problem hiding this comment.
Generally LGTM.
Thank you John. Good catch!
I've reviewed #1223 and this PR. IIUC, the original codes will not actually cause UB, but I'm happy to fix the miri failed case.
Since this refactor will break existing disk cache data, I'll mark it and delay its release to next major release.
| impl BlobIndexReader { | ||
| pub fn read(buf: &[u8]) -> Option<Vec<BlobEntryIndex>> { | ||
| let checksum = Checksummer::checksum64(&buf[BlobIndex::CHECKSUM_BYTES..]); | ||
| let count = |
There was a problem hiding this comment.
We need to check if the loaded count is within the maximum count limiation. Or L142 will overflow read buf.
There was a problem hiding this comment.
Note I have two more miri reports of UB, and a few other oddities to investigate, but I am close to putting up a PR for miri CI. As these fixes will be delayed, I will ignore the relevant tests under miri, so we can get the miri testing in early.
There was a problem hiding this comment.
I think foyer also has needs to introduce miri test. Let me add it later.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
What's changed and what's your intention?
Avoid use of uninitialised memory in storage, by being careful when creating the checksum, and implementing a custom
PartialEq.It is possible that none of these problems could occur via typical use of
foyer, and there are no other crates.io published uses of foyer-storage according to https://crates.io/crates/foyer-storage/reverse_dependencies .Checklist
cargo x(orcargo x --fastinstead if the old tests are not modified) in my local environment.Related issues or PRs (optional)
Fixes #1223