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

Add a single file compilation fuzzer #958

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bshastry
Copy link

What was wrong?

Single file compilation fuzzer was missing.

How was it fixed?

Adding a fuzzer harness.

To-Do

  • Fix build: Currently building a fuzzer library, what we need is a binary

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@bshastry bshastry changed the title WIP Add a single file compilation fuzzer Nov 15, 2023
@bshastry bshastry force-pushed the add-fuzzer branch 2 times, most recently from a8e1e57 to 17bde18 Compare November 22, 2023 03:52
@sbillig
Copy link
Collaborator

sbillig commented Dec 2, 2023

@bshastry The build process and fuzzer both seem to work fine without the changes you made to Cargo.toml. I'm doing
cargo +nightly fuzz run single_file_fuzzer (with the latest nightly). Is there something else that isn't working that required the Cargo.toml changes?

@bshastry
Copy link
Author

bshastry commented Dec 4, 2023

@bshastry The build process and fuzzer both seem to work fine without the changes you made to Cargo.toml. I'm doing cargo +nightly fuzz run single_file_fuzzer (with the latest nightly). Is there something else that isn't working that required the Cargo.toml changes?

You are right, I'm not sure what I did differently that required an update. Perhaps I did a rustup default nightly followed by cargo fuzz build... (assuming cargo +nightly fuzz is equivalent to rustup default nightly followed by cargo fuzz) that may have errored on parsing the legacy root Cargo.toml but I'm not exactly sure.

I updated this PR undoing changes to the root Cargo.toml.

@sbillig
Copy link
Collaborator

sbillig commented Dec 5, 2023

When I build this locally, the following changes are automatically applied to the Cargo.lock file. This is presumably the Cargo.lock file that the CI is expecting to see. I'm not sure why the Cargo.lock file isn't being modified in the same way on your machine. Maybe delete Cargo.lock (and maybe run cargo clean), build again, and see what's generated?

diff --git a/Cargo.lock b/Cargo.lock
index fe0e8426..bd61b124 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -87,12 +87,6 @@ dependencies = [
  "syn 2.0.39",
 ]

-[[package]]
-name = "arbitrary"
-version = "1.3.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7d5a26814d8dcb93b0e5a0ff3c6d80a8843bafb21b39e8e18a6f05471870e110"
-
 [[package]]
 name = "ark-ff"
 version = "0.3.0"
@@ -440,7 +434,6 @@ version = "1.0.83"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "f1174fb0b6ec23863f8b971027804a42614e347eafb0a95bf0b12cdae21fc4d0"
 dependencies = [
- "jobserver",
  "libc",
 ]

@@ -1213,16 +1206,6 @@ dependencies = [
  "vfs",
 ]

-[[package]]
-name = "fe-fuzzers"
-version = "0.26.0"
-dependencies = [
- "dir-test",
- "fe-common",
- "fe-driver",
- "libfuzzer-sys",
-]
-
 [[package]]
 name = "fe-library"
 version = "0.26.0"
@@ -1824,15 +1807,6 @@ version = "1.0.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "af150ab688ff2122fcef229be89cb50dd66af9e01a4ff320cc137eecc9bacc38"

-[[package]]
-name = "jobserver"
-version = "0.1.27"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8c37f63953c4c63420ed5fd3d6d398c719489b9f872b9fa683262f8edd363c7d"
-dependencies = [
- "libc",
-]
-
 [[package]]
 name = "js-sys"
 version = "0.3.66"
@@ -1885,17 +1859,6 @@ version = "0.2.150"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "89d92a4743f9a61002fae18374ed11e7973f530cb3a3255fb354818118b2203c"

-[[package]]
-name = "libfuzzer-sys"
-version = "0.4.7"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a96cfd5557eb82f2b83fed4955246c988d331975a002961b07c81584d107e7f7"
-dependencies = [
- "arbitrary",
- "cc",
- "once_cell",
-]
-
 [[package]]
 name = "libloading"
 version = "0.7.4"

@sbillig
Copy link
Collaborator

sbillig commented Dec 5, 2023

@bshastry I should have mentioned earlier that most of the compiler is in the process of being rewritten, on the fe-v2 branch. It would be more useful to add the fuzzer to that branch. The fe-v2 branch is expected to be ready for use in a few months, and we likely won't fix minor issues on the master branch. (That said, it would be useful to add any bugs you find to our test suite, to ensure that the same problems don't exist on the fe-v2 branch)

