Regression for non-addressable structs #16
Conversation
When we removed the default, unsafe mode, it caused a regression
where we are no longer able to deserialize things like
```go
map[string]struct{A string}
```
when we are deserializing into an already existing `struct` instance.
This is because the `struct` in the map is not addressable by default,
which caused a runtime panic. The previous unsafe version wrote
directly to the underlying memory and bypassed this check
We fix that by marking structs as an "immutable" type in the decoder,
forcing it to recreate the `struct` rather than try to write to
a non-addressable `struct`.
(This was found by testing `hashicorp/nomad`, which uses
`net-rpc-msgpack`, which uses this pattern of decoding two messages
into one value.)
| @@ -437,7 +437,7 @@ var immutableKindsSet = [32]bool{ | |||
| // reflect.Ptr | |||
| // reflect.Slice | |||
| reflect.String: true, | |||
| // reflect.Struct | |||
| reflect.Struct: true, | |||
There was a problem hiding this comment.
Do we have any handle on the performance impact of this? Could it lead to a lot of garbage if we decode elements into a large struct 1-by-1? (I haven't dug into the guts yet, so not sure if that's what it does)
There was a problem hiding this comment.
It definitely has a performance impact, but I haven't measured it. I don't feel comfortable with the unsafe code path, since it requires accessing the internal parts of pointers: https://github.com/hashicorp/go-msgpack/blob/v1.1.5/codec/helper_unsafe.go#L342-L346
There was a problem hiding this comment.
(I do have a branch I am about to push that fixes the benchmarking so we can more easily measure this. I wanted to get this bug fixed first though.)
There was a problem hiding this comment.
I think it would be valuable to downstream consumers to know a ballpark level of impact so that they can take action if needed, but in general I'm fine with taking a performance hit for safety and maintainability. Ignore that actually, it doesn't really make sense given the bug, the only comparable thing would be the unsafe version.
|
Thanks! |
When we removed the default, unsafe mode, it caused a regression where we are no longer able to deserialize things like
when we are deserializing into an already existing
structinstance. This is because thestructin the map is not addressable by default, which caused a runtime panic. The previous unsafe version wrote directly to the underlying memory and bypassed this checkWe fix that by marking structs as an "immutable" type in the decoder, forcing it to recreate the
structrather than try to write to a non-addressablestruct.(This was found by testing
hashicorp/nomad, which usesnet-rpc-msgpack, which uses this pattern of decoding two messages into one value.)