proto: make invalid UTF-8 errors non-fatal (#660)

The current logic currently treats RequredNotSetError as a non-fatal
error such that it continues to proceed with its normal execution, but
returns that non-fatal error at the end.

We now make it such that invalid UTF-8 is also a distinguishable error
that is treated as non-fatal by the execution logic. In the rare event
that both an RequiredNotSet error and InvalidUTF8 error is encountered,
the first one encountered is returned.

In the process of making this change, we also fix a number of cases where
RequiredNotSet was treated as fatal, when it should not have been non-fatal
(notably in the logic for extensions, message sets, and maps).

This change deliberately does not provide an API make it easy to distinguish
invalid UTF-8 errors as this is not a normal behavior we want users to
depend on. We can always expose additional API for this in the future.

Users can test for invalid UTF-8 with:
	re, ok := err.(interface{ InvalidUTF8() bool })
	isInvalidUTF8 := ok && re.InvalidUTF8()

Author: dsnet

proto/all_test.go

@@ -2256,26 +2256,32 @@ func TestInvalidUTF8(t *testing.T) {
 		label  string
 		proto2 Message
 		proto3 Message
+		want   []byte
 	}{{
 		label:  "Scalar",
 		proto2: &TestUTF8{Scalar: String(invalidUTF8)},
 		proto3: &pb3.TestUTF8{Scalar: invalidUTF8},
+		want:   []byte{0x0a, 0x07, 0xde, 0xad, 0xbe, 0xef, 0x80, 0x00, 0xff},
 	}, {
 		label:  "Vector",
 		proto2: &TestUTF8{Vector: []string{invalidUTF8}},
 		proto3: &pb3.TestUTF8{Vector: []string{invalidUTF8}},
+		want:   []byte{0x12, 0x07, 0xde, 0xad, 0xbe, 0xef, 0x80, 0x00, 0xff},
 	}, {
 		label:  "Oneof",
 		proto2: &TestUTF8{Oneof: &TestUTF8_Field{invalidUTF8}},
 		proto3: &pb3.TestUTF8{Oneof: &pb3.TestUTF8_Field{invalidUTF8}},
+		want:   []byte{0x1a, 0x07, 0xde, 0xad, 0xbe, 0xef, 0x80, 0x00, 0xff},
 	}, {
 		label:  "MapKey",
 		proto2: &TestUTF8{MapKey: map[string]int64{invalidUTF8: 0}},
 		proto3: &pb3.TestUTF8{MapKey: map[string]int64{invalidUTF8: 0}},
+		want:   []byte{0x22, 0x0b, 0x0a, 0x07, 0xde, 0xad, 0xbe, 0xef, 0x80, 0x00, 0xff, 0x10, 0x00},
 	}, {
 		label:  "MapValue",
 		proto2: &TestUTF8{MapValue: map[int64]string{0: invalidUTF8}},
 		proto3: &pb3.TestUTF8{MapValue: map[int64]string{0: invalidUTF8}},
+		want:   []byte{0x2a, 0x0b, 0x08, 0x00, 0x12, 0x07, 0xde, 0xad, 0xbe, 0xef, 0x80, 0x00, 0xff},
 	}}
 
 	for _, tt := range tests {
@@ -2284,22 +2290,37 @@ func TestInvalidUTF8(t *testing.T) {
 		if err != nil {
 			t.Errorf("Marshal(proto2.%s) = %v, want nil", tt.label, err)
 		}
-		tt.proto2.Reset()
-		err = Unmarshal(b, tt.proto2)
-		if err != nil {
+		if !bytes.Equal(b, tt.want) {
+			t.Errorf("Marshal(proto2.%s) = %x, want %x", tt.label, b, tt.want)
+		}
+
+		m := Clone(tt.proto2)
+		m.Reset()
+		if err = Unmarshal(tt.want, m); err != nil {
 			t.Errorf("Unmarshal(proto2.%s) = %v, want nil", tt.label, err)
 		}
+		if !Equal(m, tt.proto2) {
+			t.Errorf("proto2.%s: output mismatch:\ngot  %v\nwant %v", tt.label, m, tt.proto2)
+		}
 
 		// Proto3 should validate UTF-8.
-		_, err = Marshal(tt.proto3)
+		b, err = Marshal(tt.proto3)
 		if err == nil {
 			t.Errorf("Marshal(proto3.%s) = %v, want non-nil", tt.label, err)
 		}
-		tt.proto3.Reset()
-		err = Unmarshal(b, tt.proto3)
+		if !bytes.Equal(b, tt.want) {
+			t.Errorf("Marshal(proto3.%s) = %x, want %x", tt.label, b, tt.want)
+		}
+
+		m = Clone(tt.proto3)
+		m.Reset()
+		err = Unmarshal(tt.want, m)
 		if err == nil {
 			t.Errorf("Unmarshal(proto3.%s) = %v, want non-nil", tt.label, err)
 		}
+		if !Equal(m, tt.proto3) {
+			t.Errorf("proto3.%s: output mismatch:\ngot  %v\nwant %v", tt.label, m, tt.proto2)
+		}
 	}
 }
 

proto/encode.go

@@ -37,24 +37,9 @@ package proto
 
 import (
 	"errors"
-	"fmt"
 	"reflect"
 )
 
-// RequiredNotSetError is an error type returned by either Marshal or Unmarshal.
-// Marshal reports this when a required field is not initialized.
-// Unmarshal reports this when a required field is missing from the wire data.
-type RequiredNotSetError struct {
-	field string
-}
-
-func (e *RequiredNotSetError) Error() string {
-	if e.field == "" {
-		return fmt.Sprintf("proto: required field not set")
-	}
-	return fmt.Sprintf("proto: required field %q not set", e.field)
-}
-
 var (
 	// errRepeatedHasNil is the error returned if Marshal is called with
 	// a struct with a repeated field containing a nil element.

proto/lib.go

@@ -265,7 +265,6 @@ package proto
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"log"
 	"reflect"
@@ -274,7 +273,66 @@ import (
 	"sync"
 )
 
-var errInvalidUTF8 = errors.New("proto: invalid UTF-8 string")
+// RequiredNotSetError is an error type returned by either Marshal or Unmarshal.
+// Marshal reports this when a required field is not initialized.
+// Unmarshal reports this when a required field is missing from the wire data.
+type RequiredNotSetError struct{ field string }
+
+func (e *RequiredNotSetError) Error() string {
+	if e.field == "" {
+		return fmt.Sprintf("proto: required field not set")
+	}
+	return fmt.Sprintf("proto: required field %q not set", e.field)
+}
+func (e *RequiredNotSetError) RequiredNotSet() bool {
+	return true
+}
+
+type invalidUTF8Error struct{ field string }
+
+func (e *invalidUTF8Error) Error() string {
+	if e.field == "" {
+		return "proto: invalid UTF-8 detected"
+	}
+	return fmt.Sprintf("proto: field %q contains invalid UTF-8", e.field)
+}
+func (e *invalidUTF8Error) InvalidUTF8() bool {
+	return true
+}
+
+// errInvalidUTF8 is a sentinel error to identify fields with invalid UTF-8.
+// This error should not be exposed to the external API as such errors should
+// be recreated with the field information.
+var errInvalidUTF8 = &invalidUTF8Error{}
+
+// isNonFatal reports whether the error is either a RequiredNotSet error
+// or a InvalidUTF8 error.
+func isNonFatal(err error) bool {
+	if re, ok := err.(interface{ RequiredNotSet() bool }); ok && re.RequiredNotSet() {
+		return true
+	}
+	if re, ok := err.(interface{ InvalidUTF8() bool }); ok && re.InvalidUTF8() {
+		return true
+	}
+	return false
+}
+
+type nonFatal struct{ E error }
+
+// Merge merges err into nf and reports whether it was successful.
+// Otherwise it returns false for any fatal non-nil errors.
+func (nf *nonFatal) Merge(err error) (ok bool) {
+	if err == nil {
+		return true // not an error
+	}
+	if !isNonFatal(err) {
+		return false // fatal error
+	}
+	if nf.E == nil {
+		nf.E = err // store first instance of non-fatal error
+	}
+	return true
+}
 
 // Message is implemented by generated protocol buffer messages.
 type Message interface {