Stable marshaling of empty non-nil messages #59

New issue

Closed

opened 2023-11-20 17:01:33 +00:00 by alexvanin · 1 comment

alexvanin commented

2023-11-20 17:01:33 +00:00

Owner

Expected Behavior

Protobuf encodes nil message and empty message (all nested fields are zero values) differently.

	tb := aclgrpc.EACLTable{
		Version: new(refsgrpc.Version),
	}
	data, _ := proto.Marshal(&tb) // []byte{0x0a, 0x00}

	tb = aclgrpc.EACLTable{
		Version: nil,
	}
	data, _ = proto.Marshal(&tb) // []byte{}

Current Behavior

Stable marshaler ignores non-nil value of empty nested structure and removes it from wire. So this test below fails.

	oldTB := aclgrpc.EACLTable{
		Version: new(refsgrpc.Version), // manually set 0.0 version
	}
	oldData, err := proto.Marshal(&oldTB)
	require.NoError(t, err)

	var newTB acl.Table
	err = newTB.Unmarshal(oldData)
	require.NoError(t, err)

	require.Equal(t, oldData, newTB.StableMarshal(nil)) // Expected []byte{0x0a, 0x00}, got []byte{}

Possible Solution

Some nil checks via reflection instead of size checks helps to pass the test but it might be not quite good for performance. Example below fails in some tests but it works for this issue.

diff --git a/util/proto/marshal.go b/util/proto/marshal.go
index b16375a..65a8d54 100644
--- a/util/proto/marshal.go
+++ b/util/proto/marshal.go
@@ -10,6 +10,7 @@ import (
        "encoding/binary"
        "math"
        "math/bits"
+       "reflect"
 
        "google.golang.org/protobuf/encoding/protowire"
 )
@@ -255,11 +256,11 @@ func VarUIntSize(x uint64) int {
 }
 
 func NestedStructureMarshal[T stableMarshaller](field int64, buf []byte, v T) int {
-       n := v.StableSize()
-       if n == 0 {
+       if reflect.ValueOf(v).IsNil() {
                return 0
        }
 
+       n := v.StableSize()
        prefix := protowire.EncodeTag(protowire.Number(field), protowire.BytesType)
        offset := binary.PutUvarint(buf, prefix)
        offset += binary.PutUvarint(buf[offset:], uint64(n))
@@ -289,10 +290,11 @@ func NestedStructureSetMarshalData(field int64, parentData []byte, v setMarshalD
 }
 
 func NestedStructureSize[T stableMarshaller](field int64, v T) (size int) {
-       n := v.StableSize()
-       if n == 0 {
+       if reflect.ValueOf(v).IsNil() {
                return 0
        }
+
+       n := v.StableSize()
        size = protowire.SizeGroup(protowire.Number(field), protowire.SizeBytes(n))
        return
 }

Context

It was found during update checks from frostfs-node v0.36.0 to v0.37.0. Stored bearer token now fail signature check because of different marshaling output of EACL Table with non-nil empty version field.

## Expected Behavior Protobuf encodes nil message and empty message (all nested fields are zero values) differently. ```go tb := aclgrpc.EACLTable{ Version: new(refsgrpc.Version), } data, _ := proto.Marshal(&tb) // []byte{0x0a, 0x00} tb = aclgrpc.EACLTable{ Version: nil, } data, _ = proto.Marshal(&tb) // []byte{} ``` ## Current Behavior Stable marshaler ignores non-nil value of empty nested structure and removes it from wire. So this test below fails. ```go oldTB := aclgrpc.EACLTable{ Version: new(refsgrpc.Version), // manually set 0.0 version } oldData, err := proto.Marshal(&oldTB) require.NoError(t, err) var newTB acl.Table err = newTB.Unmarshal(oldData) require.NoError(t, err) require.Equal(t, oldData, newTB.StableMarshal(nil)) // Expected []byte{0x0a, 0x00}, got []byte{} ``` ## Possible Solution Some nil checks via reflection instead of size checks helps to pass the test but it might be not quite good for performance. Example below fails in some tests but it works for this issue. ```diff diff --git a/util/proto/marshal.go b/util/proto/marshal.go index b16375a..65a8d54 100644 --- a/util/proto/marshal.go +++ b/util/proto/marshal.go @@ -10,6 +10,7 @@ import ( "encoding/binary" "math" "math/bits" + "reflect" "google.golang.org/protobuf/encoding/protowire" ) @@ -255,11 +256,11 @@ func VarUIntSize(x uint64) int { } func NestedStructureMarshal[T stableMarshaller](field int64, buf []byte, v T) int { - n := v.StableSize() - if n == 0 { + if reflect.ValueOf(v).IsNil() { return 0 } + n := v.StableSize() prefix := protowire.EncodeTag(protowire.Number(field), protowire.BytesType) offset := binary.PutUvarint(buf, prefix) offset += binary.PutUvarint(buf[offset:], uint64(n)) @@ -289,10 +290,11 @@ func NestedStructureSetMarshalData(field int64, parentData []byte, v setMarshalD } func NestedStructureSize[T stableMarshaller](field int64, v T) (size int) { - n := v.StableSize() - if n == 0 { + if reflect.ValueOf(v).IsNil() { return 0 } + + n := v.StableSize() size = protowire.SizeGroup(protowire.Number(field), protowire.SizeBytes(n)) return } ```  ## Context It was found during update checks from frostfs-node v0.36.0 to v0.37.0. Stored bearer token now fail signature check because of different marshaling output of EACL Table with non-nil empty version field.