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

image: add sample fuzz tests for prototype of "fuzzing as a first class citizen" #30979

Open
thepudds opened this issue Mar 21, 2019 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@thepudds
Copy link
Contributor

Summary

As a follow-up to #30719 and in support of the proposal to "make fuzzing a first class citizen" in #19109, the suggestion here is to add Fuzz functions for the following three standard library packages:

  1. image/jpeg, using https://github.com/dvyukov/go-fuzz-corpus/blob/master/jpeg/jpeg.go

  2. image/png, using https://github.com/dvyukov/go-fuzz-corpus/blob/master/png/png.go

  3. image/gif, using a modified https://github.com/dvyukov/go-fuzz-corpus/blob/master/gif/gif.go (some additional discussion below).

Note that this issue is solely about the Fuzz functions themselves, and this issue does not cover checking in any resulting fuzzing corpus (which is likely going to be a separate repository such as golang/x/corpus or golang.org/x/fuzz or perhaps using oss-fuzz; the intent is to discuss that aspect separately in a follow-up issue).

Background

See the "Background" section of #30719 or #19109 (comment).

Additional Details

Following the pattern set by #30719 and https://golang.org/cl/167097, the following are likely true for how to proceed here:

  1. The build tag should be // +build gofuzz
  2. The name of the files should be fuzz.go
  3. The license header should be the Go standard library license. @dvyukov might need to make a similar statement as he made in CL 167097.
  4. In general, even for Fuzz functions guarded by a build tag, care should be taken to avoid introducing new dependencies, especially with the introduction of modules. Note that go mod tidy looks across all build tags, so +build gofuzz does not reduce module dependencies.

For reference, here is a gist showing the diff between dvyukov/go-fuzz-corpus/tiff/tiff.go and the final form of that file as merged into golang/x/image repo as part of #30719. For the first two listed above (image/png and image/jpeg), hopefully it would be as straightforward as that diff illustrates.

For dvyukov/go-fuzz-corpus/gif, it currently depends on "github.com/dvyukov/go-fuzz-corpus/fuzz" for a utility function fuzz.DeepEqual. I think that dependency on dvyukov/go-fuzz-corpus would need to be eliminated prior to putting go-fuzz-corpus/gif/gif.go into the standard library. Possible solutions might be: (a) to start, that piece of the Fuzz function could simply be eliminated for now, or (b) a roughly corresponding DeepEqual from the standard library could be substituted, or (c) that github.com/dvyukov/go-fuzz-corpus/fuzz utility function could temporarily be placed directly in image/gif/fuzz.go, or (d) some other solution.

Happy to discuss any aspect of this, and of course happy to be corrected if any of the above is different than how people would like to proceed here.

CC @dvyukov @josharian @nigeltao @FiloSottile @acln0

@dvyukov
Copy link
Member

dvyukov commented Mar 21, 2019

I think that dependency on dvyukov/go-fuzz-corpus would need to be eliminated prior to putting go-fuzz-corpus/gif/gif.go into the standard library.

Agree.
I think we should use whatever would be considered the idiomatic solution for tests, tests do lots of such comparisons. There is no reason to do something different here. I think the idiomatic solution is to open-code all comparisons (?).

@acln0
Copy link
Contributor

acln0 commented Mar 21, 2019

I'm happy to see that progress is being made, and I am willing to prepare these next CLs too, if that's okay with everyone.

It seems like of the three packages listed in this issue, gif requires the most attention:

First, there are two fuzz functions: a Fuzz and a FuzzAll. I assume both should be included. Do their names stay as they are?

Second, @dvyukov, what exactly do you mean by "open-code" all comparisons? Explicitly writing loops and such, and avoiding reflect.DeepEqual or similar?

@dvyukov
Copy link
Member

dvyukov commented Mar 21, 2019

First, there are two fuzz functions: a Fuzz and a FuzzAll. I assume both should be included. Do their names stay as they are?

I named them after the actual Encode/Decode functions. Seems fine to me.

Second, @dvyukov, what exactly do you mean by "open-code" all comparisons? Explicitly writing loops and such, and avoiding reflect.DeepEqual or similar?

Using != for scalars, strings; bytes.Equal for byte slices, etc. E.g.

if a.Foo != b.Foo {
  panic("bad Foo")
}
if !bytes.Equal(a.Bar, b.Bar) {
  panic("bad Bad")
}

@thepudds
Copy link
Contributor Author

thepudds commented Mar 21, 2019

Regarding this piece about FuzzAll for gif:

I think the idiomatic solution is to open-code all comparisons (?).

It seems image/src/gif/write_test.go has some logic around doing comparison, including in testEncodeAll. I have not looked carefully, but perhaps some or all of that logic could be reused for FuzzAll for gif.

@acln0
Copy link
Contributor

acln0 commented Mar 21, 2019

I would like to bring to your attention the fact that building go-fuzz binaries for standard library packages (from inside the packages themselves, unlike dvyukov/go-fuzz-corpus) seems to be a little tricky. Perhaps these things are obvious in retrospect, but to me, they were not, at least initially.

Apart from GO111MODULE=off being necessary, the go that go-fuzz-build finds through exec.LookPath must be from the Go tree as the package being instrumented. In other words, go-fuzz-build image/png tries to build an instrumented binary for the image/png relative to the go it found in PATH. More concretely, ISTM that if you are looking to fuzz image/png from /some/place/go/src/image/png, you must arrange for /some/place/go/bin/go to be the first go in PATH, when you run go-fuzz-build.

I imagine that for people who do not use two different Go trees (one for general work using Go and another for working on Go itself), this is not a problem at all, but if you are like me and use different trees, you might hit this, and be surprised or confused.

I am hoping that this post helps with this potential confusion.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/168558 mentions this issue: image/png: add Fuzz function

gopherbot pushed a commit that referenced this issue Mar 22, 2019
Add a Fuzz function to package png, under the gofuzz build
tag. This function is based on the png/png.go code, from
github.com/dvyukov/go-fuzz-corpus, modified to use direct
comparison of image bounds rather than reflect.DeepEqual.

Updates #30979
Updates #19109

Change-Id: Idb86e7ded0c2d78e6cadbeda84c7b1f35b8c579c
Reviewed-on: https://go-review.googlesource.com/c/go/+/168558
Reviewed-by: thepudds <thepudds1460@gmail.com>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Feb 17, 2021
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Feb 17, 2021
@ianlancetaylor
Copy link
Member

@katiehockman @thepudds What is the status of this issue considering the active work on fuzzing? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants