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

Minify crashes while fuzz works #28

Open
MTRNord opened this issue Feb 13, 2022 · 6 comments
Open

Minify crashes while fuzz works #28

MTRNord opened this issue Feb 13, 2022 · 6 comments

Comments

@MTRNord
Copy link

MTRNord commented Feb 13, 2022

Hi I am trying to use my fuzz harness and had a crash which I would like to minify.

Code is at https://gitlab.com/MTRNord/conduit-fuzz-harness/-/tree/03cade7d643a6cf28ba93c966ae77f5cec82c1a8 (Note there is a file reference in the cargo toml. the rev and url in there however should work though)

The setup is a little funky tbh as I had link failures when trying to do the verification tests so the cfg flags are all over the place. It however compiles just fine (expect ~10-20m compile times due to the upstream package conduit that I am fuzzing. Its annoying but it is due to codegen sadly.)

Running it with cargo fuzzcheck tests::register --command minify --input-file "fuzz/tests::register/artifacts/35056055153a44e5.json" fails with the following for me:

➜  conduit-fuzz-harness git:(main) ✗ cargo fuzzcheck tests::register --command minify --input-file "fuzz/tests::register/artifacts/35056055153a44e5.json"
launch with config: "--command read --input-file fuzz/tests::register/artifacts/35056055153a44e5.json --no-in-corpus  --no-out-corpus  --artifacts fuzz/tests::register/artifacts/35056055153a44e5.minified  --no-stats  --max-cplx 4096 --stop-after-duration 18446744073709551615 --stop-after-iterations 18446744073709551615 --stop-after-first-failure "
warning: `-Z instrument-coverage` is deprecated; use `-C instrument-coverage`

warning: field is never read: `exp`
  --> /opt/dev_env/conduit/src/client_server/session.rs:19:5
   |
19 |     exp: usize,
   |     ^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default
note: `Claims` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
  --> /opt/dev_env/conduit/src/client_server/session.rs:16:10
   |
16 | #[derive(Debug, Deserialize)]
   |          ^^^^^
   = note: this warning originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: field is never read: `statement_ref`
  --> /opt/dev_env/conduit/src/database/abstraction/sqlite.rs:22:5
   |
22 |     pub statement_ref: NonAliasingBox<rusqlite::Statement<'a>>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: `conduit` (lib) generated 3 warnings
   Compiling conduit-fuzz-harness v0.1.0 (/opt/dev_env/conduit-fuzz-harness)
warning: `conduit-fuzz-harness` (lib test) generated 1 warning (1 duplicate)
    Finished release [optimized + debuginfo] target(s) in 12m 35s
     Running unittests (target/fuzzcheck/x86_64-unknown-linux-gnu/release/deps/conduit_fuzz_harness-05780ade5151ccef)

running 1 test
test tests::register ... thread 'main' panicked at 'assertion failed: !o.status.success()', /root/.cargo/registry/src/github.com-1ecc6299db9ec823/cargo-fuzzcheck-0.10.0/src/lib.rs:175:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The json file I input is:

{"kind":"user","auth":{"session":"","type":"m.login.dummy"},"device_id":"�\u001b���I%b","inhibit_login":false,"initial_device_display_name":"��\u0000���Ŧ.�Oo�JyE1�","password":"��\u000f��","username":"��� }"}

I am not sure why this may be happening. any tips would help.

@loiclec
Copy link
Owner

loiclec commented Feb 14, 2022

Hi! Thanks for including a link to the code. I am trying to reproduce it but found this compile-time error:

error: couldn't read /opt/dev_env/conduit-fuzzer/config.toml: No such file or directory (os error 2)
   --> src/conduit_magic.rs:299:22
    |
299 |     let config_str = include_str!("/opt/dev_env/conduit-fuzzer/config.toml");
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `include_str` (in Nightly builds, run with -Z macro-backtrace for more info)

I couldn't find the file conduit-fuzzer/config.toml. Could you share its content so that I can include it?

Regarding the bug itself, it seems like fuzzcheck thinks that the input doesn't cause any crash. Have you verified that it fails the test reliably? That being said, I wouldn't be surprised if there is a bug in fuzzcheck itself, the minify command hasn't received a lot of attention lately.

