Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit qbsdiff #55

Closed
nbraud opened this issue Nov 25, 2019 · 6 comments
Closed

Audit qbsdiff #55

nbraud opened this issue Nov 25, 2019 · 6 comments

Comments

@nbraud
Copy link

nbraud commented Nov 25, 2019

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);
}
@Shnatsel
Copy link
Member

I've already requested a lint for this, but perhaps my request was too broad. You can follow up on it here: rust-lang/rust-clippy#4483

@Shnatsel
Copy link
Member

I see the maintainer has already replied and is supportive of your PR. They have left some comments that I fully support; specifically, it might be possible to get the zero-initialization for free.

Other than that, looks very good to me and hopefully will be merged as soon as the comments are addressed. Thanks for doing this!

@danielhenrymantilla
Copy link

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

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

Just a nitpick: such code is not UB per se, since the following code is sound:

let mut v = Vec::with_capacity(s);
unsafe {
    v.set_length(s);
} // <- Safety invariant broken but never witnessed
unsafe {
    v.set_length(0);
}

UB happens as soon as you interact with like 90% of Vec's API, which relies on that safety invariant. The worst offender being Deref{,Mut}. So, regarding the lint, (which would of course be a great addition!), the phrasing "breaks a safety invariant" ought to be preferred to UB.

@nbraud
Copy link
Author

nbraud commented Nov 26, 2019

@danielhenrymantilla You are entirely correct; I guess that's what I get for writing issues in the middle of the night :)

@nbraud
Copy link
Author

nbraud commented Nov 26, 2019

I've already requested a lint for this, but perhaps my request was too broad. You can follow up on it here: rust-lang/rust-clippy#4483

Thanks! I think I read it and missed that it covered my case already :)

@nbraud
Copy link
Author

nbraud commented Nov 26, 2019

Fixes merged upstream, no unsafe left, clippy lint already requested.

@nbraud nbraud closed this as completed Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants