Memory corruption/panic on Clone/Merge of nonnullable repeated field

[euroelessar]

Apparently reflection-based logic for merging protobuf messages misbehaves if it sees repeated non-nullable message field.
We've noticed it due to proto.Clone(message) returning shallow copy of the message, however while trying to get smaller example similar use case panics, which potentially points to the same cause.

Example:

message A {
  B B = 1 [(gogoproto.nullable) = false];
  repeated C c = 2; [(gogoproto.nullable) = false];
}

message B {
  int64 c = 1;
}

message C {
  int64 d = 1;
}

Test:

func TestClone2(t *testing.T) {
        f1 := &A{C: []C{{D: 1}, {D: 2}}}
        f2 := proto.Clone(f1).(*A)
        if !reflect.DeepEqual(f1.C, f2.C) {
                t.Fatalf("got %v want %v", f2, f1)
        }
        if fmt.Sprintf("%p", f1.C) == fmt.Sprintf("%p", f2.C) {
                t.Fatalf("slice is not deep copied")
        }
}

Panic:

--- FAIL: TestClone2 (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=0x2 pc=0x105a312]

goroutine 18 [running]:
testing.tRunner.func1(0xc0000d8f00)
	/usr/local/Cellar/go/1.12.4/libexec/src/testing/testing.go:830 +0x392
panic(0x11fc3c0, 0x13f1500)
	/usr/local/Cellar/go/1.12.4/libexec/src/runtime/panic.go:522 +0x1b5
github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc000086240, 0xc0000921a8, 0xc000092148)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/table_merge.go:158 +0x2b8
github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc0000921a8, 0xc000092148)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3e
github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc000086180, 0xc000092180, 0xc000092120)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/table_merge.go:139 +0x478
github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x13fb100, 0x1278820, 0xc000092180, 0x1278820, 0xc000092120)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/table_merge.go:50 +0x4f
github.com/gogo/protobuf/test/merge.(*A).XXX_Merge(0xc000092180, 0x1278820, 0xc000092120)
	/Users/elessar/go/src/github.com/gogo/protobuf/test/merge/merge.pb.go:45 +0x57
github.com/gogo/protobuf/proto.Merge(0x1278820, 0xc000092180, 0x1278820, 0xc000092120)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/clone.go:95 +0x32d
github.com/gogo/protobuf/proto.Clone(0x1278820, 0xc000092120, 0x13fbc40, 0xc000040740)
	/Users/elessar/go/src/github.com/gogo/protobuf/proto/clone.go:52 +0x193
github.com/gogo/protobuf/test/merge.TestClone2(0xc0000d8f00)
	/Users/elessar/go/src/github.com/gogo/protobuf/test/merge/merge_test.go:18 +0xbd
testing.tRunner(0xc0000d8f00, 0x1241b38)
	/usr/local/Cellar/go/1.12.4/libexec/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
	/usr/local/Cellar/go/1.12.4/libexec/src/testing/testing.go:916 +0x35a
FAIL	github.com/gogo/protobuf/test/merge	0.042s

[jmarais]

Hi. Thanks. I did take a look and your example/test looks good. I did noticed an older case related to proto.Clone.

#14

I think the (gogoproto.nullable) = false is currently case is currently not handled for proto.Clone

We can recommend using this library in the meantime:
https://github.com/awalterschulze/goderive
It provides a deriveClone function which generates a deep copy. Its not a fix for this issue, but it might be useful until then.

[euroelessar]

Based on #14 it looks like the state is "help wanted to add more support" and help is provided by #569.
It fixes the behavior for repeated (gogoproto.nullable) = false fields & adds tests for it to ensure gogoproto does not regress in future releases.