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

fuzztests: check error code of each individual fuzztest #297

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

oetr
Copy link
Contributor

@oetr oetr commented Jan 26, 2023

Now failed fuzz tests will be recognized as such, and the GitHub pipeline will fail.
New feature: fuzz tests now support timeouts.

@oetr oetr requested a review from kyakdan January 26, 2023 16:27
@oetr
Copy link
Contributor Author

oetr commented Jan 26, 2023

I "fixed" the pipeline and the failing tests. Locally the script runs without issues.
Running this locally in the fuzztests directory:
JAZZER_FUZZ=1 npm test -- --testNamePattern='consumes the provided input'
results in error code 142:
[1] 24145 alarm JAZZER_FUZZ=1 npm test -- --testNamePattern='consumes the provided input'
Any idea of what is causing the issue?

@0xricksanchez
Copy link
Contributor

libfuzzer has a SIGALARM handler that is triggered IFF a timeout has been given to the libfuzzer invocation via --timeout:

https://github.com/llvm/llvm-project/blob/6772966dcdf5175c517832c1b2352b4fcd1d6b16/compiler-rt/lib/fuzzer/FuzzerUtilPosix.cpp#L117

The default is quite high already, like over a 1000secs AFAIK. I am uncertain if we set some smaller value anywhere, from the top of my head, rn.

@0xricksanchez
Copy link
Contributor

0xricksanchez commented Jan 26, 2023

Looks like packages/jest-runner/fuzz.ts:75 is the culprit here. When changing the default timeout value (s. below), which was like 5k it works just fine for me:

	const timeout = currentTimeout();

	const corpus = new Corpus(testFile, testStatePath);

	const fuzzingConfig = loadConfig();
        // CHANGED
	fuzzingConfig.timeout = 100000; //timeout;

@kyakdan
Copy link
Member

kyakdan commented Jan 27, 2023

The -timeout option in libFuzzer affects single fuzzer iterations and defaults to 1200 seconds. This means that if a given input takes longer than this timeout, the process is treated as a failure case. In our tests, we set it to 5 seconds. I don't see why this can be triggered in this test case. I'll have a look.

@oetr
Copy link
Contributor Author

oetr commented Jan 27, 2023

I see that in that fuzz test, -max_total_time is set to 30 (seconds), however, the standard Jest timeout is 5000 (milliseconds). That should be the reason the problem happens. But for some reason loadConfig ignores my setting in .jazzerjsrc and continues to use the default value of 5000. Looking into that right now.

@bertschneider
Copy link
Contributor

@oetr Ah, yes. Timeouts should be handled differently, depending on the execution mode, but probably aren't.
In regression mode one test case per input is generated, whereas only one test case for all invocations is generated in fuzzing mode. This could lead to the observed problems.

@oetr oetr force-pushed the FUZZ-537-fix-fuzztests-pipeline-and-tests branch 2 times, most recently from e3c485a to 4dd38b2 Compare January 27, 2023 08:40
@oetr oetr marked this pull request as ready for review January 27, 2023 09:12
@oetr oetr requested a review from a team January 27, 2023 10:33
Copy link
Member

@kyakdan kyakdan left a comment

Choose a reason for hiding this comment

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

The PR looks very good. I've left a single comment to make the timeout flag consistent with libFuzzer.

packages/jest-runner/fuzz.ts Outdated Show resolved Hide resolved
fuzztests/runFuzzTests.js Outdated Show resolved Hide resolved
fuzztests/runFuzzTests.js Outdated Show resolved Hide resolved
packages/jest-runner/fuzz.ts Show resolved Hide resolved
packages/jest-runner/config.ts Outdated Show resolved Hide resolved
packages/jest-runner/fuzz.ts Outdated Show resolved Hide resolved
packages/jest-runner/fuzz.ts Outdated Show resolved Hide resolved
@oetr oetr force-pushed the FUZZ-537-fix-fuzztests-pipeline-and-tests branch from a74d4b0 to af92df2 Compare January 31, 2023 18:53
packages/instrumentor/edgeIdStrategy.ts Outdated Show resolved Hide resolved
packages/core/cli.ts Outdated Show resolved Hide resolved
@bertschneider
Copy link
Contributor

Really good addition and fix!

Could you please add parenthesis around if/else blocks for a consistent coding style?

@oetr oetr force-pushed the FUZZ-537-fix-fuzztests-pipeline-and-tests branch 4 times, most recently from 6bb7e56 to 97dbadd Compare February 1, 2023 10:20
@oetr oetr force-pushed the FUZZ-537-fix-fuzztests-pipeline-and-tests branch from 97dbadd to 074745b Compare February 1, 2023 12:12
@oetr oetr merged commit ce115f5 into main Feb 1, 2023
@oetr oetr deleted the FUZZ-537-fix-fuzztests-pipeline-and-tests branch February 1, 2023 12:28
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.

None yet

4 participants