tidwall / gjson

Get JSON values quickly - JSON parser for Go
MIT License
13.95k stars 841 forks source link

"slice bounds out of range" regression introduced in v1.11.0 #263

Closed edwargix closed 2 years ago

edwargix commented 2 years ago

Hiya,

The mautrix-go library uses gjson. Some of its json tests broke on commit 2c9fd24 (aka tag v1.11.0), as can be seen under "Steps to reproduce". An issue for this was filed in mautrix-go: https://github.com/mautrix/go/issues/52

The mautrix-go function seems to be using the json slice optimization described under Working with Bytes. Perhaps this has something to do with the problem? https://github.com/mautrix/go/blob/v0.10.9/crypto/canonicaljson/json.go#L266-L280

Steps to reproduce

1. checkout mautrix-go version v0.10.9 (the latest release)

$ git clone https://github.com/mautrix/go mautrix-go
$ cd mautrix-go
$ git checkout v0.10.9
$ git show
commit 6feb65ab23f1b52b4e35fb9b530ebe8f8049a561 (HEAD, tag: v0.10.9)
Author: Tulir Asokan <tulir@maunium.net>
Date:   Tue Jan 4 20:00:42 2022 +0200

    Bump version to v0.10.9

diff --git a/version.go b/version.go
index a9b8a87..1c9d158 100644
--- a/version.go
+++ b/version.go
@@ -1,5 +1,5 @@
 package mautrix

-const Version = "v0.10.8"
+const Version = "v0.10.9"

 var DefaultUserAgent = "mautrix-go/" + Version

2. run the crypto/canonicaljson tests using gjson v1.10.2 and 7cadbb5 (the commit after v1.10.2)

Notice that the tests run fine for these version of gjson:

$ grep gjson go.mod
    github.com/tidwall/gjson v1.10.2
$ go test -v ./crypto/canonicaljson
=== RUN   TestSortJSON
--- PASS: TestSortJSON (0.00s)
=== RUN   TestCompactJSON
--- PASS: TestCompactJSON (0.00s)
=== RUN   TestReadHex
--- PASS: TestReadHex (0.00s)
PASS
ok      maunium.net/go/mautrix/crypto/canonicaljson 0.002s
$ go get -u github.com/tidwall/gjson@7cadbb5
go get: upgraded github.com/tidwall/gjson v1.10.2 => v1.10.3-0.20211028160900-7cadbb575617
$ go test -v ./crypto/canonicaljson
=== RUN   TestSortJSON
--- PASS: TestSortJSON (0.00s)
=== RUN   TestCompactJSON
--- PASS: TestCompactJSON (0.00s)
=== RUN   TestReadHex
--- PASS: TestReadHex (0.00s)
PASS
ok      maunium.net/go/mautrix/crypto/canonicaljson (cached)

3. re-run the same tests using gjson v1.11.0 (commit 2c9fd24, aka first commit after 7cadbb5 from step 2)

Notice that we experience a panic on this version of gjson:

$ go get -u github.com/tidwall/gjson@2c9fd24
go get: upgraded github.com/tidwall/gjson v1.10.3-0.20211028160900-7cadbb575617 => v1.11.0
$ go test -v ./crypto/canonicaljson
=== RUN   TestSortJSON
    json_test.go:27: SortJSON("[{\"b\":\"two\",\"a\":1}]"): want "[{\"a\":1,\"b\":\"two\"}]" got "[{a\"::},b\"::two\",}]"
--- FAIL: TestSortJSON (0.00s)
panic: runtime error: slice bounds out of range [:27] with capacity 25 [recovered]
    panic: runtime error: slice bounds out of range [:27] with capacity 25

goroutine 34 [running]:
testing.tRunner.func1.2({0x53a300, 0xc000158060})
    /usr/lib/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
    /usr/lib/go/src/testing/testing.go:1212 +0x218
panic({0x53a300, 0xc000158060})
    /usr/lib/go/src/runtime/panic.go:1038 +0x215
