Repository: jhump/protoreflect (go)

Issue:
ParseFilesButDoNotLink panics in option validation with invalid .proto
Feel free to tell me that I’m holding it wrong, but I figured I’d report this issue.

We have a pipeline with which we analyze all .proto files in our repo. The pipeline parses proto files using the `InterpretOptionsInUnlinkedFiles` and `IncludeSourceCodeInfo` options, so that we can work with some custom options we’re using. 

I noticed that the pipeline panic'd when processing a specific .proto file, but that behavior only started happening recently.

I distilled it down into the following test case:

```go
func TestOptionsValidation(t *testing.T) {
	testCases := []struct {
		contents string
		succeeds bool
		errMsg   string
	}{
		{
			contents: `syntax = "proto2";
package foo;
option malformed_non_existant = true;
option features.utf8_validation = NONE;
`,
			errMsg: `bleh`,
		},
	}

	for i, tc := range testCases {
		_, err := Parser{
			Accessor: FileContentsFromMap(map[string]string{"test.proto": tc.contents}),
			//ValidateUnlinkedFiles:           true,
			InterpretOptionsInUnlinkedFiles: true,
			IncludeSourceCodeInfo:           true,
		}.ParseFilesButDoNotLink("test.proto")
		if tc.succeeds {
			testutil.Ok(t, err, "case #%d should succeed", i)
		} else {
			testutil.Nok(t, err, "case #%d should fail", i)
			testutil.Eq(t, tc.errMsg, err.Error(), "case #%d bad error message", i)
		}
	}
}
```

…which panics with the following backtrace:

