SIGSEGV: panic: runtime error: invalid memory address or nil pointer dereference in v1.15.2 about protoreflect HOT 8 CLOSED

@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.

from protoreflect.

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.

from protoreflect.

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
		}
	}
}

from protoreflect.

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.

from protoreflect.

@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...

from protoreflect.

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

from protoreflect.

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

from protoreflect.

Fixed in #575.

from protoreflect.