maunium.net/go/mautrix/crypto/canonicaljson.RawJSONFromResult(...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:274
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject.func1({0x3, {0xc0001120a8, 0x3}, {0xc0001120a9, 0x1}, 0x0, 0x18, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:113 +0x2d4
github.com/tidwall/gjson.Result.ForEach({0x5, {0xc0001120a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/.local/share/go/pkg/mod/github.com/tidwall/gjson@v1.11.0/gjson.go:289 +0x4e5
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject({0x5, {0xc0001120a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:110 +0x165
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONValue({0x5, {0xc0001120a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:62 +0x289
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject({0x5, {0xc000112090, 0x25}, {0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:136 +0x5f3
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONValue({0x5, {0xc000112090, 0x25}, {0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:62 +0x289
maunium.net/go/mautrix/crypto/canonicaljson.SortJSON({0xc000112060, 0x54d3f5, 0x30}, {0x0, 0x0, 0x0})
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:51 +0x23a
maunium.net/go/mautrix/crypto/canonicaljson.testSortJSON(0xc000118ea0, {0x54d3f5, 0x25}, {0x54d3d0, 0xc000154760})
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json_test.go:23 +0x65
maunium.net/go/mautrix/crypto/canonicaljson.TestSortJSON(0x0)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json_test.go:33 +0x58
testing.tRunner(0xc000118ea0, 0x5509e0)
    /usr/lib/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
    /usr/lib/go/src/testing/testing.go:1306 +0x35a
FAIL    maunium.net/go/mautrix/crypto/canonicaljson 0.006s
FAIL

4. re-run the tests with gjson v1.13.0 (the latest release)

Notice that the problem persists:

$ go get -u github.com/tidwall/gjson@v1.13.0
go: downloading github.com/tidwall/gjson v1.13.0
go get: upgraded github.com/tidwall/gjson v1.11.0 => v1.13.0
$ go test -v ./crypto/canonicaljson
=== RUN   TestSortJSON
    json_test.go:27: SortJSON("[{\"b\":\"two\",\"a\":1}]"): want "[{\"a\":1,\"b\":\"two\"}]" got "[{a\"::},b\"::two\",}]"
--- FAIL: TestSortJSON (0.00s)
panic: runtime error: slice bounds out of range [:27] with capacity 25 [recovered]
    panic: runtime error: slice bounds out of range [:27] with capacity 25

goroutine 18 [running]:
testing.tRunner.func1.2({0x53a300, 0xc0000d8060})
    /usr/lib/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
    /usr/lib/go/src/testing/testing.go:1212 +0x218
panic({0x53a300, 0xc0000d8060})
    /usr/lib/go/src/runtime/panic.go:1038 +0x215
maunium.net/go/mautrix/crypto/canonicaljson.RawJSONFromResult(...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:274
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject.func1({0x3, {0xc0000920a8, 0x3}, {0xc0000920a9, 0x1}, 0x0, 0x18, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:113 +0x2d4
github.com/tidwall/gjson.Result.ForEach({0x5, {0xc0000920a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/.local/share/go/pkg/mod/github.com/tidwall/gjson@v1.13.0/gjson.go:293 +0x52c
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject({0x5, {0xc0000920a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:110 +0x165
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONValue({0x5, {0xc0000920a7, 0xd}, {0x0, 0x0}, 0x0, 0x17, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:62 +0x289
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONObject({0x5, {0xc000092090, 0x25}, {0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:136 +0x5f3
maunium.net/go/mautrix/crypto/canonicaljson.sortJSONValue({0x5, {0xc000092090, 0x25}, {0x0, 0x0}, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:62 +0x289
maunium.net/go/mautrix/crypto/canonicaljson.SortJSON({0xc000092060, 0x54d401, 0x30}, {0x0, 0x0, 0x0})
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json.go:51 +0x23a
maunium.net/go/mautrix/crypto/canonicaljson.testSortJSON(0xc000098ea0, {0x54d401, 0x25}, {0x54d3dc, 0xc00002e760})
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json_test.go:23 +0x65
maunium.net/go/mautrix/crypto/canonicaljson.TestSortJSON(0x0)
    /home/edwargix/git/mautrix-go/crypto/canonicaljson/json_test.go:33 +0x58
testing.tRunner(0xc000098ea0, 0x550a08)
    /usr/lib/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
    /usr/lib/go/src/testing/testing.go:1306 +0x35a
FAIL    maunium.net/go/mautrix/crypto/canonicaljson 0.009s
FAIL
tidwall commented 2 years ago

gjson v1.11.0 fixed a couple bugs that are related to your specific issue.

1) In v1.10.2 and below the Parse function returned a Result where the Index field is always zero.

This was wrong. It should have been an Index that is the position of the first character of its Raw value in the original json.

2) Also in v1.10.2 and below the ForEach iterator returned values with an Index that was relative to the parent, not the original json.

This too was wrong. It should have also been an Index that is the position of the first character of its Raw value in the original json.

For example:

In v1.10.2 (wrong):

json := "\t\n[{\"key\":9007199254740991},{\"key\":9007199298273917}]"
println(Parse(json).Index)
Parse(json).ForEach(func(key, value Result) bool {
    println(value.Index)
    return true
})
// output:
// 0
// 1
// 26

In v1.11.0 (right):

json := "\t\n[{\"key\":9007199254740991},{\"key\":9007199298273917}]"
println(Parse(json).Index)
Parse(json).ForEach(func(key, value Result) bool {
    println(value.Index)
    return true
})
// output:
// 2
// 3
// 28

I hope this information is understandable.

I took the liberty to change your crypto/canonicaljson/json.go code so that it will fix the issue on your side:

https://gist.github.com/tidwall/7549bdbf3fae285dd20620c63d4672d5

edwargix commented 2 years ago

I took the liberty to change your crypto/canonicaljson/json.go code so that it will fix the issue on your side:

https://gist.github.com/tidwall/7549bdbf3fae285dd20620c63d4672d5

Thank you much!