`collections.defaultdict`'s `__repr__` can lead to infinite recursion

[KowalskiThomas]

I have spotted what I believe is a bug in the implementation of defaultdict's __repr__ method (code).

Long story short, the current implementation does not properly reset the recursion guard after delegating to the factory's __repr__ (when the factory's __repr__ calls back into defaultdict's __repr__). As far as I can tell, this comes from Py_ReprLeave not being called under the right conditions.

I've made this reproducer, I think it's as simple as it can get:

from collections import defaultdict

class ProblematicFactory:
    def __call__(self) -> dict[str, str]:
        return {}

    def __repr__(self):
        print(f"dd: {repr(dd)}")

        return f"ProblematicFactory for {repr(dd)}"

dd = defaultdict[str, dict[str, str]](ProblematicFactory())
print(repr(dd.default_factory))

Running this gives...

% python3 repro.py
dd: defaultdict(..., {})
... LOTS OF THIS
dd: defaultdict(..., {})
Traceback (most recent call last):
  File "/Users/thomas.kowalski/Documents/cpython/repro.py", line 13, in <module>
    print(repr(dd.default_factory))
          ~~~~^^^^^^^^^^^^^^^^^^^^
  File "/Users/thomas.kowalski/Documents/cpython/repro.py", line 8, in __repr__
    print(f"dd: {repr(dd)}")
                 ~~~~^^^^
  File "/Users/thomas.kowalski/Documents/cpython/repro.py", line 10, in __repr__
    return f"ProblematicFactory for {repr(dd)}"
                                     ~~~~^^^^
  File "/Users/thomas.kowalski/Documents/cpython/repro.py", line 10, in __repr__
    return f"ProblematicFactory for {repr(dd)}"
                                     ~~~~^^^^
  File "/Users/thomas.kowalski/Documents/cpython/repro.py", line 10, in __repr__
    return f"ProblematicFactory for {repr(dd)}"
                                     ~~~~^^^^
  [Previous line repeated 995 more times]

Going back to the current code:

int status = Py_ReprEnter(dd->default_factory);
if (status != 0) { // if we already are in a __repr__ call for default factory
    if (status < 0) {
        Py_DECREF(baserepr);
        return NULL; // we early-return without "leaving" which is correct
    }
    defrepr = PyUnicode_FromString("...");
}
else // we are NOT in a __repr__ call for default factory
    defrepr = PyObject_Repr(dd->default_factory); // no early return, we fall through

// we may or may not be in a __repr__ call for default factory
// resetting here is not OK, it's not our responsibility
Py_ReprLeave(dd->default_factory);

As far as I can tell, the fix would be to move the final Py_ReprLeave to the else body so that if we already are in the factory's __repr__ we don't reset it ourselves, we let whoever entered it reset it.

Note I have a branch ready with that fix and can open it here if/once the bug is confirmed.

Linked PRs

• gh-145492: Fix defaultdict __repr__ infinite recursion #145659
• [3.13] gh-145492: Fix defaultdict __repr__ infinite recursion (GH-145659) #145746
• [3.14] gh-145492: Fix defaultdict __repr__ infinite recursion (GH-145659) #145747
• gh-145492: Fix test_repr_recursive_factory reproducer #145761
• gh-145492: fix regression test for defaultdict factory repr #145788

[mvanhorn]

I've submitted a fix in #145659.

The root cause is that Py_ReprLeave(dd->default_factory) was called unconditionally in defdict_repr, even when Py_ReprEnter returned > 0 (recursion detected). When Py_ReprEnter detects recursion, it does not add a new entry to the tracking list - it just reports that the object is already being repr'd. Calling Py_ReprLeave in that case removes the entry from the outer call, so subsequent recursive calls bypass the guard entirely.

The fix moves Py_ReprLeave inside the else branch so it's only called when Py_ReprEnter returned 0 (successfully entered).

[KowalskiThomas]

@skirpichev I see you've approved the linked PR. I have a question -- on another issue I created (#145458), I was told the following:

When you are not sure about something, please avoid create draft PRs and wait for a maintainer to acknowledge the issue. You can say that you have a PR ready in case this is acknowledge but please avoid opening PRs that are not sure to be merged.

In the current case, that's what I did -- I specified that I had a fix for the problem (it even was in the description...) and I was just waiting for the go-ahead before actually opening the PR. Was that the right thing to do? Or should I have created the PR right away?

To be clear, my intention is not to "fight" over who gets to make the PR, but I'm looking to start contributing to CPython and it's unclear to me what the guidelines are in this case. I was happy to have discovered something I could fix and I'm somewhat disappointed not to get it to the finish line myself (especially since both the PR and the associated comment here look completely AI generated 😕).

[skirpichev]

@KowalskiThomas, sorry. I overlooked your "note" sentence.

I don't think it does make sense to open a new pr, but I may suggest author to add acknowledgment for you in news entry, if you wish. BTW, don't worry - my approval barely means nothing: pr should be approved by core devs.

I specified that I had a fix for the problem (it even was in the description...) and I was just waiting for the go-ahead before actually opening the PR. Was that the right thing to do?

Here a quick guide: https://devguide.python.org/getting-started/pull-request-lifecycle/#lifecycle-of-a-pull-request

Maybe rules aren't clear, but I would say - there is nothing wrong if you open a pr request for a triaged issue (added labels and so on). Especially for bugfixes. I case of doubts - open a draft pr.

[KowalskiThomas]

I may suggest author to add acknowledgment for you in news entry, if you wish.

Sure, I'd appreciate this. It's probably not worth opening a separate PR as you said, I mostly just want to avoid this happening again in the future 😅

there is nothing wrong if you open a pr request for a triaged issue

Sounds good, thank you. It wasn't clear to me what was needed for an issue to be considered "confirmed"/triaged (in this case I was waiting for a comment from a maintainer or something acknowledging it), your explanation helps a lot.

[mvanhorn]

Hey @KowalskiThomas - apologies for jumping in without noticing you had a fix ready. I've updated the NEWS entry to credit your analysis. Thanks for the thorough bug report.