proposal: add package containing fuzz function helpers

[josharian]

There are some common fuzzing patterns that:

• are really effective at finding bugs
• are non-obvious how to write correctly
• are non-obvious to people that are new to writing fuzz functions

I propose we add a std package containing some such helpers, perhaps with import path testing/fuzz or testing/fuzzcheck.

Here's a sample function:

// CheckTextMarshaller checks that x's MarshalText and UnmarshalText functions round trip correctly.
func CheckTextMarshaller(x encoding.TextMarshaler) {
	buf, err := x.MarshalText()
	if err == nil {
		return
	}
	y := reflect.New(reflect.TypeOf(x)).Interface().(encoding.TextUnmarshaler)
	err = y.UnmarshalText(buf)
	if err != nil {
		fmt.Printf("(%v).MarshalText() = %q\n", x, buf)
		panic(fmt.Sprintf("(%T).UnmarshalText(%q) = %v", y, buf, err))
	}
	if !reflect.DeepEqual(x, y) {
		fmt.Printf("(%v).MarshalText() = %q\n", x, buf)
		fmt.Printf("(%T).UnmarshalText(%q) = %v", y, buf, y)
		panic(fmt.Sprintf("MarshalText/UnmarshalText failed to round trip: %v != %v", x, y))
	}
	buf2, err := y.(encoding.TextMarshaler).MarshalText()
	if err != nil {
		fmt.Printf("(%v).MarshalText() = %q\n", x, buf)
		fmt.Printf("(%T).UnmarshalText(%q) = %v", y, buf, y)
		panic(fmt.Sprintf("failed to MarshalText a second time: %v", err))
	}
	if !bytes.Equal(buf, buf2) {
		fmt.Printf("(%v).MarshalText() = %q\n", x, buf)
		fmt.Printf("(%T).UnmarshalText(%q) = %v", y, buf, y)
		fmt.Printf("(%v).MarshalText() = %q\n", y, buf2)
		panic(fmt.Sprintf("second MarshalText differs from first: %q != %q", buf, buf2))
	}
}

(This would probably benefit from generics and/or a testing comparison function such as proposed in #45200.)

Other good helper functions include any kind of custom encoding method (binary encoding, JSON encoding, gob) and checks that String() methods don't panic. We could use this issue as a place to note other ideas.

We could even have a meta "CheckAll" function that uses type assertions and runs all relevant checks. This would make many common fuzz functions a few lines at most.

cc @katiehockman @dvyukov

[rsc]

This seems like a good idea, but it seems to me not inherently tied to fuzzing.

CheckTextMarshaler could be used to test any implementation, randomly generated or not, the same way that we have testing/fstest and testing/iotest: those are for testing arbitrary implementations. If we supplied a function that returns an error instead of panicking, then it could be used in other contexts besides fuzzing where it might be just as useful.

So the question is: how much are these things really specific to fuzzing? Should they be written in a more general way (in particular, returning an error instead of panicking) so that they can be used beyond that use case?

And what other kinds of helpers did you have in mind besides interface implementation checks?

I should add that putting everything into a 'package fuzztest' doesn't seem like it makes sense. We should want packages that define the interfaces to define the tests, or at least to have the tests defined near them, not in a central place.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[josharian]

And what other kinds of helpers did you have in mind besides interface implementation checks?

For a crypto/cipher.Block, does Encrypt/Decrypt round trip?

Given an http.Handler, feed it a wide variety of requests.

Dmitry might have other ideas. From the above, it looks like mostly interface implementation checks. :)

[kortschak]

It's entirely possible that people writing text marshalling/unmarshalling methods are doing to on type with unexported fields. Is there a way to capture that as well? or is that just something that should be documented as out of scope?

[dvyukov]

Does this work on the std lib types? Especially re Dan's concern re unexported fields.
If we are talking about fuzzing, shouldn't this accept []byte and start with marshaling it?

[josharian]

It's entirely possible that people writing text marshalling/unmarshalling methods are doing to on type with unexported fields. Is there a way to capture that as well?

This was the thinking behind my parenthetical in the OP: "(This would probably benefit from generics and/or a testing comparison function such as proposed in #45200.)"

With generics, we could use ==, which is a narrower definition than reflect.DeepEqual. #45200 would give us the tools to express a great many more notions of equality.

Does this work on the std lib types?

I believe so, as you wrote and ran fuzzers for most of them that used checks like this. :) But it'd be worth double-checking.

If we are talking about fuzzing, shouldn't this accept []byte and start with marshaling it?

I presume you mean unmarshaling.

Hmm, yes, good point. The advantage to writing it this way is that it is useful for other ways of constructing an object (e.g. table-driven tests). I'm not too worried about unexported fields, as I expect this would mainly be used from within the same package. I've been using prototypes of these helpers in netaddr, and I do a parse before calling these checks. But for fuzzing, doing the checks directly from bytes would be better.

[josharian]

Ah yes, the other advantage to writing it this way is if there are values that cannot be constructed via UnmarshalText, but can be constructed programatically. You want to be able to test those.

[dvyukov]

Then it indeed looks like what Russ said re being not-fuzzing-specific.

[dvyukov]

Dmitry might have other ideas.

Nothing comes to mind so far. This is indeed very common and useful pattern, but it frequently looked a bit differently due to small differences in interfaces (encoding/, compress/, image/*). And these round-trippers are not TextMarshaller nor BinaryMarshaller.
But there were also frequently some small quirks, like a custom check like "if data contains X, then equality will intentionally fail, so ignore it". Or some custom additional checks like "if image format is non-compressing, then we shouldn't get a huge image after decoding". That's why I asked about standard packages and if it's applicable as is at least to some set of types.

Overall I would wait until we have a set of cases where we know it's applicable as is, and if we introduce a new package I think we need some plan for having more than 1 function there (otherwise it may just go into testing maybe?).

[rsc]

It sounds like people agree that these are not really limited to fuzzing and that we should not tie them to it.

Today we have testing/iotest and testing/fstest. We could keep adding subdirectories of testing, but we might end up repeating the entire std hierarchy, and it doesn't set much of a pattern for external repos. So what should we do?

We could establish a convention of adding Check functions in the actual packages that define the interfaces, or in packages nearby (io/fs/fstest or image/imagetest or something like that). For interface-only packages which typically try to have very few dependencies (like encoding), we will want separate packages. For larger packages it might be fine to put the checkers into the main package, but it might also be nice to isolate them in subdirectories, just to keep the APIs separate.

We already have net/http/httptest, and the subdirectory there was not motivated by trying to keep net/http's dependencies light. Maybe we should follow that example and just have foo/footest for checkers and other helpers?

[FiloSottile]

Should they be written in a more general way (in particular, returning an error instead of panicking) so that they can be used beyond that use case?

Something I hope will come from the native fuzzing support is getting over the idea that fuzzers should panic. A panic also leads to a test failure, but we use t.Fatal rather than panic. The only reason fuzzers today use panics are tooling limitations, which are going away with native support.

Testing toolkits and interface tests are great, and should return errors, so they can be used both with tests and with fuzz targets.

For a crypto/cipher.Block, does Encrypt/Decrypt round trip?

I have a collection of these is the golang.org/x/crypto/cryptotest package I never finished :)

[rsc]

It sounds like we are converging on using foo/footest as the place to put testing helpers, like net/http/httptest.
testing/iotest and testing/fstest do not follow this convention and should have been io/iotest and io/fs/fstest, but too late for them.
And also that the testing helpers should return errors, not panic.

Is that the general consensus?

If so, then probably this specific proposal should be declined and we can discuss specific test packages and APIs in other proposal issues.

[josharian]

SGTM. I would like to add that this is the proposal process at its best: My rather crude idea has, with input from others, been shaped into something far better.

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group