protocmp: Panic on embed message field comparison

[tamayika]

What version of protobuf and what language are you using?
Version: 1.34.2

What did you do?

Compare embed message field.

// in testing/protocmp/xform_test.go
	t.Run("embed message", func(t *testing.T) {
		type embedMessage struct {
			*testpb.TestAllTypes
		}
		type hasEmbedMessage struct {
			Embed *embedMessage
		}
		if diff := cmp.Diff(&hasEmbedMessage{}, &hasEmbedMessage{}, Transform()); diff != "" {
			t.Errorf("Transform() mismatch (-want +got):\n%v", diff)
		}
	})

What did you expect to see?

No panic.

What did you see instead?

Panic at https://github.com/protocolbuffers/protobuf-go/blob/master/internal/impl/api_export.go#L144 by call method for nil pointer.

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=0x0 pc=0x6fd337]

goroutine 7 [running]:
testing.tRunner.func1.2({0x73fa80, 0xab84b0})
	/go/1.22.0/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
	/go/1.22.0/src/testing/testing.go:1634 +0x377
panic({0x73fa80?, 0xab84b0?})
	/go/1.22.0/src/runtime/panic.go:770 +0x132
google.golang.org/protobuf/testing/protocmp.(*embedMessage·3).ProtoReflect(0x79e0a0?)
	<autogenerated>:1 +0x17
google.golang.org/protobuf/internal/impl.Export.MessageOf({}, {0x79e0a0, 0x0})
	/go/src/github.com/protocolbuffers/protobuf-go/internal/impl/api_export.go:144 +0xa3
google.golang.org/protobuf/testing/protocmp.Transform.func3({0x79e0a0?, 0x0?})
	/go/src/github.com/protocolbuffers/protobuf-go/testing/protocmp/xform.go:238 +0x19e
reflect.Value.call({0x72a220?, 0xc0002564f0?, 0x40?}, {0x7a1409, 0x4}, {0xc0000d9570, 0x1, 0x449d0b?})
	/go/1.22.0/src/reflect/value.go:596 +0xce5
reflect.Value.Call({0x72a220?, 0xc0002564f0?, 0x196?}, {0xc0000d9570?, 0x4120bb?, 0xc0000783b0?})
	/go/1.22.0/src/reflect/value.go:380 +0xb9
