x/image: add a sample fuzz test for prototype of "fuzzing as a first class citizen"

[thepudds]

Summary

This is an issue covering adding a sample Fuzz function to x/image in support of the proposal to "make fuzzing a first class citizen" in #19109.

The suggestion in this issue is to add https://github.com/dvyukov/go-fuzz-corpus/blob/master/tiff/tiff.go to golang.org/x/image/tiff, perhaps as tiff_fuzz.go or fuzz.go. (Ultimately, the proposal is allow Fuzz functions to reside in a standard *_test.go file, but currently I think dvyukov/go-fuzz does not support that).

It likely should be protected by a gofuzz build tag (or alternatively, a fuzz build tag; some additional discussion below).

Once a single Fuzz function is added, additional Fuzz functions could be added to golang.org/x/image or other golang.org/x repositories, but that could be viewed as a follow-on steps for now.

If there is agreement to proceed on this issue, likely someone from the broader Go community could send a CL.

Background

I suspect the people interested in weighing in on this issue here might not wish to re-read the entirety of the now lengthy #19109 discussion, so a brief recap:

After some discussion in #19109, the core Go team asked for a prototype before evaluating the proposal. For example, comments from Russ in #19109 (comment), #19109 (comment), and #19109 (comment), including:

it still seems like the right next step is to make 'go-fuzz' the separate command as close to 'go fuzz' the proposed standard command as possible, and to add fuzz tests to at least the x subrepos and maybe the standard library, so that we can understand the implications of having them in the source repos (including how much space we're going to spend on testdata/fuzz corpus directories).

In order to break things down into more manageable chunks of work (including in an attempt to break out items that must be done by someone on the core Go team vs. could be done by someone from the broader community), the following steps were suggested in #19109 (comment):

Draft Goals for a Prototype

To be done before an evaluation can be made by the Go proposal review committee:

1. Prototype proposed CLI, including interaction with existing 'go test'.
2. Add some sample fuzz tests to at least the x subrepos and maybe the standard library.
3. Start an initial set of corpus directories for the x repos and maybe the standard library (for example, earlier, the proposal suggested "For the standard library it is proposed to check in corpus into golang.org/x/fuzz repo").
4. Understand how much space is used in corpus directories for x subrepos and/or standard library based on those sample fuzz tests.
5. Add a new fuzzing signature (or change the existing Fuzz(data []byte) int signature) to work with testing.TB.

Some progress on Step 1 is thepudds/fzgo, a simple work-in-progress prototype that currently implements the majority of the proposed command from the March 2017 proposal document.

Separately, @josharian has done some experimentation regarding compiler level integration, including some related discussion in #29430.

Additional Details

This issue is intended to cover at least the start of Step 2 above. @dvyukov suggested in #19109 (comment) starting with x/image.

Regarding what build tag to use here, dvyukov/go-fuzz defaults to setting the gofuzz build tag:

go-fuzz-build builds the program with gofuzz build tag, this allows to put the
Fuzz function implementation directly into the tested package, but exclude it
from normal builds with // +build gofuzz directive.

The March 2017 proposal document suggests supporting the fuzz build tag. thepudds/fzgo sets both gofuzz and fuzz build tags. For the purposes of this issue, I suggest using the gofuzz build tag for now in x/image given that is the default for dvyukov/go-fuzz.

This issue is not currently intended to cover Step 3 above in terms of creating a new corpus repository under golang.org/x (which would require action by specific people on the core Go team). I would be happy to file a separate issue for creating a new corpus repository, or if people prefer this single issue could be expanded in scope to cover discussing that as well.

Comments? Concerns? Alternative thoughts on how to make concrete progress on Step 2 above?

CC @FiloSottile @bradfitz @acln0

[josharian]

This was discussed also at google/oss-fuzz#2188 (comment) and I believe @FiloSottile is taking the lead on it.

FYI, I have been working on go-fuzz some recently, including improving the API (broadly interpreted). I have been doing so with one eye on #19109, but mainly with an emphasis on making it more useful and usable now. One of the next things I plan to do is your step 5 above ("Add a new fuzzing signature").

[dvyukov]

Sounds good to me.

I am pretty sure we do not want corpuses check-in with code. Based on my experience with go-fuzz[-corpus] this produces too much churn and changes. Imagine we have 100 fuzzers in the std lib, each day infra checks in corpus update for each. That's 100 additional changes each day and tons of diff on each check out and significant repo increase.
Maybe go modules can provide a nice solution to this? Corpus is in a separate repo but at the same time can be automatically discovered by go tool based on some convention?

[gopherbot]

Change https://golang.org/cl/167097 mentions this issue: tiff: add Fuzz function

[acln0]

I also have an interest in fuzzing as a first class citizen, so I have sent a CL to hopefully make the conversation more concrete.

[thepudds]

Earlier today, @dvyukov wrote in google/oss-fuzz#2188 (comment):

@thepudds proposed x/image, @FiloSottile proposed x/crypto at the first target. I don't think it matter much which one will be first. We can do both.

That makes sense to me. x/image might be a bit simpler, but personally I would be excited for any Fuzz function to land as a first example.

[thepudds]

@josharian Thank you for the reference to the conversation from the last 1-2 weeks in google/oss-fuzz#2188. I had not read that recent portion, but makes sense.

@FiloSottile Based on the comments from @dvyukov and the CL from @acln0, do you think it makes sense to proceed with this issue here?

[ianlancetaylor]

CC @nigeltao

[nigeltao]

General approach SGTM. I'm happy for people like @dvyukov or @josharian to review actual CLs.