```
go test -run=OptionsVal
--- FAIL: TestOptionsValidation (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x7525ec]

goroutine 6 [running]:
testing.tRunner.func1.2({0xa4a540, 0x107f7a0})
	/usr/lib/go-1.22/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/usr/lib/go-1.22/src/testing/testing.go:1634 +0x377
panic({0xa4a540?, 0x107f7a0?})
	/usr/lib/go-1.22/src/runtime/panic.go:770 +0x132
github.com/bufbuild/protocompile/options.findOptionNode[...].func1({0xc11430, 0xc000013c68})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:994 +0x6c
github.com/bufbuild/protocompile/options.(*optionsRanger).Range.optionsRanger.Range.func1(0x10?)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:1018 +0x1f
github.com/bufbuild/protocompile/ast.(*FileNode).RangeOptions(0x413fc5?, 0xc000362d50)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/ast/file.go:142 +0x58
github.com/bufbuild/protocompile/options.optionsRanger.Range(...)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:1017
github.com/bufbuild/protocompile/options.findOptionNode[...]({0xc0003809a8, 0x2, 0x2}, {0xc0cce0, 0xc000362d20}, 0xc000362d30)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:992 +0x103
github.com/bufbuild/protocompile/options.(*interpreter).findOptionNode(0xc00032c6c0, {0xc0003809a8, 0x2, 0x2}, {0xc0c3a0?, 0xc000328600?})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:970 +0x116
github.com/bufbuild/protocompile/options.(*interpreter).validateFeatureSupport(0xc00032c6c0, 0x0, 0xc0003a6690, {0xb183fd, 0x5}, {0xc0001504e4, 0x2a}, {0xc0003809a8, 0x2, 0x2}, ...)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:937 +0x9c
github.com/bufbuild/protocompile/options.(*interpreter).validateRecursive.func1({0xc21788, 0xc0001448f0}, {{}, 0xa0cb80?, 0x0?, 0xa0bec0?})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:831 +0x6d6
google.golang.org/protobuf/internal/impl.(*messageState).Range(0xc00032c840, 0xc00032c900)
	/home/stapelberg/go/pkg/mod/google.golang.org/protobuf@v1.34.2-0.20240529085009-ca837e5c658b/internal/impl/message_reflect_gen.go:51 +0x1e3
github.com/bufbuild/protocompile/options.(*interpreter).validateRecursive(0xc00032c6c0, 0x0, {0xc1d810, 0xc00032c840}, {0xc0003809b0, 0x9}, {0xc0c3a0, 0xc000328600}, {0xc0003809a8, 0x1, ...}, ...)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:799 +0x1cc
github.com/bufbuild/protocompile/options.(*interpreter).validateRecursive.func1({0xc21788, 0xc000147270}, {{}, 0xafa3a0?, 0xc00032c840?, 0x0?})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:900 +0xca5
google.golang.org/protobuf/internal/impl.(*messageState).Range(0xc0002a85a0, 0xc00032c8a0)
	/home/stapelberg/go/pkg/mod/google.golang.org/protobuf@v1.34.2-0.20240529085009-ca837e5c658b/internal/impl/message_reflect_gen.go:51 +0x1e3
github.com/bufbuild/protocompile/options.(*interpreter).validateRecursive(0xc00032c6c0, 0x0, {0xc1d810, 0xc0002a85a0}, {0x0, 0x0}, {0xc0c3a0, 0xc000328600}, {0x0, 0x0, ...}, ...)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:799 +0x1cc
github.com/bufbuild/protocompile/options.(*interpreter).interpretOptions(0xc00032c6c0, {0xb1cebe, 0xa}, 0x1, {0xc0c3a0, 0xc000328600}, {0xc0c3e0, 0xc0002a82d0}, {0xc000094818, 0x1, ...}, ...)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:653 +0xaa5
github.com/bufbuild/protocompile/options.interpretElementOptions[...](0xc00032c6c0, {0xb1cebe, 0xa}, 0x1083c00, 0xc000328600, 0x1)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:538 +0x3e5
github.com/bufbuild/protocompile/options.(*interpreter).interpretFileOptions(0xc00032c6c0, {0xc1de48?, 0xc0003628e0?}, 0x1)
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:159 +0xf2
github.com/bufbuild/protocompile/options.interpretOptions(0x1, {0xc1de48, 0xc0003628e0}, {0x0, 0x0}, 0xc0003844c0, {0x0, 0x0, 0x0?})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:147 +0x1f0
github.com/bufbuild/protocompile/options.InterpretUnlinkedOptions({0xc1db10?, 0xc0000b11c0?}, {0x0, 0x0, 0x0})
	/home/stapelberg/go/pkg/mod/github.com/bufbuild/protocompile@v0.14.0/options/options.go:125 +0xfe
github.com/jhump/protoreflect/desc/protoparse.Parser.ParseFilesButDoNotLink({{0x0, 0x0, 0x0}, 0x0, 0x0, 0x0, 0xc000362750, 0x1, 0x0, 0x1, ...}, ...)
	/home/stapelberg/go/src/github.com/jhump/protoreflect/desc/protoparse/parser.go:281 +0x3bb
github.com/jhump/protoreflect/desc/protoparse.TestOptionsValidation(0xc0000fdba0)
	/home/stapelberg/go/src/github.com/jhump/protoreflect/desc/protoparse/validate_test.go:437 +0x228
testing.tRunner(0xc0000fdba0, 0xb697b0)
	/usr/lib/go-1.22/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 1
	/usr/lib/go-1.22/src/testing/testing.go:1742 +0x390
exit status 2
FAIL	github.com/jhump/protoreflect/desc/protoparse	0.045s

```

Notably, it works with github.com/bufbuild/protocompile v0.13.0, but panics only when I `go get github.com/bufbuild/protocompile@v0.14.0`.

Also, the panic disappears when I additionally enable `ValidateUnlinkedFiles`, presumably because execution doesn’t get as far anymore in that case.

Is it expected that we need to enable `ValidateUnlinkedFiles` in order for option interpretation to work, or is this a regression?

Thanks 

Pull request:
Update protocompile, which fixes possible panic in ParseButDontLink
This resolves #620. Adds the repro test case to prevent regressions. (The repro case in that bug report was also added to test cases in the underlying protocompile package [here](https://github.com/bufbuild/protocompile/commit/0142a0715e5fc3929c67a99e0a0534a4d11b4a8d#diff-58e3ae23c6156427aa2282edbe860d5742047bc68679cd616b4121286942612dR257-R274)).

Patch:
diff --git a/go.mod b/go.mod
index 5c7d732..d14a1d2 100644
--- a/go.mod
+++ b/go.mod
@@ -1,19 +1,19 @@
 module github.com/jhump/protoreflect
 
