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

testing: unclear what t.Skip does in the context of fuzzing #48779

Open
mvdan opened this issue Oct 4, 2021 · 7 comments · May be fixed by #55048
Open

testing: unclear what t.Skip does in the context of fuzzing #48779

mvdan opened this issue Oct 4, 2021 · 7 comments · May be fixed by #55048
Labels
fuzz Issues related to native fuzzing support NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Oct 4, 2021

The current "first class fuzzing" proposal design doc (https://go.googlesource.com/proposal/+/master/design/draft-fuzzing.md) only has one mention of "skip":

	// Run the fuzz test
	f.Fuzz(func(t *testing.T, a string, num *big.Int) {
		t.Parallel() // seed corpus tests can run in parallel
		if num.Sign() <= 0 {
			t.Skip() // only test positive numbers
		}

As I was porting one of my fuzz funcs from go-fuzz to first class fuzzing, I remembered that go-fuzz actually has two kinds of "skips":

The function must return 1 if the fuzzer should increase priority of the given input during subsequent fuzzing (for example, the input is lexically correct and was parsed successfully); -1 if the input must not be added to corpus even if gives new coverage; and 0 otherwise; other values are reserved for future use.

So it seems like, with go-fuzz, you have three options: give priority (+1), must ignore (-1), or default behavior (0).

Personally, I only used +1 or 0 in my go-fuzz funcs; I never had any input that must never be added to the corpus. I've adapted those returns for the new fuzzer so that the return 1 is now a regular return ending the function, and return 0 is now a t.Skip to signal an uninteresting input. Here's an example: mvdan/sh@e186e04

I'd love it if the meaning of t.Skip in the context of fuzz funcs was better documented. Perhaps my understanding of what it does is incorrect, or perhaps it doesn't correspond to return 0 in go-fuzz.

Another question is whether we want an equivalent to go-fuzz's return -1. I personally didn't have a need for it, but others might be using it and not know how to transition those to the new fuzzer.

cc @katiehockman

@mvdan mvdan added the fuzz Issues related to native fuzzing support label Oct 4, 2021
@mvdan
Copy link
Member Author

mvdan commented Oct 4, 2021

The docs do say:

The Skip method of *T can be used in a fuzz target if the input is invalid, but should not be considered a crash.

Still unclear to me if this means that the input can end up in the cached corpus inside the build cache or not - to my understanding, that would be the difference between go-fuzz's 0 and -1.

@jayconrod
Copy link
Contributor

cc @golang/fuzzing We should clarify this.

Currently T.Skip does nothing specific to fuzzing; it's like returning early.

We should make sure that if T.Skip is called, an input will not be considered "interesting" even if it provides new coverage. I think that would be equivalent to return -1 in go-fuzz.

@jayconrod jayconrod added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 4, 2021
@jayconrod jayconrod added this to the Go1.19 milestone Oct 4, 2021
@mvdan
Copy link
Member Author

mvdan commented Oct 4, 2021

So I guess go-fuzz's return 0 would be equivalent to just return without t.Skip? In that scenario I guess I'd mostly avoid t.Skip in the context of fuzzing, just like I didn't need to use return -1 for go-fuzz. Some guidance on when or when not to use Skip would be certainly welcome.

@mvdan mvdan changed the title cmd/go: unclear what t.Skip does in the context of fuzzing testing: unclear what t.Skip does in the context of fuzzing Oct 5, 2021
@rsc
Copy link
Contributor

rsc commented Oct 7, 2021

Did you mean Go 1.19 here? Or is this Go 1.18?

@jayconrod
Copy link
Contributor

1.19 most likely, though it would be nice to have for 1.18. There are already a lot of open release blockers to fix soon though.

@ianlancetaylor
Copy link
Contributor

CC @golang/fuzzing

What is the status of this issue? Thanks.

capnspacehook added a commit to capnspacehook/go that referenced this issue Jul 15, 2022
Calling T.Skip previously had the same effect as returning
early from a fuzz test. Now it will signal to the fuzzer
that the input should never be considered "interesting" even
if it produces new coverage. This is equivalent to "return 1"
in go-fuzz.

Fixes golang#48779
@gopherbot gopherbot modified the milestones: Go1.19, Go1.20 Aug 2, 2022
@julieqiu julieqiu modified the milestones: Go1.20, Backlog Sep 8, 2022
capnspacehook pushed a commit to capnspacehook/go that referenced this issue Sep 13, 2022
Calling T.Skip previously had the same effect as returning
early from a fuzz test. Now it will signal to the fuzzer
that the input should never be considered "interesting" even
if it produces new coverage. This is equivalent to "return 1"
in go-fuzz.

Fixes golang#48779
capnspacehook pushed a commit to capnspacehook/go that referenced this issue Sep 13, 2022
Calling T.Skip previously had the same effect as returning
early from a fuzz test. Now it will signal to the fuzzer
that the input should never be considered "interesting" even
if it produces new coverage. This is equivalent to "return 1"
in go-fuzz.

Fixes golang#48779
@gopherbot
Copy link

Change https://go.dev/cl/430676 mentions this issue: internal/fuzz: make T.Skip ignore inputs when fuzzing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzz Issues related to native fuzzing support NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: No status
Status: No status
Development

Successfully merging a pull request may close this issue.

6 participants