mirror of
https://github.com/odin-lang/Odin.git
synced 2026-09-01 17:53:35 +00:00
bugfix: core/encoding/json leaks the key and value of a malformed object
parse_object_body allocates an object key, then may fail in parse_colon or
parse_value before that key is ever inserted into the object. Its cleanup defer
only walks `obj`, so a key that never got there is unreachable to it. The caller
cannot free it either -- a failed parse returns a nil Value -- so it leaks.
The same applies to the parsed element on the duplicate-key path, and to both on
the out-of-memory path.
JSON5 makes this reachable from ordinary malformed input, because an unquoted
ident is a legal key and anything other than a colon after it fails. Plain JSON
leaks it too, via a quoted key.
before, measured with a tracking allocator over 8 inputs x 2 specs:
LEAK JSON5 colon fails after unquoted key 1 alloc / 7 bytes
LEAK JSON colon fails after quoted key 1 alloc / 2 bytes
LEAK JSON5 colon fails after quoted key 1 alloc / 2 bytes
LEAK JSON value fails after key 1 alloc / 2 bytes
LEAK JSON5 value fails after key 1 alloc / 2 bytes
LEAK JSON nested value fails 2 alloc / 4 bytes
LEAK JSON5 nested value fails 2 alloc / 4 bytes
LEAK JSON deep nesting fails 3 alloc / 6 bytes
LEAK JSON5 deep nesting fails 3 alloc / 6 bytes
LEAK JSON array element fails 1 alloc / 2 bytes
LEAK JSON5 array element fails 1 alloc / 2 bytes
total leaked allocations: 17
after, same probe:
total leaked allocations: 0
The leak scales with nesting depth -- one orphaned key per enclosing object -- so
a service parsing untrusted JSON leaks a little on every malformed request.
The fix marks the key and the element as owned by the loop iteration until they
are stored, and frees them otherwise. The duplicate-key path loses its explicit
delete, which the same mechanism now covers.
Found via odinfmt, which reported a 7-byte leak in a downstream test that parses
`{ broken not json` to check that invalid input is rejected.
Regression test added to tests/core/encoding/json: it reports
`17 leaks and 0 bad frees` without this change and passes with it. The existing
11 tests pass unchanged under -define:ODIN_TEST_FAIL_ON_BAD_MEMORY=true.
This commit is contained in:
@@ -279,12 +279,30 @@ parse_object_body :: proc(p: ^Parser, end_token: Token_Kind, loc := #caller_loca
|
||||
|
||||
for p.curr_token.kind != end_token {
|
||||
key := parse_object_key(p, p.allocator, loc) or_return
|
||||
|
||||
// `key` is allocated here and does not belong to `obj` until the insert below, so every
|
||||
// path that leaves in between has to free it. The cleanup defer at the top of this proc
|
||||
// only walks `obj`, so it cannot reach a key that never got there.
|
||||
//
|
||||
// JSON5 makes this reachable from ordinary malformed input: an unquoted ident is a legal
|
||||
// key, so `{ broken not json` allocates "broken" and then fails in parse_colon, leaking
|
||||
// it. The parse returns a nil Value, so the caller has nothing to destroy either.
|
||||
key_stored := false
|
||||
defer if !key_stored {
|
||||
delete(key, p.allocator, loc)
|
||||
}
|
||||
|
||||
parse_colon(p) or_return
|
||||
elem := parse_value(p, loc) or_return
|
||||
|
||||
// `elem` is owned by this iteration for the same reason, until it is stored.
|
||||
elem_stored := false
|
||||
defer if !elem_stored {
|
||||
destroy_value(elem, loc = loc)
|
||||
}
|
||||
|
||||
if key in obj {
|
||||
err = .Duplicate_Object_Key
|
||||
delete(key, p.allocator, loc)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -297,6 +315,8 @@ parse_object_body :: proc(p: ^Parser, end_token: Token_Kind, loc := #caller_loca
|
||||
return nil, .Out_Of_Memory
|
||||
}
|
||||
obj[key] = elem
|
||||
key_stored = true
|
||||
elem_stored = true
|
||||
}
|
||||
|
||||
if parse_comma(p) {
|
||||
|
||||
@@ -529,3 +529,34 @@ enumerated_array :: proc(t: ^testing.T) {
|
||||
testing.expect_value(t, unmarshaled, Sparse_Fruit_Stock)
|
||||
}
|
||||
}
|
||||
|
||||
@test
|
||||
malformed_object_frees_its_partial_allocations :: proc(t: ^testing.T) {
|
||||
// parse_object_body allocates a key, then may fail in parse_colon or parse_value before that
|
||||
// key is ever inserted into the object. That proc's cleanup only walks the object, so such a
|
||||
// key was orphaned: unreachable to the caller, which is handed a nil Value, and therefore
|
||||
// leaked on every malformed input of this shape.
|
||||
//
|
||||
// JSON5 makes it reachable from ordinary input, since an unquoted ident is a legal key and
|
||||
// anything other than a colon after it fails -- but plain JSON leaks it too, via a quoted key.
|
||||
//
|
||||
// The test runner's memory tracking is what asserts this: each case only has to parse and be
|
||||
// destroyed without leaving an allocation behind.
|
||||
cases := []string {
|
||||
`{ broken not json`,
|
||||
`{"a" 1}`,
|
||||
`{"a": }`,
|
||||
`{"a": 1, "a": 2}`,
|
||||
`{"a": {"b" 1}}`,
|
||||
`{"a": {"b": {"c" 1}}}`,
|
||||
`{"a": [1, }`,
|
||||
}
|
||||
|
||||
for spec in ([]json.Specification{.JSON, .JSON5}) {
|
||||
for src in cases {
|
||||
value, err := json.parse(transmute([]u8)src, spec = spec)
|
||||
testing.expectf(t, err != nil, "%q must not parse under %v", src, spec)
|
||||
json.destroy_value(value)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user