nil panic in FieldsOnCorrectTypeRule

[titanous]

I'm fuzzing the graphql parser with go-fuzz, and I found an issue that I'm not sure about the best option to fix.

This test panics:

func TestValidate_MutationPanic(t *testing.T) {
       testutil.ExpectFailsRule(t, graphql.FieldsOnCorrectTypeRule, `mutation{o}`,
               []gqlerrors.FormattedError{
                       testutil.RuleError(``, 0, 0),
               },
       )
}

--- FAIL: TestValidate_MutationPanic (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x7fb7f]

goroutine 346 [running]:
panic(0x318800, 0xc82000a100)
    /Users/titanous/src/go-tip/src/runtime/panic.go:481 +0x3e6
testing.tRunner.func1(0xc8203a66c0)
    /Users/titanous/src/go-tip/src/testing/testing.go:467 +0x192
panic(0x318800, 0xc82000a100)
    /Users/titanous/src/go-tip/src/runtime/panic.go:443 +0x4e9
github.com/graphql-go/graphql.(*Object).Name(0x0, 0x0, 0x345940)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/definition.go:431 +0x1f
github.com/graphql-go/graphql.FieldsOnCorrectTypeRule.func1(0x32c1a0, 0xc820439e00, 0x2ccd60, 0xc8202ba180, 0x0, 0x0, 0xc820207800, 0x5, 0x8, 0xc8202b8440, ...)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/rules.go:236 +0x758
github.com/graphql-go/graphql/language/visitor.VisitInParallel.func1(0x32c1a0, 0xc820439e00, 0x2ccd60, 0xc8202ba180, 0x0, 0x0, 0xc820207800, 0x5, 0x8, 0xc8202b8440, ...)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/language/visitor/visitor.go:734 +0x1fd
github.com/graphql-go/graphql/language/visitor.VisitWithTypeInfo.func1(0x32c1a0, 0xc820439e00, 0x2ccd60, 0xc8202ba180, 0x0, 0x0, 0xc820207800, 0x5, 0x8, 0xc8202b8440, ...)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/language/visitor/visitor.go:783 +0x182
github.com/graphql-go/graphql/language/visitor.Visit(0x60a0b0, 0xc8203c0690, 0xc8203c08d0, 0x0, 0x0, 0x0)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/language/visitor/visitor.go:400 +0x141e
github.com/graphql-go/graphql.VisitUsingRules(0xc8200a32c0, 0xc820207700, 0xc8203c0690, 0xc8201d7e60, 0x1, 0x1, 0x0, 0x0, 0x0)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/validator.go:71 +0x490
github.com/graphql-go/graphql.ValidateDocument(0xc8200a32c0, 0xc8203c0690, 0xc8201d7e60, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/validator.go:47 +0x3ab
github.com/graphql-go/graphql/testutil.expectInvalidRule(0xc8203a66c0, 0xc8200a32c0, 0xc8201d7e60, 0x1, 0x1, 0x3699e0, 0xb, 0xc8203c05d0, 0x1, 0x1)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/testutil/rules_test_harness.go:507 +0x2f0
github.com/graphql-go/graphql/testutil.ExpectFailsRule(0xc8203a66c0, 0x3fcc48, 0x3699e0, 0xb, 0xc8203c05d0, 0x1, 0x1)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/testutil/rules_test_harness.go:532 +0xcc
github.com/graphql-go/graphql_test.TestValidate_FooBar(0xc8203a66c0)
    /Users/titanous/src/gopkgs/src/github.com/graphql-go/graphql/rules_fields_on_correct_type_test.go:218 +0x170
testing.tRunner(0xc8203a66c0, 0x50e0d0)
    /Users/titanous/src/go-tip/src/testing/testing.go:473 +0x98
created by testing.RunTests
    /Users/titanous/src/go-tip/src/testing/testing.go:582 +0x892

The problem is subtle, the nil check of the ParentType doesn't take into account that nil can be typed, in this case it is a (*Object)(nil). I noticed that there is a properly implemented check for a nil Composite in the codebase already. Should that be turned into a function? Is it likely that this bug exists elsewhere? If so is there a good strategy for eliminating it entirely?

I'm just getting started with this codebase, so I'm not sure what the best strategy to check for and eliminate this problem entirely is.

[titanous]

Actually, after looking at that "correct" type check that I linked closer, it is wrong as well, as the type asserted value is not nil checked.

[bigdrum]

Hello @titanous, do you mind sharing your setup for go-fuzz? I would like to run it for my fork.

[titanous]

Sure: https://github.com/titanous/graphql-fuzz

Please contribute back any additions to the corpus or other changes that are effective!

The current corpus was built over about 1 billion execs, initially seeded with a couple queries and a dump of all strings quoted in backticks from the graphql codebase (random graphql queries, but there is also some random Go code and other stuff because I didn't do any filtering).