Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audit: codec/types: AnyUnpacker unnecessary creates a reflect.Value, boxes it then casts its to an interface just to see if it implements proto.Message, but could really just check if the original type implements the interface #17084

Closed
odeke-em opened this issue Jul 20, 2023 · 0 comments · Fixed by #17085
Labels
Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@odeke-em
Copy link
Collaborator

Summary of Bug

Noticed while auditing changes between v0.47.0 and the latest that the code in question in cases that don't matter creates an unnecessary reflect.Value, boxes its zero value to an interface then performs a type assertion to see if it implements proto.Message

Version

3d15233

Suggestion

We can simply check against the original type that it implements proto.Message like this

diff --git a/codec/types/interface_registry.go b/codec/types/interface_registry.go
index 50a5ca01c..c8b18f08d 100644
--- a/codec/types/interface_registry.go
+++ b/codec/types/interface_registry.go
@@ -12,6 +12,8 @@ import (
        "cosmossdk.io/x/tx/signing"
 )
 
+var protoMessageType = reflect.TypeOf((*proto.Message)(nil)).Elem()
+
 // AnyUnpacker is an interface which allows safely unpacking types packed
 // in Any's against a whitelist of registered types
 type AnyUnpacker interface {
@@ -305,11 +307,13 @@ func (registry *interfaceRegistry) UnpackAny(any *Any, iface interface{}) error
                return fmt.Errorf("no concrete type registered for type URL %s against interface %T", any.TypeUrl, iface)
        }
 
-       msg, ok := reflect.New(typ.Elem()).Interface().(proto.Message)
-       if !ok {
-               return fmt.Errorf("can't proto unmarshal %T", msg)
+       // Firstly check if the type implements proto.Message to avoid
+        // unnecessary invocations to reflect.New
+       if !typ.Implements(protoMessageType) {
+               return fmt.Errorf("can't proto unmarshal %T", typ)
        }
 
+       msg := reflect.New(typ.Elem()).Interface().(proto.Message)
        err := proto.Unmarshal(any.Value, msg)
        if err != nil {
                return err

/cc @ elias-orijtech

@odeke-em odeke-em added the Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Jul 20, 2023
odeke-em added a commit that referenced this issue Jul 20, 2023
Creates a type from proto.Message which can be type asserted against
with reflect.Type.Implements and is a quick check instead of firstly
constructing an entire reflect.Value zero value then casting its
value to an interface/any then manually type asserting against
proto.Message.

Fixes #17084
odeke-em added a commit that referenced this issue Jul 20, 2023
Creates a type from proto.Message which can be type asserted against
with reflect.Type.Implements and is a quick check instead of firstly
constructing an entire reflect.Value zero value then casting its
value to an interface/any then manually type asserting against
proto.Message.

Fixes #17084
odeke-em added a commit that referenced this issue Jul 23, 2023
Creates a type from proto.Message which can be type asserted against
with reflect.Type.Implements and is a quick check instead of firstly
constructing an entire reflect.Value zero value then casting its
value to an interface/any then manually type asserting against
proto.Message.

Fixes #17084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant