-
Notifications
You must be signed in to change notification settings - Fork 29
Add fuzzers for RRG actions #182
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
base: master
Are you sure you want to change the base?
Conversation
| **/target/ | ||
| **/Cargo.lock |
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.
Let's not do that, I could easily see a Rust submodule named target (e.g. for different platforms) and then spending hours trying to debug missing files on CI.
Let's just be explicit about the files we want to ignore:
/target/
/Cargo.lock
/fuzz/target/
/fuzz/Cargo.lock
| ] | ||
|
|
||
| [workspace] | ||
| members =["."] |
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.
Nit: missing space.
| path = "utils.rs" | ||
|
|
||
| [dependencies] | ||
| libfuzzer-sys="0.4" |
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.
Nit: missing space.
|
|
||
| [lib] | ||
| name = "fuzz_utils" | ||
| path = "utils.rs" |
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.
Any reason for not using fuzz_utils.rs for uniformity?
| log = {version = "0.4.22", features = ["std"]} | ||
| rrg-proto = {path = "../crates/rrg-proto"} |
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.
Nit: inconsistent brace spacing (line above is style the rest of the project follows).
|
|
||
| let full_path = root_path.join(&safe_path); | ||
|
|
||
| // Ensure Parent Directories Exist |
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.
Not a very useful comment.
| return None; | ||
| } | ||
|
|
||
| let mut file = unsafe { File::from_raw_fd(fd) }; |
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.
https://doc.rust-lang.org/book/ch20-01-unsafe-rust.html
Whenever we write an unsafe function, it is idiomatic to write a comment
starting with SAFETY and explaining what the caller needs to do to call
the function safely. Likewise, whenever we perform an unsafe operation, it
is idiomatic to write a comment starting with SAFETY to explain how the
safety rules are upheld.
| if file.write_all(content).is_err() { | ||
| return None; | ||
| } | ||
|
|
||
| std::mem::forget(file); |
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.
Maybe instead of wrapping, calling write_all and forget we can just do libc::write instead?
|
|
||
| impl Drop for MemFd { | ||
| fn drop(&mut self) { | ||
| unsafe { libc::close(self.fd) }; |
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.
Similar comment about SAFETY comment.
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.
Hmm, I'm not sure how this file is used here. CTRL + F for disk_images_corpus yields no results.
No description provided.