Skip to content

PyList_SetItem error-path double-free in decoder EndObject/EndArray #231

Description

@K-ANOY

Three sites in the decoder decref an item again after PyList_SetItem() returns
-1. PyList_SetItem() steals the item reference even on failure (it calls
Py_XDECREF(item) internally before returning -1), so the extra Py_DECREF on
the error branch releases the same reference twice.

File: rapidjson.cpp

Functions: EndObject, EndArray

The three sites share one shape:

Py_ssize_t listLen = PyList_GET_SIZE(current.object);
rc = PyList_SetItem(current.object, listLen - 1, pair);

// NB: PyList_SetItem() steals a reference on the replacement, so it
// must not be DECREFed when the operation succeeds

if (rc == -1) {
    Py_DECREF(pair);          // double-free: PyList_SetItem already stole/released `pair`
    return false;
}
Function PyList_SetItem erroneous Py_DECREF item
EndObject (key/value-pairs branch) rapidjson.cpp:1055 rapidjson.cpp:1061 pair
EndObject (array branch) rapidjson.cpp:1080 rapidjson.cpp:1086 replacement
EndArray (array branch) rapidjson.cpp:1175 rapidjson.cpp:1181 replacement

The in-code comment is the tell: it says the item "must not be DECREFed when the
operation succeeds", implying the author believed a Py_DECREF is required on
failure. But PyList_SetItem() steals on failure too, so each item (refcount 1
at that point) is freed by the internal Py_XDECREF and then freed again by the
explicit Py_DECREF — a double-free / use-after-free.

The sibling dict branches at these sites use PyDict_SetItem() /
PyObject_SetItem(), which do not steal, and correctly Py_DECREF the item
afterward — confirming the list-branch handling is the odd one out.

Reachability

PyList_SetItem() returns -1 only when the first argument is not a list or the
index is out of range. In these branches current.object is always a list, and a
placeholder is normally appended (rapidjson.cpp:896, :912) before the matching
End* handler runs, so listLen - 1 is normally in range. The branch becomes
reachable if that placeholder is missing — e.g. an allocation-failure path where
PyList_GET_SIZE is 0, making listLen - 1 == -1 (out of range). See the
companion report on the unchecked PyList_Append() in Handle(), which can break
that invariant. This is an error-path defect, not something ordinary JSON input
triggers, but the handling is wrong and should be fixed independently.

Suggested fix

Drop the Py_DECREF on the error branch at all three sites:

rc = PyList_SetItem(current.object, listLen - 1, pair);
if (rc == -1) {
    return false;        /* PyList_SetItem already stole/released `pair` */
}

If a defensive check against an empty list is wanted, guard the index before
calling PyList_SetItem rather than decrefing afterward.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions