-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
Fuzz for compatibility with bincode v1 #498
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #498 +/- ##
=======================================
Coverage 69.09% 69.09%
=======================================
Files 44 44
Lines 3058 3058
=======================================
Hits 2113 2113
Misses 945 945 Continue to review full report at Codecov.
|
fuzz/fuzz_targets/roundtrip.rs
Outdated
@@ -40,12 +38,22 @@ enum AllTypes { | |||
} | |||
|
|||
fuzz_target!(|data: &[u8]| { | |||
let config = bincode::config::standard().with_limit::<1024>(); | |||
let config = bincode::config::legacy().with_limit::<1024>(); | |||
let configv1 = bincodev1::DefaultOptions::new().with_limit(1024).allow_trailing_bytes(); |
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.
This will use varint encoding, the legacy()
will use fixint. The true legacy config from bincode 1 is bincodev1::Config
Okay, this seems like it's correct? Unless I'm missing something else with the config (endian?). Example output is
|
That specific issue should now be fixed in trunk. |
I'm getting Bytes: [12, 0, 0, 0, 0, 100, 0, 0, 0, 100]
Bincode V1: Ok(Ipv4Addr(0.100.0.0))
Bincode V2: Err(UnexpectedEnd) now. |
Fixed some compatibility issues in #503 including |
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’ll look at a fix for the ip addr bug. But this PR looks good. We can run the fuzzer for a while.
This finds this input:
Which fails because of
Not sure if this is a bug, but making this as a draft to just make a note of it. I might want to move the compatibility check to a new fuzzer before this is merged.