all: improve extension validation

Changes made:
* Ensure protoreflect.ExtensionType.IsValidInterface never panics,
especially if given a nil interface value.
* Have protoreflect.ExtensionType.IsValid{Interface,Value} only
perform type-checks. It does not do value checks (i.e., whether the
value itself is valid). Value validity is left to when an actual
protoreflect.Message.Set operation is performed.
* Add special-casing on proto.SetExtension to treat an invalid
message or list as functionally equivalent to Clear. This is to
be more consistent with the legacy SetExtension implementation
which never panicked when given such values.
* Add special-casing on proto.HasExtension to treat a mismatched
extension descriptor as simply not being present in the message.
This is also to be more consistent with the legacy HasExtension
implementation which did the same thing.

Change-Id: Idf0419abf27b9f85d9b92bd2ff8088e25b7990cc
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/229558
Reviewed-by: Damien Neil <dneil@google.com>

internal/impl/convert.go

@@ -162,7 +162,7 @@ func (c *boolConverter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *boolConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *boolConverter) New() pref.Value  { return c.def }
 func (c *boolConverter) Zero() pref.Value { return c.def }
@@ -186,7 +186,7 @@ func (c *int32Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *int32Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *int32Converter) New() pref.Value  { return c.def }
 func (c *int32Converter) Zero() pref.Value { return c.def }
@@ -210,7 +210,7 @@ func (c *int64Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *int64Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *int64Converter) New() pref.Value  { return c.def }
 func (c *int64Converter) Zero() pref.Value { return c.def }
@@ -234,7 +234,7 @@ func (c *uint32Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *uint32Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *uint32Converter) New() pref.Value  { return c.def }
 func (c *uint32Converter) Zero() pref.Value { return c.def }
@@ -258,7 +258,7 @@ func (c *uint64Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *uint64Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *uint64Converter) New() pref.Value  { return c.def }
 func (c *uint64Converter) Zero() pref.Value { return c.def }
@@ -282,7 +282,7 @@ func (c *float32Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *float32Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *float32Converter) New() pref.Value  { return c.def }
 func (c *float32Converter) Zero() pref.Value { return c.def }
@@ -306,7 +306,7 @@ func (c *float64Converter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *float64Converter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *float64Converter) New() pref.Value  { return c.def }
 func (c *float64Converter) Zero() pref.Value { return c.def }
@@ -336,7 +336,7 @@ func (c *stringConverter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *stringConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *stringConverter) New() pref.Value  { return c.def }
 func (c *stringConverter) Zero() pref.Value { return c.def }