github.com/google/go-cmp/cmp.(*state).callTRFunc(0xc000261220, {0x72a220?, 0xc0002564f0?, 0x79e0a0?}, {0x79e0a0?, 0xc0000783b0?, 0xc0000a4650?}, {0x4120bb?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:333 +0x225
github.com/google/go-cmp/cmp.(*transformer).apply(0xc000228c40, 0xc000261220, {0x79e0a0?, 0xc0000783b0?, 0xc0000a46c0?}, {0x79e0a0?, 0xc0000783b8?, 0x87e0e0?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/options.go:320 +0x139
github.com/google/go-cmp/cmp.(*state).tryOptions(0xc000261220, {0x87e0e0, 0x79e0a0}, {0x79e0a0?, 0xc0000783b0?, 0x10?}, {0x79e0a0?, 0xc0000783b8?, 0xacef20?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:303 +0x109
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000261220, {0x877320, 0xc0001fab00})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:258 +0x4c7
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc000261220, {0x87e0e0, 0x748a40}, {0x748a40?, 0xc0000783b0?, 0x4187bf?}, {0x748a40?, 0xc0000783b8?, 0x0?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:427 +0x586
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000261220, {0x877380, 0xc000228cc0})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:286 +0xcce
github.com/google/go-cmp/cmp.(*state).comparePtr(0xc000261220, {0x87e0e0, 0x717600}, {0x717600?, 0xc0000783b0?, 0x743820?}, {0x717600?, 0xc0000783b8?, 0x40?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:579 +0x373
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000261220, {0x8773e0, 0xc000228c80})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:292 +0xb72
github.com/google/go-cmp/cmp.Diff({0x717600, 0xc0000783b0}, {0x717600, 0xc0000783b8}, {0xc000256520?, 0xaa3860?, 0xc000076760?})
	/go/pkg/mod/github.com/google/[email protected]/cmp/compare.go:119 +0x75
google.golang.org/protobuf/testing/protocmp.TestTransform.func6(0xc00026c9c0)
	/go/src/github.com/protocolbuffers/protobuf-go/testing/protocmp/xform_test.go:305 +0xab
testing.tRunner(0xc00026c9c0, 0x7b81b0)
	/go/1.22.0/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 6
	/go/1.22.0/src/testing/testing.go:1742 +0x390

Anything else we should know about your project / environment?

[znkr]

Embedding a proto message like this

type embedMessage struct {
  *testpb.TestAllTypes
}

Generates a lot of methods that forward to the generated testpb.TestAllTypes type. protocmp therefore thinks that this is a proto message (it implements the correct interface). In this case, something equivalent to

func (e *embedMessage) ProtoReflect() protoreflect.ProtoMessage {
  return e.TestAllTypes.ProtoReflect()
}

The problem is that protocmp is passing this type to MessageOf in the first place. The documentation is pretty clear that the type must be a "message type generated by protoc-gen-go and must be a pointer to a named struct type". OTOH, protocmp has no way of knowing that *embedMessage is not a proto type, after all *embedMessage implements the correct interface.

All in all, I don't think this is a problem that can be fixed without severely breaking backwards compatibility. If you must embed a proto, and I don't think embetting is good idea for any type you don't fully control, then you are responsible for making sure that it really behaves like a proto message.

[tamayika]

I think we can implement this feature without losing compatibility.
Please check the PR.
If PR is likely accepted, I will send it to google's gerrit too.

[znkr]

Thanks for the PR.

IIUC, the PR assumes that there are no implementations of proto.Message that rely on embedding another proto.Message. That's unlikely to exist, but there are a lot of proto users out there and therefore even unlikely things like that happen regularly.

I am also not convinced that the additional complexity is worth it: There are number of functions that take a proto.Message, handling this differently in protocmp is bound to cause confusion.

I don't think this solution should be accepted.

[tamayika]

I think protocmp is totally broken for proto.Message embed structs and no one is happy with this behavior at the point of comparing of go-cmp's one.
I keep this issue open for other users facing the same issue.

[znkr]

Keep in mind that your type is implementing proto.Message if you embed a proto message. If you do that and pass it to a function that accepts a proto.Message it's your responsibility to implement it correctly. Granted, it would be nice if that happened automatically, but it's not. I think a fix should be on that level. If that's not possible, so be it.

Either way, as I said before, embedding proto messages is not a good idea. I would be very surprised if protocmp is the only sharp corner you'll run into with that approach.

[neild]

A protobuf message is any type that implements "google.golang.org/protobuf/proto".Message.

The *embedMessage type implements proto.Message, therefore it's a message. However, a nil *embedMessage panics when its ProtoReflect method is called, therefore the panic as observed here. You either need to take care not to pass a nil *embedMessage to something that expects a message, change it to not panic in this case, or change it to not implement proto.Message.

For example, you could avoid panicing:

type embedMessage struct {
  *testpb.TestAllTypes
}

func (m *embedMessage) ProtoReflect() protoreflect.Message {
  var p *testpb.TestAllTypes
  if m != nil {
    p = m.TestAllTypes
  }
  return p.ProtoReflect()
}

Or you could avoid implementing proto.Message:

type doNotImplementMessage struct{}
func (doNotImplementMessage) ProtoReflect() protoreflect.Message

type embedMessage struct {
  *testpb.TestAllTypes
  doNotImplementMessage
}

Or, as @znkr suggests, you could avoid embedding altogether, which is probably the simplest solution.

[tamayika]

Yes, I know I can avoid panic by defining ProtoReflect() and works well if and only if there is no other fields.
Embeding proto.Message is useful to extend Message with fields which is not defined in Message but it behaves the same for the fields defined in Message.
Tests in the PR show the problem in current implementatioin.