-
Notifications
You must be signed in to change notification settings - Fork 835
Small clean-ups (including fix for UB) #616
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
Conversation
src/slice_transform.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning 0xff here was somewhat questionable (as a bool is expected in C++ land), but not a problem as far as I can tell after reading the current implementation in RocksDB.
src/db.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was undefined behavior because the temporary c_int would be out of scope before the pointer is used in the called function.
Aditionally, there would be an out-of-bounds read if cfs_v.len() > 1, because the implementation expects a pointer to cfs_v.len() consecutive entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something, but where can out-of-bound read occur? cfnames? But it is an array, so everything should be OK. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the ttls array. Previously we were passing a pointer to a single integer value (&(ttl.as_secs() as c_int) as *const _), but cfs_v.len() integer values are expected (https://github.com/facebook/rocksdb/blob/f20b674796ffd7ca32471705876fc651b8e246db/db/c.cc#L827-L828), one for each column family.
|
Oh, looks like the pointers to temporaries are fine, actually:
https://doc.rust-lang.org/stable/reference/expressions.html?highlight=Tempo#temporaries That leaves only the out of bounds read as UB, with all other changes being cosmetic. |
src/db.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something, but where can out-of-bound read occur? cfnames? But it is an array, so everything should be OK. Am I missing something?
|
Rebased since #627 got merged first. |
|
Prepared rustsec advisory rustsec/advisory-db#1237. The changes in this PR are related, because they all adressed clippy lints, but I can split it, if that would be useful. The commits can also be viewed separately. |
Most of these are trivial. I'll highlight some important bits with inline comments.