@MTRNord
Copy link
Author

MTRNord commented Feb 14, 2022

Hi! Thanks for including a link to the code. I am trying to reproduce it but found this compile-time error:

error: couldn't read /opt/dev_env/conduit-fuzzer/config.toml: No such file or directory (os error 2)
   --> src/conduit_magic.rs:299:22
    |
299 |     let config_str = include_str!("/opt/dev_env/conduit-fuzzer/config.toml");
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `include_str` (in Nightly builds, run with -Z macro-backtrace for more info)

I couldn't find the file conduit-fuzzer/config.toml. Could you share its content so that I can include it?

Regarding the bug itself, it seems like fuzzcheck thinks that the input doesn't cause any crash. Have you verified that it fails the test reliably? That being said, I wouldn't be surprised if there is a bug in fuzzcheck itself, the minify command hasn't received a lot of attention lately.

Hi oops seems i forgot that file.
It's contents are:

[global]
server_name = "localhost"
database_path = ":db_path:"
port = 1111
max_request_size = 20_000_000
allow_registration = true
allow_encryption = true
allow_federation = false
trusted_servers = ["matrix.org"]
address = "127.0.0.1"
workers = 1

The error I had was "double free or corruption (out)". However I am not entirely sure if thats a bug in the code I was fuzzing or something in between fuzzcheck and the code. (As for rust code this is a quite unusual error I think).

The verification test I now did doesnt seem to fail (using the same harness but outside of fuzzcheck). So this error may have happened in fuzzcheck or due to the way fuzzcheck works. So that may explain why minify might not work 🤔

@loiclec
Copy link
Owner

loiclec commented Feb 15, 2022

Thanks, I can compile the code and reproduce it now :)

I have added an option to cargo-fuzzcheck to enable AddressSanitizer, in the hopes that it would catch the error exactly when it happens and help debug it. And I have verified that it works well when used in another simple fuzz test. But in this specific case it doesn't help much.

I have also added a way to look into the details of each test failure (they will be written to fuzz/<test_name>/stats/test_failures.json. The three failures I found for the conduit fuzzer are:

"panicked at 'Database data for admin room alias must be valid: SqliteError { source: SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some(\"no such table: alias_roomid\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/admin.rs:72:18"
 "panicked at 'called `Result::unwrap()` on an `Err` value: SqliteError { source: SqliteFailure(Error { code: SystemIoFailure, extended_code: 1802 }, Some(\"disk I/O error\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/abstraction/sqlite.rs:73:83"
"panicked at 'called `Result::unwrap()` on an `Err` value: SqliteError { source: SqliteFailure(Error { code: SystemIoFailure, extended_code: 1802 }, Some(\"disk I/O error\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/abstraction/sqlite.rs:68:83"

But none of them are reproducible by using the verification test. Fuzz tests that perform IO operations unfortunately come with a lot of drawbacks: each iteration is very slow and the failures may not be reproducible. I have honestly not given them much thought when writing fuzzcheck, as it was originally meant as a fuzzer for pure functions whose behaviour doesn't change at all between different executions.

Also note that the fuzz test which gave these panic messages was done on my computer using the latest (unpublished) version of fuzzcheck. It may be that there really was a bug in fuzzcheck 0.10.1 but that it was fixed in the meantime.

How did you find that the error was “double free or corruption (out)”? And do you think there is a way to get a backtrace or some other debug information right when it happens?

Finally, let me know if you managed to find more information about why these errors happen, where the double-free happens, or whether you managed to make everything work. Then, if you decide to go ahead, I can also help you make the best use of fuzzcheck :) For example, one thing I noticed in the conduit fuzzer is that the coverage information from the actual tested crate is not observed by fuzzcheck, because it defaults to observing only the top-level crate. This should be better emphasised in the documentation.

@MTRNord
Copy link
Author

MTRNord commented Feb 15, 2022

Thanks, I can compile the code and reproduce it now :)

