Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

presence of an interface value always panics #27

Open
odeke-em opened this issue Feb 18, 2019 · 20 comments · May be fixed by #68
Open

presence of an interface value always panics #27

odeke-em opened this issue Feb 18, 2019 · 20 comments · May be fixed by #68

Comments

@odeke-em
Copy link

Hello there, thank you for this project!

I am currently looking at using it to fuzz inputs to https://github.com/census-instrumentation/opencensus-service

However, if a struct has a field with a value whose type is an interface, regardless of if that field is set or not. For example

type A struct {
            Name string
            Debug bool
            Err error
}
a := new(A)
f.Fuzz(a)

gofuzz will always crash with

On no value set

panic: Can't handle <nil>: interface [recovered]
	panic: Can't handle <nil>: interface

On value set

panic: Can't handle &errors.errorString{s:"Foo"}: interface [recovered]
	panic: Can't handle &errors.errorString{s:"Foo"}: interface

Perhaps we could skip trying to mutate interface values that we don't know about and let tests proceed normally, otherwise it becomes unproductive to debug this cryptic error.

Thank you.

@howardjohn
Copy link

Any update? This makes this infeasible to use for any testing using protobuf enums

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

Hi, sorry, I must have missed this in my email-- can you not register a custom fuzzing function for an interface{} type? Alternatively, register one for the enclosing struct. Let me know if that doesn't work.

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

It's unfortunately not generally possible to know what makes sense for interfaces, functions, or channels. :(

The panic message is not very helpful, though--that should be improved.

@howardjohn
Copy link

type Test struct {
	ErrorType TestInterface
}

type TestInterface interface {
	Size() int
}

func TestFuzzE(t *testing.T) {
	x := Test{}
	f := fuzz.New().NilChance(.5).Funcs(
		func(e *TestInterface, c fuzz.Continue) {
		})
	f.Fuzz(&x)
	f1 := fuzz.New().NilChance(.5).Funcs(
		func(e *interface{}, c fuzz.Continue) {
		})
	f1.Fuzz(&x)
}

First one works, second one doesn't. It doesn't seem to get called as a custom function. I'll play around with it a bit more to see if I can get something to stick. Basically my plan was to have a function called for every type, and filter out the interfaces or similar

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

Yeah, I'd expect your second one to get called for this struct:

type Test struct {
	ErrorType interface{}
}

It kinda has to work this way, since only you know how to construct reasonable random implementations of your interface.

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

Or are you asking for a second lookup so you can set the behavior for custom interfaces without a specific custom fuzzing function?

@howardjohn
Copy link

Basically my understanding is right now the presence of any interface type leads to a panic. The only way to avoid this is to define a custom function (which does work).

Ideally I would not need to define custom functions as I have a lot of interfaces and I would rather be lazy 🙂. Instead I would expect to basically be able to skip any interface types. So instead of panicking, we would just do nothing to that field. Then if you do want to add custom logic for that type, you can define your own function. This could be a configurable option if that is not generally desirable?

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

A goal is to not give false confidence, by e.g. silently not fuzzing things. I guess it makes sense to have an opt-out. A second, fallback call like I just suggested doesn't actually make sense, since the interfaces are of different types.

What if the panic message listed the types that are missing fuzzing functions, in a form that you could copy-paste that into your test file with your custom fuzzing functions?

@howardjohn
Copy link

A helpful error message + some doc on resolving it seems reasonable to me

@lavalamp
Copy link
Contributor

lavalamp commented Jul 7, 2020

OK, great. I'm totally open to a PR fixing this. I probably won't have time to write one anytime soon, though.

@howardjohn
Copy link

I was thinking about

It kinda has to work this way, since only you know how to construct reasonable random implementations of your interface.

a bit more. One common case for this to occur is using protobuf oneof. I wonder if its feasible to implement awareness of protobuf and automatically handle these cases.

@lavalamp
Copy link
Contributor

lavalamp commented Dec 4, 2020

How could one programmatically identify that arrangement? E.g. is it represented in struct tags? I'm open to ideas.

@howardjohn
Copy link

	// Types that are valid to be assigned to HealthCheckMethod:
	//	*ReadinessProbe_HttpGet
	//	*ReadinessProbe_TcpSocket
	//	*ReadinessProbe_Exec
	HealthCheckMethod    isReadinessProbe_HealthCheckMethod `protobuf_oneof:"health_check_method"`

this is the top level struct, it has a protobuf_oneof tag.

Then there is a helper function:

func (*ReadinessProbe) XXX_OneofWrappers() []interface{} {
	return []interface{}{
		(*ReadinessProbe_HttpGet)(nil),
		(*ReadinessProbe_TcpSocket)(nil),
		(*ReadinessProbe_Exec)(nil),
	}
}

which seems to return all possible types

I am not sure how much these are meant to be implementation details though, it might be better to use https://godoc.org/google.golang.org/protobuf/reflect/protoreflect

@lavalamp
Copy link
Contributor

lavalamp commented Dec 4, 2020

gofuzz operates strictly on go types, so doing something proto specific doesn't seem feasible.

Given the setup you posted, I can imagine a fuzz function that operates on interface types, looks at the name of the type, somehow looks for a corresponding XXX_something() []interface{} method, calls it, picks one of the choices (or nil, according to the nil chance).

@tzachshabtay
Copy link

@lavalamp maybe I'm missing something, but I have the same issue with fuzzing proto oneof.
I can't do the suggested workaround to solve this, i.e I can't do a custom fuzz function on that interface because it is non-exported (i.e see the example posted above by @howardjohn , isReadinessProbe_HealthCheckMethod is private).

I tried declaring my own interface with the same shape but that didn't work.

Any ideas?

@freman
Copy link

freman commented Jul 27, 2022

I've just run into this without the protobuf problem, would a struct tag, say nofuzz, feasible?

@lavalamp
Copy link
Contributor

lavalamp commented Aug 1, 2022

BTW go has built-in fuzzing now: https://go.dev/doc/fuzz/

@jackgene
Copy link

BTW go has built-in fuzzing now: https://go.dev/doc/fuzz/

It does, but the fuzzing it does is limited, and I still rely on gofuzz. For instance, since Go 1.18's fuzzer doesn't fuzz structs, I still do:

func FuzzWithStruct(f *testing.F) {
	f.Add([]byte("seed"))
	f.Fuzz(func(t *testing.T, data []byte) {
		// Set up
		fuzzer := fuzz.NewFromGoFuzz(data)
		testSet := map[PIIField]struct{}{}
		fuzzer.Fuzz(&testSet)

		t.Logf("Set: %+v", testSet)

		// Test
		_ = doThingWithSet(testSet)

		// Verify
		// The point of the fuzz is just to ensure method does not panic
	})
}

@lavalamp
Copy link
Contributor

@jackgene you are right, that's more limited than I realized.

@freman Yeah if there's a better idea than struct tags I'm not seeing it. Not sure what to do about e.g. map[string]interface{} though.

@aviganguly
Copy link

Hi guys. Will there be a release on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants