Skip to content

Fix Delete matching keys that contain a backslash - #293

Open
waw666waw666 wants to merge 1 commit into
buger:masterfrom
waw666waw666:fix-delete-escaped-path-key
Open

waw666waw666 wants to merge 1 commit into
buger:masterfrom
waw666waw666:fix-delete-escaped-path-key

Conversation

@waw666waw666

Copy link
Copy Markdown

Description:

Delete and DeleteFound treat a path key that contains a backslash differently from Get, Set and EachKey, and in one case delete the wrong part of the document.

data := []byte(`{"\"q":1,"keep":0,"a\\b":2}`)

v, _, _, _ := jsonparser.Get(data, `a\b`)       // 2
out, found := jsonparser.DeleteFound(data, `a\b`)
// out = {}, found = true   ("\"q" and "keep" are gone too)

jsonparser.Delete([]byte(`{"a\\b":1,"c":2}`), `a\b`)
// {"a\\b":1,"c":2}   (nothing deleted, although Get finds the key)

Cause: findKeyStartConfig, which Delete uses for the last path element, JSON-unescapes the path key before comparing it with the document's keys. searchKeys (used by Get, Set, EachKey) compares the path as is against the unescaped document key, and the existing Get tests rely on that ({"a\\b":1} is found with the path a\b). So in Delete, \b in the path turned into a backspace and the key was not found.

The unescaped path key was also stored in the same stackbuf that is then used to unescape each escaped key of the document. The first escaped key of the same length overwrote the path key and so compared equal to itself, and findKeyStart returned its offset. The value end still came from internalGet with the real path, so Delete cut from the earlier key through the end of the matched value, removing every member in between.

Fix: compare the path key as is in findKeyStartConfig, like searchKeys does. That removes both the mismatch and the shared buffer.

Tests: added cases to deleteTests (escaped key, Unicode-escaped key, an earlier escaped key of the same length at top level and nested, and a path that only equals the escaped form of a key). Three of them fail on master and pass with this change. go vet shows no new findings and gofmt is clean for the changed lines. The full go test ./... result is the same as on master: TestOracleSetPr286Regression already fails on master, and #290 looks related.

Benchmark before change:

BenchmarkJsonParserDeleteMedium-2   297740   8943 ns/op   10175 B/op   7 allocs/op
BenchmarkJsonParserDelSmall-2      1417653   1694 ns/op     848 B/op   8 allocs/op

Benchmark after change:

BenchmarkJsonParserDeleteMedium-2   347984   6763 ns/op    9984 B/op   4 allocs/op
BenchmarkJsonParserDelSmall-2      1774606   1289 ns/op     591 B/op   3 allocs/op

(go test -bench JsonParserDel -benchmem ./benchmark/, arm64. The time numbers are noisy on this machine. The drop in allocations is consistent: stackbuf no longer escapes, because the path key isn't stored in it anymore.)

findKeyStart, which Delete uses to find the key of the last path element,
JSON-unescaped the path key before comparing it with the (unescaped) keys
in the document. Get, Set and EachKey compare the path as is, so for a
path with a backslash Delete looked for a different key than Get:
Delete(`{"a\\b":1}`, `a\b`) left the key in place, because `\b` became a
backspace.

Worse, the unescaped path key lived in the same stack buffer that is
then used to unescape each escaped key in the document, so the first
escaped key of the same length overwrote it and compared equal to
itself. Delete then cut from that earlier key to the end of the real
value: Delete(`{"\"q":1,"keep":0,"a\\b":2}`, `a\b`) returned {}.

Compare the path key as is, like searchKeys does.
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

Successfully merging this pull request may close these issues.

1 participant