`_json`: stale markers entries on error paths in `encoder_listencode_obj`

[raminfp]

Bug report

Bug description:

encoder_listencode_obj() in Modules/_json.c calls PyDict_SetItem to register
an object in the circular-reference tracking dict (markers), but skips
PyDict_DelItem on all three error paths. This leaves a permanent strong reference
to the object inside markers.

Modules/_json.c - encoder_listencode_obj()

Root cause

/* L1444 */ PyDict_SetItem(s->markers, ident, obj);   // strong ref added

/* L1449 */ newobj = PyObject_CallOneArg(s->defaultfn, obj);
/* L1450 */ if (newobj == NULL) {
/* L1452 */     return -1;   // PATH A - no PyDict_DelItem
            }

/* L1455 */ if (_Py_EnterRecursiveCall("...")) {
/* L1458 */     return -1;   // PATH B - no PyDict_DelItem
            }

/* L1460 */ rv = encoder_listencode_obj(s, writer, newobj, newline_indent);
/* L1464 */ if (rv) {
/* L1466 */     return -1;   // PATH C - no PyDict_DelItem
            }

/* L1469 */ PyDict_DelItem(s->markers, ident);  // only reached on success

Impact

PATH A - default() raises

markers holds a strong reference to obj after failure.
del obj does not free it as long as markers is alive.

import _json, json, gc, sys

markers = {}
enc = _json.make_encoder(markers, lambda o: (_ for _ in ()).throw(TypeError()),
    json.encoder.encode_basestring_ascii, None, ': ', ', ', False, False, True)

class Obj: pass
obj = Obj()

rc_before = sys.getrefcount(obj)   # 2
try:
    enc(obj, 0)
except TypeError:
    pass
rc_after = sys.getrefcount(obj)    # 3  elevated by markers

del obj
gc.collect()
assert list(markers.values())[0]   # obj still alive - cannot be freed

PATH B - RecursionError

Each recursive frame adds one entry to markers without cleanup.
Triggerable via the public json.dumps() API.

import json

class Obj: pass

class R(json.JSONEncoder):
    def default(self, o): return Obj()

try:
    json.dumps(Obj(), cls=R)
except RecursionError:
    pass
# ~158 000 Obj instances were held alive in markers during the call

PATH C - recursive encoding of default()'s return value fails

One failed nested encoding leaves 3 stale entries: the original object,
the dict returned by default(), and the unencodable value inside it.

import _json, json, sys

class Obj: pass
class Inner: pass

def fn(o):
    if isinstance(o, Obj): return {"key": Inner()}
    raise TypeError

markers = {}
enc = _json.make_encoder(markers, fn,
    json.encoder.encode_basestring_ascii, None, ': ', ', ', False, False, True)

obj = Obj()
try:
    enc(obj, 0)
except TypeError:
    pass

assert len(markers) == 3        # Obj, dict, Inner - all leaked
assert sys.getrefcount(obj) > 2 # refcount elevated by markers

Expected behavior

PyDict_DelItem(s->markers, ident) should be called before every return -1
in the else-branch, mirroring the cleanup already present in
encoder_listencode_dict() and encoder_listencode_list() on their success paths.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-146152: Fix memory leak in _json encoder error paths #146164

[okiemute04]

The root cause is clear encoder_listencode_obj() adds objects to the markers dict, but only removes them on the success path, not on the three error paths. What's needed is to clean up references on error paths, then add PyDict_DelItem(s->markers, ident) before each return -1 in the error paths, this ensure proper cleanup and prevents memory leaks. I will send a pr now for this fix. This should solve the issue. Any suggestions @raminfp @StanFromIreland

[serhiy-storchaka]

The markers dictionary is created in JSONEncoder.iterencode() and destroyed when that method is finished -- either normally or by raising an exception. In is not used in any public API and never leaked outside of iterencode(), It is not worth to remove objects one by one if the whole dictionary will be cleared.

[raminfp]

Thank you for the clarification, you are correct that for PATH A and PATH C, the stale entries are harmless because markers is local to iterencode() and gets released when the call returns.

However, PATH B (the _Py_EnterRecursiveCall path) is a different case worth considering.

When default() returns a new unknown object on every call, the recursion drives encoder_listencode_obj
deeper until the stack limit is hit. Each frame executes PyDict_SetItem(s->markers, ident, obj) at L1444 before _Py_EnterRecursiveCall returns non-zero. Since cleanup never happens per-frame, the markers dict holds a strong reference to every object allocated across all ~159000 frames simultaneously, for the entire duration of the exception propagation back up to iterencode().

[okiemute04]

@raminfp @serhiy-storchaka Thank you for the detailed analysis! I see the distinction - PATH B is the real problem because the objects accumulate during recursion before the exception propagates. will update the pr for a fix

[okiemute04]

@raminfp @serhiy-storchaka I've updated the PR based on your feedback. Now only PATH B (RecursionError) includes the marker cleanup.