-go 1.19
+go 1.21
 
 require (
-	github.com/bufbuild/protocompile v0.13.0
+	github.com/bufbuild/protocompile v0.14.1
 	github.com/golang/protobuf v1.5.4
 	github.com/jhump/gopoet v0.1.0
 	github.com/jhump/goprotoc v0.5.0
 	google.golang.org/grpc v1.61.0
-	google.golang.org/protobuf v1.34.1
+	google.golang.org/protobuf v1.34.2
 )
 
 require (
 	golang.org/x/net v0.25.0 // indirect
-	golang.org/x/sync v0.7.0 // indirect
+	golang.org/x/sync v0.8.0 // indirect
 	golang.org/x/sys v0.20.0 // indirect
 	golang.org/x/text v0.15.0 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20231106174013-bbf56f31fb17 // indirect
diff --git a/go.sum b/go.sum
index 2076e60..2dae5cb 100644
--- a/go.sum
+++ b/go.sum
@@ -1,12 +1,13 @@
 cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
-github.com/bufbuild/protocompile v0.13.0 h1:6cwUB0Y2tSvmNxsbunwzmIto3xOlJOV7ALALuVOs92M=
-github.com/bufbuild/protocompile v0.13.0/go.mod h1:dr++fGGeMPWHv7jPeT06ZKukm45NJscd7rUxQVzEKRk=
+github.com/bufbuild/protocompile v0.14.1 h1:iA73zAf/fyljNjQKwYzUHD6AD4R8KMasmwa/FBatYVw=
+github.com/bufbuild/protocompile v0.14.1/go.mod h1:ppVdAIhbr2H8asPk6k4pY7t9zB1OU5DoEw9xY/FUi1c=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk=
@@ -32,6 +33,7 @@ github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
 github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/jhump/gopoet v0.0.0-20190322174617-17282ff210b3/go.mod h1:me9yfT6IJSlOL3FCfrg+L6yzUEZ+5jW6WHt4Sk+UPUI=
 github.com/jhump/gopoet v0.1.0 h1:gYjOPnzHd2nzB37xYQZxj4EIQNpBrBskRqQQ3q4ZgSg=
@@ -48,6 +50,7 @@ github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
 github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
+github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
@@ -67,8 +70,8 @@ golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
-golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
-golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
+golang.org/x/sync v0.8.0 h1:3NFvSEYkUoMifnESzZl15y791HH1qU2xm6eCJU5ZPXQ=
+golang.org/x/sync v0.8.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
@@ -110,12 +113,13 @@ google.golang.org/protobuf v1.23.1-0.20200526195155-81db48ad09cc/go.mod h1:EGpAD
 google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlbajtzgsN7c=
 google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
 google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
-google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
-google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
+google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg=
+google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=


Test patch:
diff --git a/desc/protoparse/options_test.go b/desc/protoparse/options_test.go
index 78791ea..fa5b84b 100644
--- a/desc/protoparse/options_test.go
+++ b/desc/protoparse/options_test.go
@@ -143,6 +143,24 @@ func TestOptionsInUnlinkedFiles(t *testing.T) {
 	}
 }
 
+func TestInvalidOptionsInUnlinkedFiles(t *testing.T) {
+	// https://github.com/jhump/protoreflect/issues/620
+	// This would previously panic. This also ensures that the reported error
+	// is not due to the inability to resolve "malformed_non_existant".
+	_, err := Parser{
+		Accessor: FileContentsFromMap(map[string]string{
+			"test.proto": `syntax = "proto2";
+				package foo;
+				option malformed_non_existant = true;
+				option features.utf8_validation = NONE;`,
+		}),
+		InterpretOptionsInUnlinkedFiles: true,
+		IncludeSourceCodeInfo:           true,
+	}.ParseFilesButDoNotLink("test.proto")
+	testutil.Nok(t, err)
+	testutil.Eq(t, `test.proto:4:33: field "google.protobuf.FeatureSet.utf8_validation" was not introduced until edition 2023`, err.Error())
+}
+
 func accessorFor(name, contents string) FileAccessor {
 	return func(n string) (io.ReadCloser, error) {
 		if n == name {