@@ -363,7 +363,7 @@ func (c *bytesConverter) IsValidPB(v pref.Value) bool {
 	return ok
 }
 func (c *bytesConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 func (c *bytesConverter) New() pref.Value  { return c.def }
 func (c *bytesConverter) Zero() pref.Value { return c.def }
@@ -400,7 +400,7 @@ func (c *enumConverter) IsValidPB(v pref.Value) bool {
 }
 
 func (c *enumConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 
 func (c *enumConverter) New() pref.Value {
@@ -455,7 +455,7 @@ func (c *messageConverter) IsValidPB(v pref.Value) bool {
 }
 
 func (c *messageConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 
 func (c *messageConverter) New() pref.Value {

internal/impl/convert_list.go

@@ -22,7 +22,7 @@ func newListConverter(t reflect.Type, fd pref.FieldDescriptor) Converter {
 }
 
 type listConverter struct {
-	goType reflect.Type
+	goType reflect.Type // []T
 	c      Converter
 }
 
@@ -48,11 +48,11 @@ func (c *listConverter) IsValidPB(v pref.Value) bool {
 	if !ok {
 		return false
 	}
-	return list.v.Type().Elem() == c.goType && list.IsValid()
+	return list.v.Type().Elem() == c.goType
 }
 
 func (c *listConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 
 func (c *listConverter) New() pref.Value {
@@ -64,7 +64,7 @@ func (c *listConverter) Zero() pref.Value {
 }
 
 type listPtrConverter struct {
-	goType reflect.Type
+	goType reflect.Type // *[]T
 	c      Converter
 }
 
@@ -88,7 +88,7 @@ func (c *listPtrConverter) IsValidPB(v pref.Value) bool {
 }
 
 func (c *listPtrConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 
 func (c *listPtrConverter) New() pref.Value {

internal/impl/convert_map.go

@@ -12,7 +12,7 @@ import (
 )
 
 type mapConverter struct {
-	goType           reflect.Type
+	goType           reflect.Type // map[K]V
 	keyConv, valConv Converter
 }
 
@@ -43,11 +43,11 @@ func (c *mapConverter) IsValidPB(v pref.Value) bool {
 	if !ok {
 		return false
 	}
-	return mapv.v.Type() == c.goType && mapv.IsValid()
+	return mapv.v.Type() == c.goType
 }
 
 func (c *mapConverter) IsValidGo(v reflect.Value) bool {
-	return v.Type() == c.goType
+	return v.IsValid() && v.Type() == c.goType
 }
 
 func (c *mapConverter) New() pref.Value {

internal/impl/message_reflect.go

@@ -170,6 +170,8 @@ func (m *extensionMap) Has(xt pref.ExtensionType) (ok bool) {
 		return x.Value().List().Len() > 0
 	case xd.IsMap():
 		return x.Value().Map().Len() > 0
+	case xd.Message() != nil:
+		return x.Value().Message().IsValid()
 	}
 	return true
 }
@@ -186,15 +188,28 @@ func (m *extensionMap) Get(xt pref.ExtensionType) pref.Value {
 	return xt.Zero()
 }
 func (m *extensionMap) Set(xt pref.ExtensionType, v pref.Value) {
-	if !xt.IsValidValue(v) {
+	xd := xt.TypeDescriptor()
+	isValid := true
+	switch {
+	case !xt.IsValidValue(v):
+		isValid = false
+	case xd.IsList():
+		isValid = v.List().IsValid()
+	case xd.IsMap():
+		isValid = v.Map().IsValid()
+	case xd.Message() != nil:
+		isValid = v.Message().IsValid()
+	}
+	if !isValid {
 		panic(fmt.Sprintf("%v: assigning invalid value", xt.TypeDescriptor().FullName()))
 	}
+
 	if *m == nil {
 		*m = make(map[int32]ExtensionField)
 	}
 	var x ExtensionField
 	x.Set(xt, v)
-	(*m)[int32(xt.TypeDescriptor().Number())] = x
+	(*m)[int32(xd.Number())] = x
 }
 func (m *extensionMap) Mutable(xt pref.ExtensionType) pref.Value {
 	xd := xt.TypeDescriptor()

proto/extension.go

@@ -9,40 +9,65 @@ import (
 )
 
 // HasExtension reports whether an extension field is populated.
-// It panics if ext does not extend m.
-func HasExtension(m Message, ext protoreflect.ExtensionType) bool {
+// It returns false if m is invalid or if xt does not extend m.
+func HasExtension(m Message, xt protoreflect.ExtensionType) bool {
 	// Treat nil message interface as an empty message; no populated fields.
 	if m == nil {
 		return false
 	}
 
-	return m.ProtoReflect().Has(ext.TypeDescriptor())
+	// As a special-case, we reports invalid or mismatching descriptors
+	// as always not being populated (since they aren't).
+	if xt == nil || m.ProtoReflect().Descriptor() != xt.TypeDescriptor().ContainingMessage() {
+		return false
+	}
+
+	return m.ProtoReflect().Has(xt.TypeDescriptor())
 }
 
 // ClearExtension clears an extension field such that subsequent
 // HasExtension calls return false.
-// It panics if ext does not extend m.
-func ClearExtension(m Message, ext protoreflect.ExtensionType) {
-	m.ProtoReflect().Clear(ext.TypeDescriptor())
+// It panics if m is invalid or if xt does not extend m.
+func ClearExtension(m Message, xt protoreflect.ExtensionType) {
+	m.ProtoReflect().Clear(xt.TypeDescriptor())
 }
 
 // GetExtension retrieves the value for an extension field.
 // If the field is unpopulated, it returns the default value for
 // scalars and an immutable, empty value for lists or messages.
-// It panics if ext does not extend m.
-func GetExtension(m Message, ext protoreflect.ExtensionType) interface{} {
+// It panics if xt does not extend m.
+func GetExtension(m Message, xt protoreflect.ExtensionType) interface{} {
 	// Treat nil message interface as an empty message; return the default.
 	if m == nil {
-		return ext.InterfaceOf(ext.Zero())
+		return xt.InterfaceOf(xt.Zero())
 	}
 
-	return ext.InterfaceOf(m.ProtoReflect().Get(ext.TypeDescriptor()))
+	return xt.InterfaceOf(m.ProtoReflect().Get(xt.TypeDescriptor()))
 }
 
 // SetExtension stores the value of an extension field.
-// It panics if ext does not extend m or if value type is invalid for the field.
-func SetExtension(m Message, ext protoreflect.ExtensionType, value interface{}) {
-	m.ProtoReflect().Set(ext.TypeDescriptor(), ext.ValueOf(value))
+// It panics if m is invalid, xt does not extend m, or if type of v
+// is invalid for the specified extension field.
+func SetExtension(m Message, xt protoreflect.ExtensionType, v interface{}) {
+	xd := xt.TypeDescriptor()
+	pv := xt.ValueOf(v)
+
+	// Specially treat an invalid list, map, or message as clear.
+	isValid := true
+	switch {
+	case xd.IsList():
+		isValid = pv.List().IsValid()
+	case xd.IsMap():
+		isValid = pv.Map().IsValid()
+	case xd.Message() != nil:
+		isValid = pv.Message().IsValid()
+	}
+	if !isValid {
+		m.ProtoReflect().Clear(xd)
+		return
+	}
+
+	m.ProtoReflect().Set(xd, pv)
 }
 
 // RangeExtensions iterates over every populated extension field in m in an

proto/extension_test.go

@@ -6,12 +6,14 @@ package proto_test
 
 import (
 	"fmt"
+	"reflect"
 	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoreflect"
 	pref "google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/runtime/protoimpl"
 	"google.golang.org/protobuf/testing/protocmp"
@@ -69,6 +71,139 @@ func TestExtensionFuncs(t *testing.T) {
 	}
 }
 
+func TestIsValid(t *testing.T) {
+	tests := []struct {
+		xt   protoreflect.ExtensionType
+		vi   interface{}
+		want bool
+	}{
+		{testpb.E_OptionalBool, nil, false},
+		{testpb.E_OptionalBool, bool(true), true},
+		{testpb.E_OptionalBool, new(bool), false},
+		{testpb.E_OptionalInt32, nil, false},
+		{testpb.E_OptionalInt32, int32(0), true},
+		{testpb.E_OptionalInt32, new(int32), false},
+		{testpb.E_OptionalInt64, nil, false},
+		{testpb.E_OptionalInt64, int64(0), true},
+		{testpb.E_OptionalInt64, new(int64), false},
+		{testpb.E_OptionalUint32, nil, false},
+		{testpb.E_OptionalUint32, uint32(0), true},
+		{testpb.E_OptionalUint32, new(uint32), false},
+		{testpb.E_OptionalUint64, nil, false},
+		{testpb.E_OptionalUint64, uint64(0), true},
+		{testpb.E_OptionalUint64, new(uint64), false},
+		{testpb.E_OptionalFloat, nil, false},
+		{testpb.E_OptionalFloat, float32(0), true},
+		{testpb.E_OptionalFloat, new(float32), false},
+		{testpb.E_OptionalDouble, nil, false},
+		{testpb.E_OptionalDouble, float64(0), true},
+		{testpb.E_OptionalDouble, new(float32), false},
+		{testpb.E_OptionalString, nil, false},
+		{testpb.E_OptionalString, string(""), true},
+		{testpb.E_OptionalString, new(string), false},
+		{testpb.E_OptionalNestedEnum, nil, false},
+		{testpb.E_OptionalNestedEnum, testpb.TestAllTypes_BAZ, true},
+		{testpb.E_OptionalNestedEnum, testpb.TestAllTypes_BAZ.Enum(), false},
+		{testpb.E_OptionalNestedMessage, nil, false},
+		{testpb.E_OptionalNestedMessage, (*testpb.TestAllExtensions_NestedMessage)(nil), true},
+		{testpb.E_OptionalNestedMessage, new(testpb.TestAllExtensions_NestedMessage), true},
+		{testpb.E_OptionalNestedMessage, new(testpb.TestAllExtensions), false},
+		{testpb.E_RepeatedBool, nil, false},
+		{testpb.E_RepeatedBool, []bool(nil), true},
+		{testpb.E_RepeatedBool, []bool{}, true},
+		{testpb.E_RepeatedBool, []bool{false}, true},
+		{testpb.E_RepeatedBool, []*bool{}, false},
+		{testpb.E_RepeatedInt32, nil, false},
+		{testpb.E_RepeatedInt32, []int32(nil), true},
+		{testpb.E_RepeatedInt32, []int32{}, true},
+		{testpb.E_RepeatedInt32, []int32{0}, true},
+		{testpb.E_RepeatedInt32, []*int32{}, false},
+		{testpb.E_RepeatedInt64, nil, false},
+		{testpb.E_RepeatedInt64, []int64(nil), true},
+		{testpb.E_RepeatedInt64, []int64{}, true},
+		{testpb.E_RepeatedInt64, []int64{0}, true},
+		{testpb.E_RepeatedInt64, []*int64{}, false},
+		{testpb.E_RepeatedUint32, nil, false},
+		{testpb.E_RepeatedUint32, []uint32(nil), true},
+		{testpb.E_RepeatedUint32, []uint32{}, true},
+		{testpb.E_RepeatedUint32, []uint32{0}, true},
+		{testpb.E_RepeatedUint32, []*uint32{}, false},
+		{testpb.E_RepeatedUint64, nil, false},
+		{testpb.E_RepeatedUint64, []uint64(nil), true},
+		{testpb.E_RepeatedUint64, []uint64{}, true},
+		{testpb.E_RepeatedUint64, []uint64{0}, true},
+		{testpb.E_RepeatedUint64, []*uint64{}, false},
+		{testpb.E_RepeatedFloat, nil, false},
+		{testpb.E_RepeatedFloat, []float32(nil), true},
+		{testpb.E_RepeatedFloat, []float32{}, true},
+		{testpb.E_RepeatedFloat, []float32{0}, true},
+		{testpb.E_RepeatedFloat, []*float32{}, false},
+		{testpb.E_RepeatedDouble, nil, false},
+		{testpb.E_RepeatedDouble, []float64(nil), true},
+		{testpb.E_RepeatedDouble, []float64{}, true},
+		{testpb.E_RepeatedDouble, []float64{0}, true},
+		{testpb.E_RepeatedDouble, []*float64{}, false},
+		{testpb.E_RepeatedString, nil, false},
+		{testpb.E_RepeatedString, []string(nil), true},
+		{testpb.E_RepeatedString, []string{}, true},
+		{testpb.E_RepeatedString, []string{""}, true},
+		{testpb.E_RepeatedString, []*string{}, false},
+		{testpb.E_RepeatedNestedEnum, nil, false},
+		{testpb.E_RepeatedNestedEnum, []testpb.TestAllTypes_NestedEnum(nil), true},
+		{testpb.E_RepeatedNestedEnum, []testpb.TestAllTypes_NestedEnum{}, true},
+		{testpb.E_RepeatedNestedEnum, []testpb.TestAllTypes_NestedEnum{0}, true},
+		{testpb.E_RepeatedNestedEnum, []*testpb.TestAllTypes_NestedEnum{}, false},
+		{testpb.E_RepeatedNestedMessage, nil, false},
+		{testpb.E_RepeatedNestedMessage, []*testpb.TestAllExtensions_NestedMessage(nil), true},
+		{testpb.E_RepeatedNestedMessage, []*testpb.TestAllExtensions_NestedMessage{}, true},
+		{testpb.E_RepeatedNestedMessage, []*testpb.TestAllExtensions_NestedMessage{{}}, true},
+		{testpb.E_RepeatedNestedMessage, []*testpb.TestAllExtensions{}, false},
+	}
+
+	for _, tt := range tests {
+		// Check the results of IsValidInterface.
+		got := tt.xt.IsValidInterface(tt.vi)
+		if got != tt.want {
+			t.Errorf("%v.IsValidInterface() = %v, want %v", tt.xt.TypeDescriptor().FullName(), got, tt.want)
+		}
+		if !got {
+			continue
+		}
+
+		// Set the extension value and verify the results of Has.
+		wantHas := true
+		pv := tt.xt.ValueOf(tt.vi)
+		switch v := pv.Interface().(type) {
+		case protoreflect.List:
+			wantHas = v.Len() > 0
+		case protoreflect.Message:
+			wantHas = v.IsValid()
+		}
+		m := &testpb.TestAllExtensions{}
+		proto.SetExtension(m, tt.xt, tt.vi)
+		gotHas := proto.HasExtension(m, tt.xt)
+		if gotHas != wantHas {
+			t.Errorf("HasExtension(%q) = %v, want %v", tt.xt.TypeDescriptor().FullName(), gotHas, wantHas)
+		}
+
+		// Check consistency of IsValidInterface and IsValidValue.
+		got = tt.xt.IsValidValue(pv)
+		if got != tt.want {
+			t.Errorf("%v.IsValidValue() = %v, want %v", tt.xt.TypeDescriptor().FullName(), got, tt.want)
+		}
+		if !got {
+			continue
+		}
+
+		// Use of reflect.DeepEqual is intentional.
+		// We really do want to ensure that the memory layout is identical.
+		vi := tt.xt.InterfaceOf(pv)
+		if !reflect.DeepEqual(vi, tt.vi) {
+			t.Errorf("InterfaceOf(ValueOf(...)) round-trip mismatch: got %v, want %v", vi, tt.vi)
+		}
+	}
+}
+
 func TestExtensionRanger(t *testing.T) {
 	want := map[pref.ExtensionType]interface{}{
 		testpb.E_OptionalInt32:         int32(5),

proto/nil_test.go

@@ -97,7 +97,6 @@ func TestNil(t *testing.T) {
 	}, {
 		label: "HasExtension",
 		test:  func() { proto.HasExtension(nilMsg, nil) },
-		panic: true,
 	}, {
 		label: "HasExtension",
 		test:  func() { proto.HasExtension(nilMsg, extType) },

types/dynamicpb/dynamic.go

@@ -215,7 +215,18 @@ func (m *Message) Set(fd pref.FieldDescriptor, v pref.Value) {
 		panic(errors.New("%v: modification of read-only message", fd.FullName()))
 	}
 	if fd.IsExtension() {
-		if !fd.(pref.ExtensionTypeDescriptor).Type().IsValidValue(v) {
+		isValid := true
+		switch {
+		case !fd.(pref.ExtensionTypeDescriptor).Type().IsValidValue(v):
+			isValid = false
+		case fd.IsList():
+			isValid = v.List().IsValid()
+		case fd.IsMap():
+			isValid = v.Map().IsValid()
+		case fd.Message() != nil:
+			isValid = v.Message().IsValid()
+		}
+		if !isValid {
 			panic(errors.New("%v: assigning invalid type %T", fd.FullName(), v.Interface()))
 		}
 		m.ext[fd.Number()] = fd
@@ -467,6 +478,8 @@ func typecheck(fd pref.FieldDescriptor, v pref.Value) {
 
 func typeIsValid(fd pref.FieldDescriptor, v pref.Value) error {
 	switch {
+	case !v.IsValid():
+		return errors.New("%v: assigning invalid value", fd.FullName())
 	case fd.IsMap():
 		if mapv, ok := v.Interface().(*dynamicMap); !ok || mapv.desc != fd || !mapv.IsValid() {
 			return errors.New("%v: assigning invalid type %T", fd.FullName(), v.Interface())