I have added an option to cargo-fuzzcheck to enable AddressSanitizer, in the hopes that it would catch the error exactly when it happens and help debug it. And I have verified that it works well when used in another simple fuzz test. But in this specific case it doesn't help much.

I have also added a way to look into the details of each test failure (they will be written to fuzz/<test_name>/stats/test_failures.json. The three failures I found for the conduit fuzzer are:

"panicked at 'called `Result::unwrap()` on an `Err` value: SqliteError { source: SqliteFailure(Error { code: SystemIoFailure, extended_code: 1802 }, Some(\"disk I/O error\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/abstraction/sqlite.rs:73:83"
 "panicked at 'called `Result::unwrap()` on an `Err` value: SqliteError { source: SqliteFailure(Error { code: SystemIoFailure, extended_code: 1802 }, Some(\"disk I/O error\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/abstraction/sqlite.rs:73:83"
"panicked at 'called `Result::unwrap()` on an `Err` value: SqliteError { source: SqliteFailure(Error { code: SystemIoFailure, extended_code: 1802 }, Some(\"disk I/O error\")) }', /Users/loic/.cargo/git/checkouts/conduit-ee89ce08c05579d9/2b644ef/src/database/abstraction/sqlite.rs:68:83"

But none of them are reproducible by using the verification test. Fuzz tests that perform IO operations unfortunately come with a lot of drawbacks: each iteration is very slow and the failures may not be reproducible.

I expected that :) But hoped it wouldnt be too bad and to be honest the resets between runs are honestly quite hacky anyway from my side :) Thanks for looking into this though :)

I have honestly not given them much thought when writing fuzzcheck, as it was originally meant as a fuzzer for pure functions whose behaviour doesn't change at all between different executions.

Also note that the fuzz test which gave these panic messages was done on my computer using the latest (unpublished) version of fuzzcheck. It may be that there really was a bug in fuzzcheck 0.10.1 but that it was fixed in the meantime.

How did you find that the error was “double free or corruption (out)”? And do you think there is a way to get a backtrace or some other debug information right when it happens?

The message was logged between the coverage logging and then it locked up entirely it seemed. I can check later this week. Currently I am ill so I am not spending too much time at my PC :)

Finally, let me know if you managed to find more information about why these errors happen, where the double-free happens, or whether you managed to make everything work. Then, if you decide to go ahead, I can also help you make the best use of fuzzcheck :)

For example, one thing I noticed in the conduit fuzzer is that the coverage information from the actual tested crate is not observed by fuzzcheck, because it defaults to observing only the top-level crate. This should be better emphasised in the documentation.

Yeah thats mostly due to the fact that conduit (the tested crate) relies on an crate by the name "ruma" for its types. And that is a quite huge sdk for the matrix protocol. It currently is missing the derive annotations and we probably want to wait to have pattern based string mutators too. So it is kinda blocked behind #6 I think. As putting this into the conduit crate itself currently would cause quite a lot of code duplication for proper fuzzing. This currently is more in the POC phase (The endpoint tested for example is not really the best to fuzz, there are more viable ones in the proto :) ). So there is certainly a lot to improve for sure :D

@loiclec
Copy link
Owner

loiclec commented Feb 15, 2022

No worries :) It is true that it is probably a bit too early to fuzz a large crate with lots of strings such as conduit/ruma, especially when the fuzz test would have to perform IO.

I am still thinking about pattern-based string mutators. It is a tough problem to solve in the case where a single string can be interpreted in an exponential number of ways by a single pattern (e.g. for the regex ((ab)|a|b)+ and the string abababababab, which part of the string comes from which part of the pattern?). But I will try to release a version that works for unambiguous grammars sooner :)

I hope you get well soon! Have a nice day 😊

@MTRNord
Copy link
Author

MTRNord commented Feb 15, 2022

We mostly have to cover things like @<userpart>:<serverpart> where the user part has a known grammar of allowed characters. Similiar for rooms and events on the network. So the patterns we would have shouldn't be too problematic on paper. (obviously doesn't mean that there can't be edge cases I am currently not aware of 😅)

And thanks :) have a great day too. Thanks for all the help with this :)

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

2 participants