From 6ccde7f80b2ecc4b05967443c45cba6a4fdf8108 Mon Sep 17 00:00:00 2001 From: "user.name" Date: Mon, 31 Aug 2026 10:38:37 -0700 Subject: [PATCH] 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. --- core/encoding/json/parser.odin | 22 +++++++++++++- tests/core/encoding/json/test_core_json.odin | 31 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/core/encoding/json/parser.odin b/core/encoding/json/parser.odin index 91bc42e4c..f6ca0b3ca 100644 --- a/core/encoding/json/parser.odin +++ b/core/encoding/json/parser.odin @@ -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) { diff --git a/tests/core/encoding/json/test_core_json.odin b/tests/core/encoding/json/test_core_json.odin index 5fcdc7a27..1ef84e07b 100644 --- a/tests/core/encoding/json/test_core_json.odin +++ b/tests/core/encoding/json/test_core_json.odin @@ -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) + } + } +}