Add fuzz test to handle length math issues with Messages and Headers #1273
Conversation
|
It was really nice for Devin to handle all the cfg attributes here |
fuzz/fuzz_targets/header_length.rs
Outdated
|
|
||
| fuzz_target!(|data: &[u8]| { | ||
| if data.len() < Header::LENGTH { | ||
| return; | ||
| } | ||
| if let Ok(header) = Header::from_slice(data) { | ||
| if header.length < 0 { |
There was a problem hiding this comment.
This was particularly bad. Ok, the length is negative because it's random. This wasn't testing any actual driver code 😂
| @@ -158,12 +158,14 @@ fn num_decimal_digits(mut n: usize) -> usize { | |||
|
|
|||
| /// Read a document's raw BSON bytes from the provided reader. | |||
| pub(crate) fn read_document_bytes<R: Read>(mut reader: R) -> Result<Vec<u8>> { | |||
There was a problem hiding this comment.
These are the actual bugs found by fuzzing
There was a problem hiding this comment.
we should be getting a warning when we use risky as <numtype> conversions as of #1045; I wonder why these ones weren't caught
| @@ -80,6 +80,7 @@ impl Action for ParseConnectionString { | |||
| "srvMaxHosts and loadBalanced=true cannot both be present", | |||
| )); | |||
| } | |||
There was a problem hiding this comment.
I did a quick pass for any other as usize issues and added comments after I was certain they were safe
| @@ -1,7 +1,13 @@ | |||
| #[cfg(test)] | |||
| pub(crate) mod test; | |||
|
|
|||
| #[cfg(feature = "fuzzing")] | |||
There was a problem hiding this comment.
allow missing just so we don't see warning when we run the fuzz test
…se of patching annoyances
|
@isabelatkinson @abr-egn I notice that Devin added a bunch of Arbitrary implementations that are not actually used. Question is, do we remove them, or do we actually use them? |
| use mongodb::cmap::conn::wire::{ | ||
| header::{Header, OpCode}, | ||
| message::Message, | ||
| }; |
There was a problem hiding this comment.
I'd prefer not to add any of this to the public API, even if it's behind a feature flag. Can we move the contents of fuzz to within src/test and put any fuzzing additions to the message/header code behind cfg(test)?
There was a problem hiding this comment.
Unfortunately, this isn't possible because of the way cargo fuzz works (I wish it were, it's an issue with how it uses LLVM's fuzzing support, I think). However, you did bring to my attention that I was accidentally leaking some of this publicly when not fuzzing, so I have fixed that up.
| @@ -158,12 +158,14 @@ fn num_decimal_digits(mut n: usize) -> usize { | |||
|
|
|||
| /// Read a document's raw BSON bytes from the provided reader. | |||
| pub(crate) fn read_document_bytes<R: Read>(mut reader: R) -> Result<Vec<u8>> { | |||
There was a problem hiding this comment.
we should be getting a warning when we use risky as <numtype> conversions as of #1045; I wonder why these ones weren't caught
|
@isabelatkinson @abr-egn maybe our best bet here is to not check in the fuzzer, but just commit the bug fix? |
@pmeredit That would be my preference; maybe we can leave the changes on a branch in case we want to revisit? |
SGTM! I'll just make a new PR and leave this branch/push this branch to origin (if I can) |
This was mostly generated by Devin. The initial fuzz test was bad, but all the infrastructure for fuzzing is good. The test I modified (which was 4 loc?) did uncover an actual bug still present in the driver, which is exciting, and I am now confident that all our math ops are good.