From 64c9ee98b7684cf2156f620cbab4dbb6081b9771 Mon Sep 17 00:00:00 2001 From: Daniel Martí Date: Sun, 28 Jul 2019 20:16:14 -0700 Subject: encoding/json: error when encoding a pointer cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise we'd panic with a stack overflow. Most programs are in control of the data being encoded and can ensure there are no cycles, but sometimes it's not that simple. For example, running a user's html template with script tags can easily result in crashes if the user can find a pointer cycle. Adding the checks via a map to every ptrEncoder.encode call slowed down the benchmarks below by a noticeable 13%. Instead, only start doing the relatively expensive pointer cycle checks if we're many levels of pointers deep in an encode state. A threshold of 1000 is small enough to capture pointer cycles before they're a problem (the goroutine stack limit is currently 1GB, and I needed close to a million levels to reach it). Yet it's large enough that reasonable uses of the json encoder only see a tiny 1% slow-down due to the added ptrLevel field and check. name old time/op new time/op delta CodeEncoder-8 2.34ms ± 1% 2.37ms ± 0% +1.05% (p=0.000 n=10+10) CodeMarshal-8 2.42ms ± 1% 2.44ms ± 0% +1.10% (p=0.000 n=10+10) name old speed new speed delta CodeEncoder-8 829MB/s ± 1% 820MB/s ± 0% -1.04% (p=0.000 n=10+10) CodeMarshal-8 803MB/s ± 1% 795MB/s ± 0% -1.09% (p=0.000 n=10+10) name old alloc/op new alloc/op delta CodeEncoder-8 43.1kB ± 8% 42.5kB ±10% ~ (p=0.989 n=10+10) CodeMarshal-8 1.99MB ± 0% 1.99MB ± 0% ~ (p=0.254 n=9+6) name old allocs/op new allocs/op delta CodeEncoder-8 0.00 0.00 ~ (all equal) CodeMarshal-8 1.00 ± 0% 1.00 ± 0% ~ (all equal) Finally, add a few tests to ensure that the code handles the edge cases properly. Fixes #10769. Change-Id: I73d48e0cf6ea140127ea031f2dbae6e6a55e58b8 Reviewed-on: https://go-review.googlesource.com/c/go/+/187920 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Bjørn Erik Pedersen Reviewed-by: Andrew Bonventre --- src/encoding/json/encode_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'src/encoding/json/encode_test.go') diff --git a/src/encoding/json/encode_test.go b/src/encoding/json/encode_test.go index 40f16d86ff..5110c7de9b 100644 --- a/src/encoding/json/encode_test.go +++ b/src/encoding/json/encode_test.go @@ -138,10 +138,45 @@ func TestEncodeRenamedByteSlice(t *testing.T) { } } +type SamePointerNoCycle struct { + Ptr1, Ptr2 *SamePointerNoCycle +} + +var samePointerNoCycle = &SamePointerNoCycle{} + +type PointerCycle struct { + Ptr *PointerCycle +} + +var pointerCycle = &PointerCycle{} + +type PointerCycleIndirect struct { + Ptrs []interface{} +} + +var pointerCycleIndirect = &PointerCycleIndirect{} + +func init() { + ptr := &SamePointerNoCycle{} + samePointerNoCycle.Ptr1 = ptr + samePointerNoCycle.Ptr2 = ptr + + pointerCycle.Ptr = pointerCycle + pointerCycleIndirect.Ptrs = []interface{}{pointerCycleIndirect} +} + +func TestSamePointerNoCycle(t *testing.T) { + if _, err := Marshal(samePointerNoCycle); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + var unsupportedValues = []interface{}{ math.NaN(), math.Inf(-1), math.Inf(1), + pointerCycle, + pointerCycleIndirect, } func TestUnsupportedValues(t *testing.T) { -- cgit v1.3