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

go-fuzz: add fuzz.F #223

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

Conversation

josharian
Copy link
Collaborator

This is initial work towards #218.

DO NOT MERGE

I am sending it for discussion.

I am not entirely satisfied with this as it stands.

  • It's a bit cumbersome.
  • It's not well documented (yet).
  • It requires starting a goroutine for every fuzz function invocation. Fixing this requires re-doing some of the layering and will be pretty disruptive.

Though it is not ready to merge, it is my hope that this will help move along the conversation. Feedback welcome as we all ponder more.

This is initial work towards dvyukov#218.
@dvyukov
Copy link
Owner

dvyukov commented Mar 14, 2019

Good that you preserved backwards compatibility!

Re cumbersome, do you mean the large code template embed in the code? I am not too worried about strictly internal details that we are free to change at any time.

Re goroutine per test, the only reason for this is Skip/Goexit, right?

@dvyukov
Copy link
Owner

dvyukov commented Mar 14, 2019

Re goroutine per test, the only reason for this is Skip/Goexit, right?

Have you considered panicing and recovering? I think the same could be used for Fail as well.

@josharian
Copy link
Collaborator Author

Re cumbersome, do you mean the large code template embed in the code?

Yes.

I am not too worried about strictly internal details that we are free to change at any time.

Fair enough.

Re goroutine per test, the only reason for this is Skip/Goexit, right?

Yes.

Have you considered panicing and recovering? I think the same could be used for Fail as well.

If the fuzz function calls recover, then we might never see the panic. This is the same reason that package testing uses a goroutine per invocation plus runtime.Goexit.

@dvyukov
Copy link
Owner

dvyukov commented Apr 1, 2019

If the fuzz function calls recover, then we might never see the panic. This is the same reason that package testing uses a goroutine per invocation plus runtime.Goexit.

Recovering is kind of a corner case, but I guess sooner or later people will hit it and ask for proper handling. So handling it from day one is probably the right thing to do.

But we don't have to have a goroutine per test because of this, right?
We could create a worker goroutine and a reused channel for completion and as long as it does not Goexit, we reuse the same goroutine. If it exits, we create another one. This should give the same amortized performance as we have now.

josharian added a commit to josharian/go-fuzz that referenced this pull request May 9, 2019
libfuzzer's generated main function uses package reflect.
When attempting to build a package that doesn't depend
on reflect, such as github.com/dvyukuv/go-fuzz-corpus/{bzip2,gif,url},
package reflect wasn't getting copied to GOROOT, and the build failed.

Fix that.

We may need something similar in the future for fuzz.F; see dvyukov#223.

Updates google/oss-fuzz/#2188
dvyukov pushed a commit that referenced this pull request May 10, 2019
libfuzzer's generated main function uses package reflect.
When attempting to build a package that doesn't depend
on reflect, such as github.com/dvyukuv/go-fuzz-corpus/{bzip2,gif,url},
package reflect wasn't getting copied to GOROOT, and the build failed.

Fix that.

We may need something similar in the future for fuzz.F; see #223.

Updates google/oss-fuzz/#2188
bradleyjkemp pushed a commit to bradleyjkemp/simple-fuzz that referenced this pull request Oct 23, 2019
libfuzzer's generated main function uses package reflect.
When attempting to build a package that doesn't depend
on reflect, such as github.com/dvyukuv/go-fuzz-corpus/{bzip2,gif,url},
package reflect wasn't getting copied to GOROOT, and the build failed.

Fix that.

We may need something similar in the future for fuzz.F; see dvyukov#223.

Updates google/oss-fuzz/#2188
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

2 participants