Audit qbsdiff · Issue #55 · rust-secure-code/safety-dance (original) (raw)

qbsdiff is a binary diff/patch library and tool that are compatible with bsdiff 4.0.

Its use of unsafe is for creating (or extending) Vec<u8>s with uninitialized content; this turns out to likely be undefined behaviour.

Sent fixes as hucsmn/qbsdiff#3 :

• In one case (qbsdiff), the unsafe vector was only used locally, and could be easily replaced with safe code without extra overhead (no extra allocations or writes); there were 2 extra benefits:
  • the safe version clears the vector after use, so it's obvious no data can leak when reusing it;
  • might be faster and more readable as we eliminated complex iterator chains.
• In the second case (qbspatch), the unsafe vectors were shared across many functions (through a Context struct), and it was easier to replace the unsafe code with safe versions that initialize the allocated space to 0, though I will try to remove that overhead.

I believe this is a generalisable pattern: it isn't the first time I run into uses of unsafe to create known-length byte arrays whose contents are uninitialized. They can usually be replaced with:

• construction with Vec::with_capacity;
• writing to the vector with Vec::extend rather than using mutable slices;
• using Vec::clear to ensure data doesn't leak when reusing that buffer.

Would it be good idea to request a clippy lint for such code, given that many (wrongly) believe it to be UB-free:

let v = Vec::with_capacity(s) unsafe { v.set_length(s); }