codec/types: NewAnyWithCustomTypeURL wastes memory if proto.Marshal returns a non-nil error

[odeke-em]

Summary of Bug

As part of my continual security audit, and in particular preparing for the Stargate release, I've noticed this code

cosmos-sdk/codec/types/any.go

Lines 72 to 79 in c54025d

	func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) {
	bz, err := proto.Marshal(v)
	return &Any{
	TypeUrl: typeURL,
	Value: bz,
	cachedValue: v,
	}, err
	}

regardless of the result from marshalling, it'll consume a bunch of bytes even if an error is returned. The risk isn't too bad as it is invoked in very few places (at least a few in the cosmos-sdk) but if someone wanted to make memory consumption grow they'd serialize to a type whose XXX_Marshal method ALWAYS returns an error for example

type errOnMarshal struct {
        testdata.Dog
}

var _ proto.Message = (*errOnMarshal)(nil)

var errAlways = fmt.Errorf("always erroring")

func (eom *errOnMarshal) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
        return nil, errAlways
}

Version

Stargate v0.40.X to tip

Steps to Reproduce

You can see the resutls by running this benchmark

type errOnMarshal struct {
        testdata.Dog
}

var _ proto.Message = (*errOnMarshal)(nil)

var errAlways = fmt.Errorf("always erroring")

func (eom *errOnMarshal) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) {
        return nil, errAlways
}

var sink interface{}

func BenchmarkBytesLost(b *testing.B) {
        eom := &errOnMarshal{}
        fauxURL := "/anyhere"
        b.ResetTimer()
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
                any, err := types.NewAnyWithCustomTypeURL(eom, fauxURL)
                if err == nil {
                        b.Fatal("err wasn't returned")
                }
                sink = any
        }
        if sink == nil {
                b.Fatal("benchmark didn't run")
        }
        sink = (interface{})(nil)
}

Remedy

The code needs to look like this

func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) {
        bz, err := proto.Marshal(v)
        if err != nil {
                return nil, err
        }
        return &Any{
                TypeUrl:     typeURL,
                Value:       bz,
                cachedValue: v,
        }, nil
}

Results

The results after running that benchmark, before and after

$ benchstat before.txt after.txt 
name         old time/op    new time/op    delta
BytesLost-8     142ns ± 6%      55ns ±12%   -61.65%  (p=0.000 n=9+10)

name         old alloc/op   new alloc/op   delta
BytesLost-8     96.0B ± 0%      0.0B       -100.00%  (p=0.000 n=10+10)

name         old allocs/op  new allocs/op  delta
BytesLost-8      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)

Please backport this to v0.40.X if you please.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[robert-zaremba]

Hmm, we are talking here about saving 96 bytes (1 allocation) vs increasing 90ns (60%) for an unusual scenario.
I think this doesn't make any difference in the runtime. That being said, thanks for doing detailed PRs 👍

[odeke-em]

Incorrect code needs to be fixed regardless of savings. The savings exhibited there were a by-product of correcting the code. If I didn’t show them, the change would still be valid to fix on its own, I wouldn’t be too fixated on the savings unless the entire premise was an optimization.

…

On Tue, Mar 16, 2021 at 6:17 AM Robert Zaremba ***@***.***> wrote: Hmm, we are talking here about saving 96 bytes (1 allocation) vs increasing 90ns (60%) for an unusual scenario. I think this doesn't make any difference in the runtime. That being said, thanks for doing detailed PRs 👍 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8537 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V7VHF5TYFC4LCF72FDTD5K6JANCNFSM4XITKAVQ> .