Skip to content

copt: reference leaks in getObjectSpecification on error and fallthrough paths #360

@jensens

Description

@jensens

A detailed analysis of _zope_interface_coptimizations.c was published at
https://gist.github.com/devdanzin/5afbad864934b165a2cc080f110aa720 by @devdanzin.
We reviewed the report with agent support and verified the findings against the
current source (zope.interface 8.2). This issue is one of several extracted from
that review.

Problem

getObjectSpecification leaks the result reference from
PyObject_GetAttr(ob, "__provides__") on two paths:

  1. Line 2507: When PyObject_IsInstance returns -1 (error), the function
    returns NULL without decrementing result.
  2. Line 2512: When __provides__ exists but is not a SpecificationBase
    (is_instance == 0), result is not decremented before falling through to
    the cls lookup path where it gets overwritten.

These leaks were introduced during the PyObject_TypeCheck to
PyObject_IsInstance migration (commit 84872a8).

Fix

if (is_instance < 0) {
    Py_DECREF(result);
    return NULL;
}
if (is_instance) {
    return result;
}
Py_DECREF(result);
// fall through to cls lookup

Scope

2 lines added. Error and fallthrough paths only.

Tests

Hard to test directly from Python. Reference leaks are not observable as
wrong behavior — they only waste memory. Two approaches:

  1. Indirect: The existing test_existing_provides_is_not_spec in
    Test_getObjectSpecificationFallback already exercises the is_instance == 0
    path (sets __provides__ to a non-spec object). It verifies correct return
    value but cannot detect the leak. After the fix, this test still passes —
    it serves as a regression guard for the control flow.

  2. With sys.getrefcount (optional, fragile):

def test_getObjectSpecification_no_leak_on_non_spec_provides(self):
    import sys
    marker = object()

    class Foo:
        __provides__ = marker

    before = sys.getrefcount(marker)
    self._callFUT(Foo())
    after = sys.getrefcount(marker)
    self.assertEqual(before, after)

This works because marker should be borrowed and returned — if the leak
exists, after > before. Fragile across implementations but valid for
the C extension.

Recommendation: include the sys.getrefcount test. It directly proves the fix.

Performance impact

Zero on the happy path. Py_DECREF is only added to paths that currently leak
(error return and non-SpecificationBase fallthrough). The normal case
(is_instance == 1, returns result) is unchanged.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions