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

Interface nil value not set on decoding, with panic #304

Open
l0nax opened this issue May 12, 2021 · 3 comments
Open

Interface nil value not set on decoding, with panic #304

l0nax opened this issue May 12, 2021 · 3 comments

Comments

@l0nax
Copy link

l0nax commented May 12, 2021

The interface field isn't set to nil when msgpack code is nil.

Expected Behavior

The field value of the struct is set to nil.

Current Behavior

Application is panic'ing:

  reflect: reflect.Value.Set using unaddressable value
  /root/.gvm/gos/go1.16/src/reflect/value.go:260

  Full Stack Trace
  reflect.flag.mustBeAssignableSlow(0x16)
        /root/.gvm/gos/go1.16/src/reflect/value.go:260 +0x138
  reflect.flag.mustBeAssignable(...)
        /root/.gvm/gos/go1.16/src/reflect/value.go:247
  reflect.Value.Set(0x946980, 0xc000612078, 0x16, 0x946980, 0x0, 0x16)
        /root/.gvm/gos/go1.16/src/reflect/value.go:1558 +0x3b
  github.com/vmihailenco/msgpack/v5.ptrValueDecoder.func1(0xc000154480, 0x946980, 0xc000612078, 0x16, 0x946980, 0xc000612078)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode_value.go:130 +0x29d
  github.com/vmihailenco/msgpack/v5.(*Decoder).DecodeValue(0xc000154480, 0x946980, 0xc000612078, 0x16, 0xc000612078, 0x16)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode.go:309 +0x8e
  github.com/vmihailenco/msgpack/v5.decodeInterfaceValue(0xc000154480, 0x971e00, 0xc00000e5d8, 0x194, 0x1, 0x1)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode_value.go:184 +0x9a
  github.com/vmihailenco/msgpack/v5.(*field).DecodeValue(0xc00031c780, 0xc000154480, 0x9ac6e0, 0xc00000e5d0, 0x199, 0x0, 0x0)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/types.go:118 +0x9a
  github.com/vmihailenco/msgpack/v5.(*Decoder).decodeStruct(0xc000154480, 0x9ac6e0, 0xc00000e5d0, 0x199, 0x2, 0xc0001b8b18, 0x8575c5)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode_map.go:324 +0x24f
  github.com/vmihailenco/msgpack/v5.decodeStructValue(0xc000154480, 0x9ac6e0, 0xc00000e5d0, 0x199, 0x9ac6e0, 0x8)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode_map.go:282 +0x330
  github.com/vmihailenco/msgpack/v5.(*Decoder).DecodeValue(0xc000154480, 0x9ac6e0, 0xc00000e5d0, 0x199, 0xc00000e5d0, 0x199)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode.go:309 +0x8e
  github.com/vmihailenco/msgpack/v5.(*Decoder).Decode(0xc000154480, 0x946a40, 0xc00000e5d0, 0x0, 0xc0001b8c20)
        /root/.go/pkg/mod/github.com/vmihailenco/msgpack/[email protected]/decode.go:288 +0x178
[...]

Possible Solution

When a field is of type interface{} function decodeInterfaceValue(…) will be called.
It should be checked if the next code, is nil and the nil behavior should be respected by the interfaceValue(…) method.

We've developed a patch – debugged and tested – but since we're not really familiar with the library this patch might be incomplete.

diff --git a/decode_value.go b/decode_value.go
index d2ff2ae..4fda5bd 100644
--- a/decode_value.go
+++ b/decode_value.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+
+	"github.com/vmihailenco/msgpack/v5/msgpcode"
 )
 
 var (
@@ -178,7 +180,9 @@ func decodeBoolValue(d *Decoder, v reflect.Value) error {
 }
 
 func decodeInterfaceValue(d *Decoder, v reflect.Value) error {
-	if v.IsNil() {
+	c, _ := d.PeekCode()
+
+	if v.IsNil() || c == msgpcode.Nil {
 		return d.interfaceValue(v)
 	}
 	return d.DecodeValue(v.Elem())
@@ -199,6 +203,8 @@ func (d *Decoder) interfaceValue(v reflect.Value) error {
 		}
 
 		v.Set(reflect.ValueOf(vv))
+	} else if v.CanSet() {
+		v.Set(reflect.Zero(v.Type()))
 	}
 
 	return nil

Steps to Reproduce

package main

import (
	"bytes"
	"log"

	"github.com/vmihailenco/msgpack/v5"
)

type ReplyData struct {
	Result int
	Data   interface{}
}

type Recipe struct {
	Name string
}

func main() {
	myData := &ReplyData{
		Result: 0,
		Data:   nil,
	}

	rpObj := ReplyData{Result: 0, Data: &Recipe{}}

	s := serialize(myData)
	deserialize(s, &rpObj)

        fmt.Printf("%+v", rpObj)
	fmt.Printf("%+v", rpObj.Data)
}

func deserialize(raw []byte, data interface{}) {
	dec := msgpack.GetDecoder()
	defer msgpack.PutDecoder(dec)

	dec.Reset(bytes.NewReader(raw))
	dec.DisallowUnknownFields(true)

	if err := dec.Decode(data); err != nil {
		log.Fatal(err)
	}
}

func serialize(data interface{}) []byte {
	var buf bytes.Buffer

	enc := msgpack.GetEncoder()
	defer msgpack.PutEncoder(enc)

	enc.Reset(&buf)
	enc.UseCompactInts(true)
	enc.UseCompactFloats(true)

	if err := enc.Encode(data); err != nil {
		log.Fatal(err)
	}

	return buf.Bytes()
}

Context (Environment)

Detailed Description

See above

Possible Implementation

@l0nax l0nax changed the title Interface nil value not set on decoding with panic Interface nil value not set on decoding, with panic May 12, 2021
@l0nax
Copy link
Author

l0nax commented Sep 29, 2021

Any updates to this issue @vmihailenco?

@rnov
Copy link

rnov commented Jul 29, 2022

@l0nax facing the same issue, despite not being merged, has it worked for you ?

@rnov
Copy link

rnov commented Aug 2, 2022

@vmihailenco have you considered this fix, are there any concerns? It seems the issue has not been addressed in the latter versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants