[dev.fuzz] testing: crash due to incompatible type

[dsnet]

Using 529e5d0.

Consider this snippet:

func FuzzFoo(f *testing.F) {
	for _, td := range myTestdata {
		f.Add(0, []byte(td))
	}

	f.Fuzz(func(t *testing.T, seed int64, b []byte) {
		...
	})
}

Currently, this crashes with:

panic: reflect: Call using int as type int64 [recovered]
	panic: reflect: Call using int as type int64

goroutine 54 [running]:
testing.tRunner.func1.2(0x5fd600, 0xc00058e220)
	testing/testing.go:1153 +0x34b
testing.tRunner.func1(0xc0000f8300)
	testing/testing.go:1156 +0x4b6
panic(0x5fd600, 0xc00058e220)
	runtime/panic.go:972 +0x1d4
reflect.Value.call(0x603a20, 0x653690, 0x13, 0x64222a, 0x4, 0xc001c9c240, 0x3, 0x4, 0x4, 0x30, ...)
	reflect/value.go:409 +0x1917
reflect.Value.Call(0x603a20, 0x653690, 0x13, 0xc001c9c240, 0x3, 0x4, 0x2, 0x4, 0x786c00)
	reflect/value.go:338 +0xb9
testing.(*F).Fuzz.func1.1(0xc0000f8300)
	testing/fuzz.go:343 +0x225
testing.tRunner(0xc0000f8300, 0xc001c9c1e0)
	testing/testing.go:1203 +0xef
created by testing.(*F).Fuzz.func1
	testing/fuzz.go:338 +0x370

The cause of the crash is because testing.F.Add is called with an int, while the Fuzz function itself is expecting an int64.

Either, the testing framework should:

• report an with a more clearer message about what's wrong, or
• automatically convert assignable values.

Also, with generics on the horizon, is there an API design that would guarantee type safety between testing.F.Add and testing.F.Fuzz? It's probably better to statically prevent type errors like than to have it occur at runtime.

\cc @katiehockman @jayconrod

[katiehockman]

Ah thanks for testing and reporting this! There's probably something better we can do here, like you said. So I'll think about that more. At the very least we can improve the error messaging to say something more direct.

It would be great to be able to use generics at some point soon. It would likely help for cases like this. If we can't, then at the very least we can add go vet checks that can verify this at build time so it isn't caught at runtime.

[dsnet]

It would be great to be able to use generics at some point soon. It would likely help for cases like this.

Although, I'm not sure if it's going to be possible with the currently accepted generics proposal since it doesn't support variadic type parameters. A go vet check might be necessary.

@ianlancetaylor @griesemer

[mknyszek]

I put this in backlog for now, but is it a release blocker?

[katiehockman]

One tricky thing to consider is that we have to decide what our source of truth is if the arguments differ.

For example, consider these two fuzz targets:
a)

func FuzzFoo(f *testing.F) {
	f.Add(int32(0))
	f.Fuzz(func(t *testing.T, i int64) { ... })
}

b)

func FuzzFoo(f *testing.F) {
	f.Add(int64(0))
	f.Fuzz(func(t *testing.T, i int32) { ... })
}

Should we pick int64 values to fuzz and write to the corpus, since that can hold the most bytes, and is thus the most likely to be able to assign to? Or should we assume that the arguments in the f.Fuzz function are the source of truth, and if whatever is passed via f.Add or in testdata can't be cast to that type, then we fail the test? I think I'd prefer the latter.

Do you have an opinion on this?

[dsnet]

Or should we assume that the arguments in the f.Fuzz function are the source of truth, and if whatever is passed via f.Add or in testdata can't be cast to that type, then we fail the test? I think I'd prefer the latter.

I believe f.Fuzz should be authoritative with the rationale that f.Fuzz is given explicit types in the arguments list. On the other hand, the types given to f.Add are inferred by the values given.

[gopherbot]

Change https://golang.org/cl/325702 mentions this issue: [dev.fuzz] testing: convert seed corpus values where possible

[katiehockman]

I've been thinking about this a bit more, and at least for the first major release of fuzzing, I don't think we should be attempting to convert inputs of different types. I'd like to revert the part of my change that does the conversion.

I agree that the UX could be better even without doing this though: at the very least with a better error message (which I'm happy to do). I'm concerned that if this ends up causing problems later down the line, then the backwards compatibility promises are going to make us keep it around even when we don't want to. We can always make it less restrictive later, but we can't do the opposite.

@dsnet do you feel strongly about this either way?

@golang/fuzzing

[dsnet]

do you feel strongly about this either way?

I don't feel strongly, but I support a revert. I prefer stricter checks over automatic conversions that have surprising edge cases. It seems more aligned with the Go philosophy that requires manually casting integers of different widths rather than automatically casting them.

[gopherbot]

Change https://golang.org/cl/350251 mentions this issue: Revert "[dev.fuzz] testing: convert seed corpus values where possible"

[katiehockman]

I've gone ahead and reverted the change, and will close the issue now. If there are still changes we should make as far as printing a better error message, feel free to re-open or just file a new issue with those details.

[bcmills]

I wonder if it would make sense to restore the implicit conversions, but only from the default types of constants (bool, rune, int, float64, complex128, or string) to other types that are compatible with the same untyped constants.

That might give some of the ergonomic benefits of conversion for Add calls with constants (which are likely to be frequent), but without quite as many edge cases to worry about. (Integer overflow is still problematic, but at least float-to-integer conversion goes away!)

[bcmills]

(Either way, I think it's totally fine to keep the check strict for Go 1.18!)