However, the fe-v2 branch driver crate (currently called driver2) doesn't have a simple entry point. You could add one to start fuzzing the new parser at least. The other components are maybe not quite ready for fuzzing, but some major changes should be merged into the fe-v2 branch someday soon.

@bshastry
Copy link
Author

bshastry commented Dec 5, 2023

When I build this locally, the following changes are automatically applied to the Cargo.lock file. This is presumably the Cargo.lock file that the CI is expecting to see. I'm not sure why the Cargo.lock file isn't being modified in the same way on your machine. Maybe delete Cargo.lock (and maybe run cargo clean), build again, and see what's generated?

diff --git a/Cargo.lock b/Cargo.lock
index fe0e8426..bd61b124 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -87,12 +87,6 @@ dependencies = [
  "syn 2.0.39",
 ]

-[[package]]
-name = "arbitrary"
-version = "1.3.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7d5a26814d8dcb93b0e5a0ff3c6d80a8843bafb21b39e8e18a6f05471870e110"
-
 [[package]]
 name = "ark-ff"
 version = "0.3.0"
@@ -440,7 +434,6 @@ version = "1.0.83"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "f1174fb0b6ec23863f8b971027804a42614e347eafb0a95bf0b12cdae21fc4d0"
 dependencies = [
- "jobserver",
  "libc",
 ]

@@ -1213,16 +1206,6 @@ dependencies = [
  "vfs",
 ]

-[[package]]
-name = "fe-fuzzers"
-version = "0.26.0"
-dependencies = [
- "dir-test",
- "fe-common",
- "fe-driver",
- "libfuzzer-sys",
-]
-
 [[package]]
 name = "fe-library"
 version = "0.26.0"
@@ -1824,15 +1807,6 @@ version = "1.0.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "af150ab688ff2122fcef229be89cb50dd66af9e01a4ff320cc137eecc9bacc38"

-[[package]]
-name = "jobserver"
-version = "0.1.27"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8c37f63953c4c63420ed5fd3d6d398c719489b9f872b9fa683262f8edd363c7d"
-dependencies = [
- "libc",
-]
-
 [[package]]
 name = "js-sys"
 version = "0.3.66"
@@ -1885,17 +1859,6 @@ version = "0.2.150"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "89d92a4743f9a61002fae18374ed11e7973f530cb3a3255fb354818118b2203c"

-[[package]]
-name = "libfuzzer-sys"
-version = "0.4.7"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a96cfd5557eb82f2b83fed4955246c988d331975a002961b07c81584d107e7f7"
-dependencies = [
- "arbitrary",
- "cc",
- "once_cell",
-]
-
 [[package]]
 name = "libloading"
 version = "0.7.4"

I applied this patch locally and force pushed to the source branch. Now there are failing tests in the macOS CI. So definitely progress :-)

I hope the failing tests have nothing to do with the changes in this PR, at least that's what I feel after taking a cursory look at the failures in that CI.

@bshastry
Copy link
Author

bshastry commented Dec 5, 2023

@bshastry I should have mentioned earlier that most of the compiler is in the process of being rewritten, on the fe-v2 branch. It would be more useful to add the fuzzer to that branch. The fe-v2 branch is expected to be ready for use in a few months, and we likely won't fix minor issues on the master branch. (That said, it would be useful to add any bugs you find to our test suite, to ensure that the same problems don't exist on the fe-v2 branch)

However, the fe-v2 branch driver crate (currently called driver2) doesn't have a simple entry point. You could add one to start fuzzing the new parser at least. The other components are maybe not quite ready for fuzzing, but some major changes should be merged into the fe-v2 branch someday soon.

Sure, would targeting this parser API be a good start (and targeting a different PR to fe-v2 instead of the master branch)?

pub fn parse_source_file(text: &str) -> (GreenNode, Vec<ParseError>) {
let lexer = lexer::Lexer::new(text);
let mut parser = parser::Parser::new(lexer);
let checkpoint = parser.enter(RootScope::default(), None);
parser.parse(parser::ItemListScope::default(), None);
parser.leave(checkpoint);
let (node, errs) = parser.finish();
(node, errs)
}

@bshastry
Copy link
Author

bshastry commented Dec 6, 2023

Minor update: I created a fe-v2 fuzzer and a draft PR here #969

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.

2 participants