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

[thepudds]

Summary

In #30719 and #30979, dvyukov/go-fuzz compatible fuzz functions were landed in:

• the standard library in src/image/png/fuzz.go
• golang.org/x in golang.org/x/image/tiff/fuzz.go

That was done primarily to help with the exploration requested by the core Go team in discussion of the #19109 proposal to "make fuzzing a first class citizen" (in addition to other benefits).

The follow-up suggestion here is to add a Fuzz function to the standard library encoding/json package.

The starting point most likely should be the Fuzz function in https://github.com/dvyukov/go-fuzz-corpus/blob/master/json/json.go.

This is a particularly interesting example given the Fuzz function there is a bit more complex than the first two samples landed, and hopefully there is some value especially given @mvdan has recently been landing a nice series of performance-related changes to encoding/json.

There would be no corpus checked in for now. (The approach to the corpus is being discussed elsewhere, e.g., #31215).

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, new dependencies should be avoided, especially with the introduction of modules.

For reference, here is a gist showing the diff between dvyukov/go-fuzz-corpus/tiff/tiff.go and the final form as merged into x/image/tiff.

Two issues that likely would need to be resolved prior to merging into the standard library:

1. The current dvyukov/go-fuzz-corpus json fuzzing function has a dependency on github.com/dvyukov/go-fuzz-corpus/fuzz for fuzz.DeepEqual. This dependency would need to be eliminated. An initial solution might be (a) eliminating that round-trip test for now, or (b) the longer term solution might be open coding the comparison (e.g., comment from @dvyukov in image: add sample fuzz tests for prototype of "fuzzing as a first class citizen" #30979 (comment)) or perhaps rely on an existing comparison function in an existing _test.go or similar, or some other solution.

2. The current dvyukov/go-fuzz-corpus json fuzzing function can trigger a false positive when round-tripping through Unmarshal/Marshal in the presence of duplicate keys (e.g., see comments from @josharian in Golang internal library fuzzers google/oss-fuzz#2188 (comment) or json: exclude panics from duplicate keys dvyukov/go-fuzz-corpus#3). An initial solution might be (a) temporarily eliminating that round-trip test, or (b) perhaps scan the serialized JSON for duplicate keys to avoid the false positive, or other possible longer term solutions.

Happy to be corrected if any of the above is different than how people would like to proceed here.

Finally, @mvdan, I don't want to put you on the spot, but in other discussions you had expressed some interest in this. Are you still interested? And of course no worries if too busy with other things.

CC @dvyukov @josharian @FiloSottile @acln0

[mvdan]

This sounds like a good idea, and I was planning on fuzzing the decoder before the 1.13 release in any case.

Are you still interested? And of course no worries if too busy with other things.

Sorry, could you clarify what part you want me to play here? I'm happy to help review and bounce ideas, for example.

[elwinar]

@thepudds I haven't read the background on everything in the above issues, but I would be willing to contribute time on this. Let me know if I can help.

[thepudds]

@elwinar That would be great! There is plenty of opportunity to help make this happen, including in some small-ish steps that individually would not be much time, I think.

The main goal of this issue here is to bring in https://github.com/dvyukov/go-fuzz-corpus/blob/master/json/json.go into the standard library encoding/json package.

However, for this issue it probably would make sense to first do a small-ish step for encoding/json, which would be still be a nice win.

The opening comment here in #31309 (comment) outlines two problems with the current JSON fuzzing function in dvyukov/go-fuzz-corpus. Fortunately, both of those problems are related to the round-trip test. Quoting that comment partially here:

1. The current dvyukov/go-fuzz-corpus json fuzzing function has a dependency on github.com/dvyukov/go-fuzz-corpus/fuzz for fuzz.DeepEqual ...
   ...
2. The current dvyukov/go-fuzz-corpus json fuzzing function can trigger a false positive when round-tripping through Unmarshal/Marshal in the presence of duplicate keys ...

The "simple" solution to get the ball rolling would just be to eliminate that round-trip test from the standard library version when https://github.com/dvyukov/go-fuzz-corpus/blob/master/json/json.go is initially converted over. The current pattern is basically Unmarshal, Marshal, Unmarshal, DeepEqual. The new pattern could likely just be Unmarshal, Marshal.

Does that sound of interest? It would be very nice to get even a basic initial version of a Fuzz function for JSON into the standard library...

(And sorry, I should have updated this issue regarding @mvdan's question above, but @mvdan and I ended up chatting elsewhere about this issue here, and he more-or-less said that he has long-term interest here, but that he was already maxed out for anything in a 1.13 timeframe).

[elwinar]

I'll have a look at it.

[gopherbot]

Change https://golang.org/cl/174058 mentions this issue: encoding/json: add a Fuzz function

[elwinar]

In the end, the function worked well with the standard DeepEqual. I ran it for half an hour without noticing any crash, which I guess is fine (but I have only 12 cores, so maybe a real fuzzing farm would find a crash).

I didn't look into the second issue (the duplicate key thing) yet.

[elwinar]

On the way, I studied the Fuzz function itself, and removed a few things. I will possibly add them back later, but I believe the conditions about the json.RawMessage thing aren't necessary any longer since json.RawMessage doesn't do funky things with base64 anymore.

[elwinar]

Also, switched the second Unmarshal error to a panic, as I believe anything that was marshaled should be unmarshaled without error.

[elwinar]

(And now I'll stop spamming and go back to work.)

[thepudds]

@elwinar That is very exciting.

In general, I think your philosophy of simplifying makes sense, especially to start. Going from 0 to 1 Fuzz function for JSON in the standard library is the biggest step, and improvements can happen later, as you say.

As part of that "start simple" approach you are taking, I would suggest removing the DeepEqual check.

Part of the overall goal is to make sure the initial round of Fuzz functions that land the standard library are high quality, especially with regards to avoiding spurious false positives. Right now, especially as we are starting, avoiding false positives is more important than the sophistication of any given fuzzing function. The duplicate key issue has triggered false positives in practice (e.g., see dvyukov/go-fuzz-corpus#3), and removing the DeepEqual check here is hopefully a trivial way to avoid those false positives for now, and a more sophisticated solution to avoid tripping over duplicate keys could come later (including perhaps we could even open a separate issue for that after your CL lands).

What do you think?

[elwinar]

I've been toying with this duplicate key issue because it seemed really annoying.

I note that it is only a problem for the S struct, and it depends on the number of indirection levels for the O field. You can play with and example here: https://play.golang.org/p/2tAoh8TG8if

Looking at dvyukov/go-fuzz-corpus#3 I agree that this input is perfectly valid JSON, but I think in this case the parser behavior is kinda inconsistent. I would expect the field to either always be nil on the first indirection or the last, while right now it depends on what was previously in the field.

Removing the DeepEqual check and opening an issue for this seems fine for now.

[thepudds]

Removing the DeepEqual check and opening an issue for this seems fine for now.

OK, great. I am happy to open up the follow-on issue if that makes sense (and to some degree it is already being tracked in dvyukov/go-fuzz-corpus#3).