Merge pull request #7487 from i-api/fix/json-parser-leaks-partial-object

bugfix: core/encoding/json leaks the key and value of a malformed object

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) {

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)
+		}
+	}
+}