Skip to content

Add 2 fuzzers for nu-path, nu-parser#10376

Merged
sholderbach merged 15 commits intonushell:mainfrom
tokatoka:main
Sep 16, 2023
Merged

Add 2 fuzzers for nu-path, nu-parser#10376
sholderbach merged 15 commits intonushell:mainfrom
tokatoka:main

Conversation

@tokatoka
Copy link
Copy Markdown
Contributor

@tokatoka tokatoka commented Sep 14, 2023

Description

This PR adds a fuzzer for the nu-path and the nu-parser crate.
Now you can go to crates/nu-path/fuzz/crates/nu-parser/fuzz and run cargo fuzz to
find crashes.
#10365 and #9417 was found by
this

User-Facing Changes

it doesn't affect user experience

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the rust-toolchain.toml, we use it to ensure the Rust version used by developers is compatible with the packaging "vendors".

Not sure about what is up with crates/nu-path/src/util.rs? Is this unlinked in the module tree?

If we land this into the tree it would be great to have a readme pointing to relevant documentation.

@tokatoka
Copy link
Copy Markdown
Contributor Author

Please keep the rust-toolchain.toml, we use it to ensure the Rust version used by developers is compatible with the packaging "vendors".

Not sure about what is up with crates/nu-path/src/util.rs? Is this unlinked in the module tree?

yes I mistakenly pushed other changes :D

@sholderbach
Copy link
Copy Markdown
Member

No problem! Thanks for sharing your work, this sounds helpful to have, great that you already found a problem through that.

@tokatoka tokatoka marked this pull request as draft September 14, 2023 20:30
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Sep 14, 2023

I think this is a great idea to introduce fuzzing. nu-path is a good place to start. Thanks!

@tokatoka tokatoka changed the title Add a fuzzer for nu-path Add 2 fuzzers for nu-path, nu-parser Sep 15, 2023
@tokatoka tokatoka marked this pull request as ready for review September 15, 2023 14:02
@tokatoka
Copy link
Copy Markdown
Contributor Author

I added the README.md now. This pr is ready

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Sep 15, 2023

gather_seeds.sh makes this look like it only works on *nix platforms. Is there no way to make this work in Windows too? Seems like the same thing could be done with a nushell script and nu gather_seeds.nu.

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't played with cargo-fuzz so far but the targets look good to me.

Will give it a try later :)

Comment thread crates/nu-path/fuzz/README.md Outdated
Comment thread crates/nu-parser/fuzz/gather_seeds.sh Outdated
@tokatoka
Copy link
Copy Markdown
Contributor Author

gather_seeds.sh makes this look like it only works on *nix platforms. Is there no way to make this work in Windows too? Seems like the same thing could be done with a nushell script and nu gather_seeds.nu.

actually cargo fuzz says it works only on *nix platforms. but anyways i switched to use nu

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this seems to go smooth!

Already found something new that triggers an access out ouf bounds panic:

minimized-from-23616a49a67ddbe0e9e36ab7fa5b0fe9751a6d04.txt

$  nu artifacts/parse/minimized-from-23616a49a67ddbe0e9e36ab7fa5b0fe9751a6d04
thread 'main' panicked at 'index out of bounds: the len is 2 but the index is 3', /home/stefan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nu-parser-0.84.0/src/parser.rs:745:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Comment thread crates/nu-parser/fuzz/.gitignore
@tokatoka
Copy link
Copy Markdown
Contributor Author

Already found something new that triggers an access out ouf bounds panic:

You'll find many :D

tokatoka and others added 2 commits September 15, 2023 19:37
@sholderbach sholderbach merged commit bc7736b into nushell:main Sep 16, 2023
@anka-213
Copy link
Copy Markdown
Contributor

anka-213 commented Sep 19, 2023

I just found a file that triggers a rapid memory leak, about 4 GB in a minute
oom-1dae5132c7066b2c686fe27d2c0f247df4d17dd1.txt

edit: It seems like it's caused by exponential memory use in the parser for tables. Here's a more minimal reproduction:

[[[[[[[[[[[[[[[[[[[[[a

I have now opened #10438 for it

@tokatoka
Copy link
Copy Markdown
Contributor Author

nice 😉

hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description

This PR adds a fuzzer for the nu-path and the nu-parser crate.
Now you can go to `crates/nu-path/fuzz`/`crates/nu-parser/fuzz` and run `cargo fuzz` to
find crashes.
nushell#10365 and nushell#9417 was found by
this


---------

Co-authored-by: sholderbach <sholderbach@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

4 participants