The current implementation of bytesToStr uses an unsafe
reflect.StringHeader value. Change the implementation of this function
to a safe and simpler version.
To explain what could go wrong here is some example code:
var d []byte
d = someFunctionThatReturnsBytes()
s := bytesToStr(d)
doSomethingWith(s)
When this code gets compiled bytesToStr would get inlined and the
code would be like the following. I have included in comments at which
point things could go wrong:
var d []byte
d = someFunctionThatReturnsBytes()
h := (*reflect.SliceHeader)(unsafe.Pointer(&d))
shdr := reflect.StringHeader{Data: h.Data, Len: h.Len}
// At this point in time d and d.Data have nothing referencing them anymore
// shdr.Data is an uintptr so it will be ignored by the GC.
// This means d and d.Data can be garbage collected here.
// Internally strings don't use a uintptr for the data, but since this is
// just a reflect.StringHeader and not a real string yet that doesn't apply
// here.
// This is why https://pkg.go.dev/unsafe#Pointer says:
// In general, reflect.SliceHeader and reflect.StringHeader should be
// used only as *reflect.SliceHeader and *reflect.StringHeader pointing
// at actual slices or strings, never as plain structs.
s := *(*string)(unsafe.Pointer(&shdr))
// Only at this point s.Data points to d.Data again and the backing storage
// of d won't be garbage collected anymore.
doSomethingWith(s)
The chance of this going wrong is probably so small that nobody ever
noticed it happening, but it is there.
* tests: don't ignore errors, verify them carefully
* fix unescaping of \\\\\" and such sequences
* tests: add Unmarshal test cases for escaped sequences
There was a small glitch:
numbers as strings where not unescaped in -disable_members_unescape
mode, though this mode was implied to affect field names only.
There are 2 issues with current implementation:
1. It performs a plain byte to byte loop to find string boundaries and
perform unescaping. Replace this with bytes.IndexByte() implementation.
2. It performs unescaping of string values even when this is not really
needed, e.g. for members which are absent in target data structure.
This patch fixes both issues and results in ~12% faster BenchmarkEJ_Unmarshal_M-8,
plus number of allocations goes down from 128 to 52 (!):
benchmark old MB/s new MB/s speedup
BenchmarkEJ_Unmarshal_M-8 317.99 356.27 1.12x
BenchmarkEJ_Unmarshal_S-8 142.19 139.77 0.98x
benchmark old allocs new allocs delta
BenchmarkEJ_Unmarshal_M-8 128 52 -59.38%
BenchmarkEJ_Unmarshal_S-8 3 3 +0.00%
The rest of benchmarks are w/o changes.
NOTE: performance can be improved up to 1.24x if unescaping is not
performed for member names.
Signed-off-by: Kirill Korotaev <kirillx@gmail.com>
This change modified the `json.Number` unmarshaling to handle `null`
values without error, similar to `encoding/json`. These values are
become `json.Number("")`.
This also modifies the json.Number value returned on error to be
`json.Number("")` as that is the zero value.
See https://play.golang.org/p/knZLugaqnni
Alexander Tumin
Erik DubbelboerandGitHub
YaroslavPodorvanovandGitHub
kirillxandGitHub
Alexandr Mayorskiy
Carlo Alberto Ferraris
Vasily RomanovandGitHub
Kirill Motkov
Muxian Wu
alexej-v
Dmitry DoroginandGitHub
Gregory Oschwald
Levi Gross