SIGSEGV: panic: runtime error: invalid memory address or nil pointer dereference in v1.15.2

[mprimeaux]

I've opened up grpcurl issue 414, which was recently released with support for protoreflect 1.15.2.

It appears a defect was introduced in protoreflect version 1.5.2 resulting in the following panic:

❯ cat mutate/challenge.action.add.json |  -plaintext -vv -d @ -format=json -use-reflection=false -proto ../internal/api/hydra_service.proto localhost:50503 api.Hydra.Mutate
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x28 pc=0x100f714e8]

goroutine 1 [running]:
github.com/jhump/protoreflect/desc/protoparse.parseToProtoRecursive({0x101406c80, 0x14000195308}, {0x140001bf8e0, 0x1c}, 0x1400025f608?, 0x100f08bf4?, 0x0?)
    github.com/jhump/protoreflect@v1.15.2/desc/protoparse/parser.go:389 +0x148
github.com/jhump/protoreflect/desc/protoparse.parseToProtoRecursive.func1(0x14000365380, 0x140003b2eb0, 0x140001e3810, {0x101406c80, 0x14000195308}, 0x1013ba560?, 0x140001d9f01?)
    github.com/jhump/protoreflect@v1.15.2/desc/protoparse/parser.go:401 +0x164
github.com/jhump/protoreflect/desc/protoparse.parseToProtoRecursive({0x101406c80, 0x14000195308}, {0x16f76a75a, 0x23}, 0x14000361400?, 0x0?, 0x1400025f7b8?)
    github.com/jhump/protoreflect@v1.15.2/desc/protoparse/parser.go:402 +0x1d8
github.com/jhump/protoreflect/desc/protoparse.parseToProtosRecursive({0x101406c80, 0x14000195308}, {0x140002fed70, 0x1, 0x0?}, 0x0?, 0x0?)
    github.com/jhump/protoreflect@v1.15.2/desc/protoparse/parser.go:365 +0x80
github.com/jhump/protoreflect/desc/protoparse.Parser.ParseFiles({{0x0, 0x0, 0x0}, 0x1, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, ...}, ...)
    github.com/jhump/protoreflect@v1.15.2/desc/protoparse/parser.go:153 +0x210
github.com/fullstorydev/grpcurl.DescriptorSourceFromProtoFiles({0x0, 0x0, 0x0}, {0x140002fed70?, 0x1400025fb18?, 0x1006a3018?})
    github.com/fullstorydev/grpcurl/desc_source.go:71 +0xbc
main.main()
    github.com/fullstorydev/grpcurl/cmd/grpcurl/grpcurl.go:501 +0xca8

The previous version of protoreflect processed the same .proto file successfully.

❯ cat mutate/challenge.action.add.json | ~/Downloads/grpcurl -plaintext -vv -d @ -format=json -use-reflection=false -proto ../internal/api/hydra_service.proto localhost:50503 api.Hydra.Mutate

Resolved method descriptor:
// Mutates one or more underlying data structures.
rpc Mutate ( .api.MutateRequest ) returns ( .api.MutateResponse );

Request metadata to send:

Response headers received:
content-type: application/grpc

Estimated response size: 50 bytes

Response contents:
{
  "data": [
    {
          "id": "6789a357-0d07-4cf9-8025-2438d7e639f2"
        }
  ]
}

Response trailers received:
(empty)
Sent 1 request and received 1 response

I'll see if I can find time to debug but before I do, I wanted to get this in front of folks who know the protoreflect code base better than I do at this juncture.

Your help and insights are greatly appreciated.

[mprimeaux]

As an update, I pulled down the protoreflect code and wrote a new test using the same failing .proto file and it parsed it just fine; no SIGSEGV panic.

After further debugging, I was able to trip the exact panic by setting InferImportPaths to true on the protoparse.Parser. I'll update this issue once I've identified the cause.

Also, I updated the related grpcurl issue since the currently workaround is to specify the -import-path option.

[mprimeaux]

I believe the fix is to the function parseToProtosRecursive in parser.go here by adding a nil check for astRoot.

func parseToProtoRecursive(res protocompile.Resolver, filename string, rep *reporter.Handler, srcPosAddr *SourcePos, results map[string]parser.Result) {
    if _, ok := results[filename]; ok {
        // already processed this one
        return
    }
    results[filename] = nil // placeholder entry

    astRoot, parseResult, _ := parseToAST(res, filename, rep)
    if rep.ReporterError() != nil {
        return
    }
    if parseResult == nil {
        parseResult, _ = parser.ResultFromAST(astRoot, true, rep)
        if rep.ReporterError() != nil {
            return
        }
    }
    results[filename] = parseResult

+   if astRoot == nil {
+       return
+   }

    for _, decl := range astRoot.Decls {
        imp, ok := decl.(*ast2.ImportNode)
        if !ok {
            continue
        }
        func() {
            orig := *srcPosAddr
            *srcPosAddr = astRoot.NodeInfo(imp.Name).Start()
            defer func() {
                *srcPosAddr = orig
            }()

            parseToProtoRecursive(res, imp.Name.AsString(), rep, srcPosAddr, results)
        }()
        if rep.ReporterError() != nil {
            return
        }
    }
}

[mprimeaux]

Looks like the panic is triggered when an import line is encountered in the .proto file. For example,

syntax = "proto3";
import "google/protobuf/struct.proto";

To reproduce this, I modified the existing test named TestBuilder_PreserveAllCommentsAfterBuild in builder_test.go that was passing before with the above import and now it fails with the exact panic.

The decl.(*ast2.ImportNode) cast here assumes the import is for a local file and doesn't seem to handle the Google protobuf types.

After I add the nil check as in my previous comment the failing test passes.

I'll leave this here until @jhump or another maintainer has an opportunity to review. I still think the nil check above might be a viable option.

[srebhan]

@mprimeaux are you willing to submit a PR for the issue as you already do have the code ready!? We are hitting the same issue in Telegraf...

[mprimeaux]

@srebhan Sure. I'll submit a PR shortly.

[jhump]

@mprimeaux, I'm working on it. Just checking for a nil AST isn't quite enough. A better fix will handle a lack of AST by using the descriptor in the parse result to recursively crawl through imports.

I also realized that the InferImportPaths never had a proper test, which is why this bug has been lurking for so long (certainly broken in v1.15.0). In adding a test, I see a few other issues (unrelated to the panic you found), and am fixing those, too. So the next release will fix the panic as well as improve the behavior of the InferImportPaths flag.

[mprimeaux]

@jhump Sounds great and thanks for your help. Shall I just cancel my PR since you're got a handle on it?

[jhump]

Fixed in #575.