nebius/SWE-rebench-V2-PRs huggingface dataset:
Repository: jhump/protoreflect
Pull request #260 (2019-08-02 17:00:21)

Add GetUnderlying function to ErrorWithPos
I need access to the underlying error without any source information, this exposes it. Everything works if you just add the function, I added specific tests cases as well though.

This effectively makes ErrorWithPos equal to the old ErrorWithSourcePos in terms of what it exposes, but as an interface.

tie: Hi! That’s a nice feature, but you should take a look at the current developments in Go. The next version will have [`errors.Unwrap`](https://tip.golang.org/pkg/errors/#Unwrap) function which unwraps the underlying error if it the wrapper implements `interface { Unwrap() error }`.

Patch:
diff --git a/desc/protoparse/errors.go b/desc/protoparse/errors.go
index 9f3db90..a4063a8 100644
--- a/desc/protoparse/errors.go
+++ b/desc/protoparse/errors.go
@@ -58,9 +58,12 @@ func (h *errorHandler) getError() error {
 
 // ErrorWithPos is an error about a proto source file that includes information
 // about the location in the file that caused the error.
+//
+// The value of Error() will contain both the SourcePos and Underlying error.
 type ErrorWithPos interface {
 	error
 	GetPosition() SourcePos
+	GetUnderlying() error
 }
 
 // ErrorWithSourcePos is an error about a proto source file that includes
@@ -90,4 +93,10 @@ func (e ErrorWithSourcePos) GetPosition() SourcePos {
 	return *e.Pos
 }
 
+// GetUnderlying implements the ErrorWithPos interface, supplying the
+// underlying error. This error will not include location information.
+func (e ErrorWithSourcePos) GetUnderlying() error {
+	return e.Underlying
+}
+
 var _ ErrorWithPos = ErrorWithSourcePos{}


Test patch:
diff --git a/desc/protoparse/error_reporting_test.go b/desc/protoparse/error_reporting_test.go
index faa446e..1f298a5 100644
--- a/desc/protoparse/error_reporting_test.go
+++ b/desc/protoparse/error_reporting_test.go
@@ -190,7 +190,10 @@ func TestErrorReporting(t *testing.T) {
 		testutil.Eq(t, len(tc.expectedErrs), count, "case #%d: parse should have called reporter %d times", i+1, len(tc.expectedErrs))
 		testutil.Eq(t, len(tc.expectedErrs), len(reported), "case #%d: wrong number of errors reported", i+1)
 		for j := range tc.expectedErrs {
-			testutil.Require(t, strings.Contains(reported[j].Error(), tc.expectedErrs[j]), "case #%d: parse error[%d] have %q; instead got %q", i+1, j, tc.expectedErrs[j], reported[j].Error())
+			testutil.Eq(t, tc.expectedErrs[j], reported[j].Error(), "case #%d: parse error[%d] have %q; instead got %q", i+1, j, tc.expectedErrs[j], reported[j].Error())
+			split := strings.SplitN(tc.expectedErrs[j], ":", 4)
+			testutil.Eq(t, 4, len(split), "case #%d: expected %q [%d] to contain at least 4 elements split by :", i+1, tc.expectedErrs[j], j)
+			testutil.Eq(t, split[3], " "+reported[j].GetUnderlying().Error(), "case #%d: parse error underlying[%d] have %q; instead got %q", i+1, j, split[3], reported[j].GetUnderlying().Error())
 		}
 
 		count = 0
Repository: jhump/protoreflect
Pull request #261 (2019-08-02 18:03:25)

Change ErrorWithPos.GetUnderlying to ErrorWithPos.Unwrap
Re: https://github.com/jhump/protoreflect/pull/260


Patch:
diff --git a/desc/protoparse/errors.go b/desc/protoparse/errors.go
index a4063a8..07a6214 100644
--- a/desc/protoparse/errors.go
+++ b/desc/protoparse/errors.go
@@ -60,10 +60,11 @@ func (h *errorHandler) getError() error {
 // about the location in the file that caused the error.
 //
 // The value of Error() will contain both the SourcePos and Underlying error.
+// The value of Unwrap() will only be the Underlying error.
 type ErrorWithPos interface {
 	error
 	GetPosition() SourcePos
-	GetUnderlying() error
+	Unwrap() error
 }
 
 // ErrorWithSourcePos is an error about a proto source file that includes
@@ -93,9 +94,9 @@ func (e ErrorWithSourcePos) GetPosition() SourcePos {
 	return *e.Pos
 }
 
-// GetUnderlying implements the ErrorWithPos interface, supplying the
-// underlying error. This error will not include location information.
-func (e ErrorWithSourcePos) GetUnderlying() error {
+// Unwrap implements the ErrorWithPos interface, supplying the underlying
+// error. This error will not include location information.
+func (e ErrorWithSourcePos) Unwrap() error {
 	return e.Underlying
 }
 


Test patch:
diff --git a/desc/protoparse/error_reporting_test.go b/desc/protoparse/error_reporting_test.go
index 1f298a5..535f57e 100644
--- a/desc/protoparse/error_reporting_test.go
+++ b/desc/protoparse/error_reporting_test.go
@@ -193,7 +193,7 @@ func TestErrorReporting(t *testing.T) {
 			testutil.Eq(t, tc.expectedErrs[j], reported[j].Error(), "case #%d: parse error[%d] have %q; instead got %q", i+1, j, tc.expectedErrs[j], reported[j].Error())
 			split := strings.SplitN(tc.expectedErrs[j], ":", 4)
 			testutil.Eq(t, 4, len(split), "case #%d: expected %q [%d] to contain at least 4 elements split by :", i+1, tc.expectedErrs[j], j)
-			testutil.Eq(t, split[3], " "+reported[j].GetUnderlying().Error(), "case #%d: parse error underlying[%d] have %q; instead got %q", i+1, j, split[3], reported[j].GetUnderlying().Error())
+			testutil.Eq(t, split[3], " "+reported[j].Unwrap().Error(), "case #%d: parse error underlying[%d] have %q; instead got %q", i+1, j, split[3], reported[j].Unwrap().Error())
 		}
 
 		count = 0
Repository: jhump/protoreflect
Pull request #412 (2021-08-05 15:45:14)

fix panics in protoparse identified by oss-fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34232
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34238

Both bugs had the same root cause: unicode chars with very high code points that collided with the various constants for known token types.

So now the lexer is small enough to limit the symbol type value for such runes to less than 256. The smallest constant is 57346, so _plenty_ of headroom in between to avoid collisions. If it sees a symbol with a greater code point, it will now report an error "invalid character".

This should be fine since this handling is only for stand-alone runes/punctuation. Protobuf does not allow such symbols as part of identifiers nor as any allowed punctuation or operator. Unicode points like this are only valid in a proto source file inside of a string literal. So if we come across a lone rune with such a value (not in a string literal), then it is definitely a syntax error.


Patch:
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index e8f2cd0..faac16e 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -61,11 +61,6 @@ func (rr *runeReader) endMark() string {
 	return m
 }
 
-func lexError(l protoLexer, pos *SourcePos, err string) {
-	pl := l.(*protoLex)
-	_ = pl.errs.handleErrorWithPos(pos, err)
-}
-
 type protoLex struct {
 	filename string
 	input    *runeReader
@@ -382,6 +377,10 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 			l.input.unreadRune(cn)
 		}
 
+		if c > 255 {
+			l.setError(lval, errors.New("invalid character"))
+			return _ERROR
+		}
 		l.setRune(lval, c)
 		return int(c)
 	}


Test patch:
diff --git a/desc/protoparse/parser_test.go b/desc/protoparse/parser_test.go
index 1d2a555..84252b0 100644
--- a/desc/protoparse/parser_test.go
+++ b/desc/protoparse/parser_test.go
@@ -3,6 +3,7 @@ package protoparse
 import (
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
@@ -35,6 +36,24 @@ func TestEmptyParse(t *testing.T) {
 	testutil.Eq(t, 0, len(fd[0].GetServices()))
 }
 
+func TestJunkParse(t *testing.T) {
+	// inputs that have been found in the past to cause panics by oss-fuzz
+	inputs := map[string]string{
+		"case-34232": `'';`,
+		"case-34238": `.`,
+	}
+	for name, input := range inputs {
+		protoName := fmt.Sprintf("%s.proto", name)
+		p := Parser{
+			Accessor: FileContentsFromMap(map[string]string{protoName: input}),
+		}
+		_, err := p.ParseFiles(protoName)
+		// we expect this to error... but we don't want it to panic
+		testutil.Nok(t, err, "junk input should have returned error")
+		t.Logf("error from parse: %v", err)
+	}
+}
+
 func TestSimpleParse(t *testing.T) {
 	protos := map[string]*parseResult{}
Repository: jhump/protoreflect
Pull request #437 (2021-10-31 15:34:01)

another fix to parsing numeric literals
In the same vein as #436, this prevents `protoparse` from accepting some quirks that the Go language supports for float literals that protobuf does not.

The `strconv.ParseFloat` function is used to help with parsing numeric literals. However, as of Go 1.13, some features were added to the language and also added to the `strconv` functions. For floats, this included support for hexadecimal float literals and for `_` to be used as a digit separator (for improved readability).

The hex floats were not inadvertently supported in `protoparse` because `ParseUint` is used when a numeric literal starts with `0x`. _But_ the digit separators were inadvertently supported by `protoparse`.

So this change fixes that and also adds test cases to make sure that float literals supported by Go but not by protobuf are not incorrectly also supported by `protoparse`.


Patch:
diff --git a/desc/protoparse/lexer.go b/desc/protoparse/lexer.go
index 87bc803..b27f105 100644
--- a/desc/protoparse/lexer.go
+++ b/desc/protoparse/lexer.go
@@ -370,6 +370,14 @@ func (l *protoLex) Lex(lval *protoSymType) int {
 }
 
 func parseFloat(token string) (float64, error) {
+	// strconv.ParseFloat allows _ to separate digits, but protobuf does not
+	if strings.ContainsRune(token, '_') {
+		return 0, &strconv.NumError{
+			Func: "parseFloat",
+			Num:  token,
+			Err:  strconv.ErrSyntax,
+		}
+	}
 	f, err := strconv.ParseFloat(token, 64)
 	if err == nil {
 		return f, nil


Test patch:
diff --git a/desc/protoparse/lexer_test.go b/desc/protoparse/lexer_test.go
index ca39a31..b454140 100644
--- a/desc/protoparse/lexer_test.go
+++ b/desc/protoparse/lexer_test.go
@@ -247,6 +247,8 @@ func TestLexerErrors(t *testing.T) {
 		{str: `0b0111`, errMsg: "invalid syntax"},
 		{str: `0o765432`, errMsg: "invalid syntax"},
 		{str: `1_000_000`, errMsg: "invalid syntax"},
+		{str: `1_000.000_001e6`, errMsg: "invalid syntax"},
+		{str: `0X1F_FFP-16`, errMsg: "invalid syntax"},
 		{str: `/* foobar`, errMsg: "unexpected EOF"},
 	}
 	for i, tc := range testCases {
Repository: jhump/protoreflect
Pull request #477 (2022-01-06 15:53:41)

protoparse: fix panic in linker
This can occur if there is a symbol collision between a file that has an AST (e.g. one that was parsed from source) and one that does not (e.g. a "well-known" import that was supplied as a descriptor).

The added test case reproduces the panic described in #476. And the small patch (to use `getNode` function, which handles the case when file has no AST nodes) resolves it.


Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 4a1a4a5..288b79e 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -145,7 +145,7 @@ func (l *linker) createDescriptorPool() error {
 					file1, file2 = file2, file1
 					desc1, desc2 = desc2, desc1
 				}
-				node := l.files[file2].nodes[desc2]
+				node := l.files[file2].getNode(desc2)
 				if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorType(desc1), file1); err != nil {
 					return err
 				}
@@ -930,7 +930,7 @@ func (l *linker) checkForUnusedImports(filename string) {
 			if pos == nil {
 				pos = ast.UnknownPos(r.fd.GetName())
 			}
-			r.errs.warn(pos, errUnusedImport(dep))
+			l.errs.warn(pos, errUnusedImport(dep))
 		}
 	}
 }
diff --git a/desc/protoparse/parser.go b/desc/protoparse/parser.go
index bdc000c..338a0a7 100644
--- a/desc/protoparse/parser.go
+++ b/desc/protoparse/parser.go
@@ -706,6 +706,13 @@ func (r *parseResult) getMethodNode(m *dpb.MethodDescriptorProto) ast.RPCDeclNod
 	return r.nodes[m].(ast.RPCDeclNode)
 }
 
+func (r *parseResult) getNode(m proto.Message) ast.Node {
+	if r.nodes == nil {
+		return ast.NewNoSourceNode(r.fd.GetName())
+	}
+	return r.nodes[m]
+}
+
 func (r *parseResult) putFileNode(f *dpb.FileDescriptorProto, n *ast.FileNode) {
 	r.nodes[f] = n
 }


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 3324acd..63cb951 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -154,6 +154,17 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"foo.proto:1:106: field b: duplicate extension: a and b are both using tag 1",
 		},
+		{
+			map[string]string{
+				"foo.proto": `
+					syntax = "proto3";
+					import "google/protobuf/descriptor.proto";
+					package google.protobuf;
+					message DescriptorProto { }
+				`,
+			},
+			`google/protobuf/descriptor.proto: duplicate symbol google.protobuf.DescriptorProto: already defined as message in "foo.proto"`,
+		},
 		{
 			map[string]string{
 				"foo.proto": "package fu.baz; extend foobar { optional string a = 1; }",
Repository: jhump/protoreflect
Pull request #522 (2022-08-15 19:20:11)

protoparse: it is not allowed to reference a synthetic map entry
Though `protoc`'s error messages can be quite confusing/misleading, the intent is clear that use of a synthetic map entry message should only be from the map field for which the message was synthesized.

The test includes two cases that fail with `protoc` but previously were accepted by protoparse.

jhump: @amckinney, mind taking a look? I will also file a ticket to get this fix ported over to protocompile.


Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 1fea1c9..d80ebde 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -3,6 +3,7 @@ package protoparse
 import (
 	"bytes"
 	"fmt"
+	"google.golang.org/protobuf/types/descriptorpb"
 	"sort"
 	"strings"
 
@@ -518,6 +519,24 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
 	}
 	switch dsc := dsc.(type) {
 	case *dpb.DescriptorProto:
+		if dsc.GetOptions().GetMapEntry() {
+			isValid := false
+			switch node.(type) {
+			case *ast.MapFieldNode:
+				// We have an AST for this file and can see this field is from a map declaration
+				isValid = true
+			case ast.NoSourceNode:
+				// We don't have an AST for the file (it came from a provided descriptor). So we
+				// need to validate that it's not an illegal reference. To be valid, the field
+				// must be repeated and the entry type must be nested in the same enclosing
+				// message as the field.
+				expectFqn := prefix + dsc.GetName()
+				isValid = expectFqn == fqn && fld.GetLabel() == descriptorpb.FieldDescriptorProto_LABEL_REPEATED
+			}
+			if !isValid {
+				return l.errs.handleErrorWithPos(node.FieldType().Start(), "%s: %s is a synthetic map entry and may not be referenced explicitly", scope, fqn)
+			}
+		}
 		fld.TypeName = proto.String("." + fqn)
 		// if type was tentatively unset, we now know it's actually a message
 		if fld.Type == nil {


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 9b3935b..0024119 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -984,6 +984,28 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"", // should succeed
 		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto3\";\n" +
+					"message Foo {\n" +
+					"  map<string,string> bar = 1;\n" +
+					"}\n" +
+					"message Baz {\n" +
+					"  Foo.BarEntry e = 1;\n" +
+					"}\n",
+			},
+			"foo.proto:6:3: field Baz.e: Foo.BarEntry is a synthetic map entry and may not be referenced explicitly",
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto3\";\n" +
+					"import \"google/protobuf/struct.proto\";\n" +
+					"message Foo {\n" +
+					"  google.protobuf.Struct.FieldsEntry e = 1;\n" +
+					"}\n",
+			},
+			"foo.proto:4:3: field Foo.e: google.protobuf.Struct.FieldsEntry is a synthetic map entry and may not be referenced explicitly",
+		},
 	}
 	for i, tc := range testCases {
 		acc := func(filename string) (io.ReadCloser, error) {