proto: mismatched field tags in messages doesn't an error in unrelated message and proto.Unmarshal returns a partially filled message

[odeke-em]

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

I expected that passing in marshaled proto of one type of message to another with different fields would fail
as protobuf seems to promise. I am writing a package to ensure safety for a system that's trying to carefully
switch to entirely using protobufs, and requires flagging when the wrong messages come down the wrong service.

TL;DR:

• Only github.com/gogo/protobuf/proto succeeds
  while the rest of the *golang/protobuf/proto packages fail:
• google.golang.org/protobuf/proto
• github.com/golang/protobuf/proto

Investigation

Given this proto below

syntax = "proto3";

package service;

option go_package = ".;gen";

message Customer1 {
    int32 id = 1;
    string name = 2;
    float subscription_fee = 3;
}

message Customer2 {
    int32 id = 1;
    int32 industry = 2;
    string name = 3;
}

In code if I accidentally unmarshal to a different struct, there isn't an error returned

package main

import (
	"fmt"

	// "google.golang.org/protobuf/proto"
	"github.com/golang/protobuf/proto"
	// "github.com/gogo/protobuf/proto"

	"github.com/orijtech/experiments/gen"
)

func main() {
	c2 := &gen.Customer2{
		Id:       289,
		Name:     "Customer1",
		Industry: 5299,
	}
	blob, err := proto.Marshal(c2)
	if err != nil {
		panic(err)
	}

	c1 := new(gen.Customer1)
	if err := proto.Unmarshal(blob, c1); err != nil {
		panic(err)
	}

	fmt.Printf("C1: %#v\n", c1)
	pm := c1.ProtoReflect()
	fields := pm.Descriptor().Fields()
	for i, n := 0, fields.Len(); i < n; i++ {
		fdi := fields.Get(i)
		fmt.Printf("Index: %d\nName:  %q\nFullName: %q\n", fdi.Index(), fdi.Name(), fdi.FullName())
	}
}

When run with

google.golang.org/protobuf/proto

Unexpectedly succeeds

$ go run main.go 
C1: &gen.Customer1{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(0xc000116500)}, sizeCache:0, unknownFields:[]uint8{0x10, 0xb3, 0x29, 0x1a, 0x9, 0x43, 0x75, 0x73, 0x74, 0x6f, 0x6d, 0x65, 0x72, 0x31}, Id:289, Name:"", SubscriptionFee:0}
Index: 0
Name:  "id"
FullName: "service.Customer1.id"
Index: 1
Name:  "name"
FullName: "service.Customer1.name"
Index: 2
Name:  "subscription_fee"
FullName: "service.Customer1.subscription_fee"

github.com/golang/protobuf/proto

Unexpectedly succeeds

$ go run main.go 
C1: &gen.Customer1{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(0xc0000d4500)}, sizeCache:0, unknownFields:[]uint8{0x10, 0xb3, 0x29, 0x1a, 0x9, 0x43, 0x75, 0x73, 0x74, 0x6f, 0x6d, 0x65, 0x72, 0x31}, Id:289, Name:"", SubscriptionFee:0}
Index: 0
Name:  "id"
FullName: "service.Customer1.id"
Index: 1
Name:  "name"
FullName: "service.Customer1.name"
Index: 2
Name:  "subscription_fee"
FullName: "service.Customer1.subscription_fee"

github.com/gogo/protobuf/proto

Fails which is my expectation

$ go run main.go 
proto: no encoder for state impl.MessageState [GetProperties]
proto: no encoder for sizeCache int32 [GetProperties]
proto: no encoder for unknownFields []uint8 [GetProperties]
proto: no encoder for state impl.MessageState [GetProperties]
proto: no encoder for sizeCache int32 [GetProperties]
proto: no encoder for unknownFields []uint8 [GetProperties]
panic: proto: bad wiretype for field gen.Customer1.Name: got wiretype 0, want 2

goroutine 1 [running]:
main.main()
	/Users/emmanuelodeke/go/src/github.com/orijtech/experiments/main.go:26 +0x430
exit status 2

This behavior surprised me quite a bit.

[dsnet]

The current behavior is "correct". The defacto standard of correctness is generally what the C++ implementation does, which operates by putting fields with the wrong wire type in the unknown fields section. Certainly debatable whether that should have been done, but it is what it currently is and unlikely to change.

[odeke-em]

Thank you for the response @dsnet and great to catch you here!
Could we perhaps ask the rest of the protobuf team to document this problem? I haven't find any references to it, yet most users expect that they should get an error.

[dsnet]

The protobuf team is aware of the relatively unspecified nature of protobufs themselves. See protocolbuffers/protobuf#6188. It doesn't seem to be a high priority for them to come up with a specification.

[dsnet]

Closing as inactionable; the situation is unfortunate. The protobuf wire format is surprisingly lax and will accept all sorts of input as valid. It's actually surprisingly hard to get the parser to fail. Since we (the Go library team) aren't responsible for the protobuf specification itself, there's not much we can do.