sql: calling String() on some ColumnDescriptors (and possibly other types) can panic

[stevendanna]

Describe the problem

(This might be a dupe of #63299)

If we run \set auto_trace=on,kv and then ALTER TABLE foo SET LOCALITY REGIONAL BY ROW, you may receive trace data that shows evidence of a panic:

 2021-04-08 18:41:50.999899+00:00:00 | 00:00:00.007879 | Put /Table/3/1/55/2/1 -> %!s(PANIC=String method: reflect.Value.Bytes of non-byte slice)                       | [n1,client=[::1]:53927,hostnossl,user=root]                                     | ©g356o:356  | flow             |   36

The following test is capable of reproducing this particular panic:

import (
	"testing"

	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
	"github.com/cockroachdb/cockroach/pkg/sql/types"
	"github.com/stretchr/testify/require"
)

func TestDescriptorProtoString(t *testing.T) {
	enumMembers := []string{"hi", "hello"}
	enumType := types.MakeEnum(typedesc.TypeIDToOID(500), typedesc.TypeIDToOID(100500))
	enumType.TypeMeta = types.UserDefinedTypeMetadata{
		Name: &types.UserDefinedTypeName{
			Schema: "test",
			Name:   "greeting",
		},
		EnumData: &types.EnumMetadata{
			LogicalRepresentations: enumMembers,
			PhysicalRepresentations: [][]byte{
				{0x42, 0x1},
				{0x42},
			},
			IsMemberReadOnly: make([]bool, len(enumMembers)),
		},
	}
	desc := &descpb.ColumnDescriptor{
		Name: "c",
		ID:   1,
		Type: enumType,
	}
	require.NotPanics(t, func() { _ = desc.String() })
}

which will fail with:

=== RUN   TestDescriptorProtoString
    descriptor_test.go:50: 
                Error Trace:    descriptor_test.go:50
                Error:          func (assert.PanicTestFunc)(0x533dce0) should not panic
                                        Panic value:    reflect.Value.Bytes of non-byte slice
                                        Panic stack:    goroutine 40 [running]:
                                runtime/debug.Stack(0xc000af62c0, 0x5500ba0, 0x5eaab00)
                                        /usr/local/Cellar/go/1.16.2/libexec/src/runtime/debug/stack.go:24 +0x9f
                                github.com/stretchr/testify/assert.didPanic.func1.1(0xc000af7e20, 0xc000af7e0f, 0xc000af7e10)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/stretchr/testify/assert/assertions.go:1013 +0x6b
                                panic(0x5500ba0, 0x5eaab00)
                                        /usr/local/Cellar/go/1.16.2/libexec/src/runtime/panic.go:965 +0x1b9
                                reflect.Value.Bytes(0x54daec0, 0xc000a95360, 0x197, 0x6070a0e49ba95b2b, 0xc00034ab60, 0xc000af6750)
                                        /usr/local/Cellar/go/1.16.2/libexec/src/reflect/value.go:291 +0x118
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeAny(0x697497a, 0xc000ad2320, 0x54daec0, 0xc000a95360, 0x197, 0xc000aef200, 0x5fbe050, 0x54daec0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:557 +0x62e
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeStruct(0x697497a, 0xc000ad2320, 0x561d4e0, 0xc000a95360, 0x199, 0x199, 0x0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:453 +0x8e7
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeAny(0x697497a, 0xc000ad2320, 0x554bfc0, 0xc000a92470, 0x196, 0xc000aeed00, 0x5fbe050, 0x554bfc0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:604 +0x287
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeStruct(0x697497a, 0xc000ad2320, 0x561d660, 0xc000a92460, 0x199, 0x199, 0x0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:453 +0x8e7
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeAny(0x697497a, 0xc000ad2320, 0x561d660, 0xc000a92460, 0x199, 0xc000a88c00, 0x5fbe050, 0x561d660)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:604 +0x287
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeStruct(0x697497a, 0xc000ad2320, 0x55e4980, 0xc000a923c0, 0x199, 0x199, 0x0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:453 +0x8e7
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeAny(0x697497a, 0xc000ad2320, 0x573c9c0, 0xc000a6bf18, 0x196, 0xc000a88a00, 0x5fbe050, 0x573c9c0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:604 +0x287
                                github.com/gogo/protobuf/proto.(*TextMarshaler).writeStruct(0x697497a, 0xc000ad2320, 0x57013e0, 0xc000a6bf00, 0x199, 0x199, 0x7700a68)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:453 +0x8e7
                                github.com/gogo/protobuf/proto.(*TextMarshaler).Marshal(0x697497a, 0x5ec4720, 0xc000ad5a10, 0x5ee7ab0, 0xc000a6bf00, 0x9, 0x22)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:894 +0x22e
                                github.com/gogo/protobuf/proto.(*TextMarshaler).Text(0x697497a, 0x5ee7ab0, 0xc000a6bf00, 0x40654db, 0xc000411590)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:906 +0x6e
                                github.com/gogo/protobuf/proto.CompactTextString(...)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/text.go:928
                                github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.(*ColumnDescriptor).String(...)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb/structured.pb.go:877
                                github.com/cockroachdb/cockroach/pkg/sql/catalog_test.TestDescriptorProtoString.func1()
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descriptor_test.go:50 +0x45
                                github.com/stretchr/testify/assert.didPanic.func1(0xc000411620, 0xc00041160f, 0xc000411610, 0xc000ade540)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/stretchr/testify/assert/assertions.go:1018 +0x6c
                                github.com/stretchr/testify/assert.didPanic(0xc000ade540, 0x2f6d7a38, 0xc0003b3c80, 0x2f6d7a58, 0xc0003b3c80, 0xc0003b3c01)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/stretchr/testify/assert/assertions.go:1020 +0x5f
                                github.com/stretchr/testify/assert.NotPanics(0x2f6d7a38, 0xc0003b3c80, 0xc000ade540, 0x0, 0x0, 0x0, 0xc000ade540)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/stretchr/testify/assert/assertions.go:1091 +0x77
                                github.com/stretchr/testify/require.NotPanics(0x5ede130, 0xc0003b3c80, 0xc000ade540, 0x0, 0x0, 0x0)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/vendor/github.com/stretchr/testify/require/require.go:1236 +0xbe
                                github.com/cockroachdb/cockroach/pkg/sql/catalog_test.TestDescriptorProtoString(0xc0003b3c80)
                                        /Users/ssd/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descriptor_test.go:50 +0x330
                                testing.tRunner(0xc0003b3c80, 0x5c87610)
                                        /usr/local/Cellar/go/1.16.2/libexec/src/testing/testing.go:1194 +0xef
                                created by testing.(*T).Run
                                        /usr/local/Cellar/go/1.16.2/libexec/src/testing/testing.go:1239 +0x2b3
                Test:           TestDescriptorProtoString

[chengxiong-ruan]

This issue is very unfortunate because gogo proto is not able to convert non-protobuf data type with its TextMarshaler which doesn't call the String() method even the data type is a Stringer.

Why gogo proto need to convert a non-protobuf type to string? you may ask. The culprit is types.T. We defined a T message in sql/types/types.proto which is supposed to be used to generate the T struct in sql/types/types.pb.go. However, looks like we need to add some meta data when users define their own enum type. Here the types.EnumMetadata struct kicks in to play the role, but it has fields with types not supported by protobuf ([][]byte, []string, []bool). So we manually defined another T struct in the same package in sql/types/types.go to override the protobuf one, but still keep the protobuf one for type discovering purpose in proto files. (I was confused a bit, but I assume gogo proto is smart enough to skip a type if it's already defined in the same package). So the ColumnDescriptor here has a type field of T type, even it says I want the T type from proto in the definition, it actually use the manually defined one, and it has the metadata struct which can't be converted to string by gogo proto.

So not just ColumnDescriptor, for any protobuf struct directly or indirectly depends on types.EnumMetadata, it would panic :( It's probably not realistic to ask gogo proto to fix it since it's still looking for active maintainer, not to mention it's us broke the protocol to make things work at cockroach.

But after walk through the text marshaling code of gogo, I find it does support repeated type but you need to tell it that your type is a repeated one through tags. Go structs generated by gogo has those tags if you look at any *.pb.go file. So a easy hack to fix this is to add tags to the fields of EnumMetadata like this:

// EnumMetadata is metadata about an ENUM needed for evaluation.
type EnumMetadata struct {
	PhysicalRepresentations [][]byte `protobuf:"group,1,rep"`
	LogicalRepresentations []string `protobuf:"group,2,rep"`
	IsMemberReadOnly []bool `protobuf:"group,3,rep"`
}

These tags does not do anything except for telling gogo "hey, these. are repeated types". But it's so weird that we put these tags here even it's not a protobuf struct. So far I'm not sure how these tags would affect json pb unmarshaling. Anyone familiar with that? otherwise I would need to look into the code more to figure out or trust our test coverage lol...

[ajwerner]

What if we implement the encoding.TextMarshaler interface on EnumMetadata? I think that MarshalText method would get invoked here https://github.com/gogo/protobuf/blob/226206f39bd7276e88ec684ea0028c18ec2c91ae/proto/text.go#L881

[chengxiong-ruan]

What if we implement the encoding.TextMarshaler interface on EnumMetadata? I think that MarshalText method would get invoked here https://github.com/gogo/protobuf/blob/226206f39bd7276e88ec684ea0028c18ec2c91ae/proto/text.go#L881

That won't work because this is only check at the top level of the struct, it doesn't check for child field types..... that's very unfortunate as well.

[chengxiong-ruan]

oh @ajwerner you might be right, it's also checked here : https://github.com/gogo/protobuf/blob/226206f39bd7276e88ec684ea0028c18ec2c91ae/proto/text.go#L594