Skip to content

gh-145492: Fix defaultdict __repr__ infinite recursion#145659

Merged
encukou merged 5 commits intopython:mainfrom
mvanhorn:osc/145492-fix-defaultdict-repr-recursion
Mar 10, 2026
Merged

gh-145492: Fix defaultdict __repr__ infinite recursion#145659
encukou merged 5 commits intopython:mainfrom
mvanhorn:osc/145492-fix-defaultdict-repr-recursion

Conversation

@mvanhorn
Copy link
Contributor

@mvanhorn mvanhorn commented Mar 9, 2026

Fixes #145492.

Summary

Fixed the recursion guard in defdict_repr so that Py_ReprLeave is only called when Py_ReprEnter successfully entered (returned 0). Previously, Py_ReprLeave was called unconditionally, including when Py_ReprEnter returned > 0 (recursion detected). Since Py_ReprEnter does not add a new entry when it detects recursion, calling Py_ReprLeave in that case incorrectly removed the entry from the outer (non-recursive) call, allowing subsequent recursive calls to bypass the guard and cause infinite recursion.

The fix is a 3-line change: move Py_ReprLeave(dd->default_factory) inside the else branch of the recursion check.

Test plan

  • Added test_repr_recursive_factory regression test that reproduces the infinite recursion scenario from the issue
  • All existing test_defaultdict tests continue to pass (13/13)

This contribution was developed with AI assistance (Claude Code).

Move Py_ReprLeave(dd->default_factory) inside the else branch so it is
only called when Py_ReprEnter returned 0 (successfully entered). When
Py_ReprEnter detects recursion (returns > 0), it does not add a new
entry to the repr tracking list. Calling Py_ReprLeave in that case
incorrectly removed the entry from the outer (non-recursive) call,
which allowed subsequent recursive calls to bypass the guard entirely,
leading to infinite recursion.

Includes a regression test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mvanhorn mvanhorn requested a review from rhettinger as a code owner March 9, 2026 03:47
@bedevere-app
Copy link

bedevere-app bot commented Mar 9, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@@ -0,0 +1,3 @@
Fix infinite recursion in :class:`collections.defaultdict` ``__repr__``
when a ``defaultdict`` contains itself. Based on analysis by KowalskiThomas
in :issue:`145492`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in :issue:`145492`.
in :gh:`145492`.

:issue: is for b.p.o issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed - changed to :gh:\145492``. Thanks for catching that!

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there duplicate blurbs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the duplicate - it was added by blurb_it after I'd already created one manually. Fixed in latest push.

Address review feedback from StanFromIreland:
- Changed :issue:`145492` to :gh:`145492` (correct syntax for GitHub issues)
- Removed duplicate NEWS blurb added by blurb_it

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@encukou encukou merged commit 2d35f9b into python:main Mar 10, 2026
50 checks passed
@encukou encukou added awaiting core review needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Mar 10, 2026
@miss-islington-app
Copy link

Thanks @mvanhorn for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @mvanhorn for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 10, 2026
…H-145659)

(cherry picked from commit 2d35f9b)

Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
Co-Authored-By: Thomas Kowalski <thom.kowa@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 10, 2026
…H-145659)

(cherry picked from commit 2d35f9b)

Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>
Co-Authored-By: Thomas Kowalski <thom.kowa@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Mar 10, 2026

GH-145746 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Mar 10, 2026
@bedevere-app
Copy link

bedevere-app bot commented Mar 10, 2026

GH-145747 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Mar 10, 2026
return {}
def __repr__(self):
repr(dd)
return "ProblematicFactory()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I am not too late, but this test never triggers an infinite recursion in python 3.14 without the fix.
I suggest replacing this line with return f"ProblematicFactory for {repr(dd)}" as reported in the @KowalskiThomas issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Thanks for the catch! I though I tested, but apparently I used the wrong code :/

@mvanhorn, why did you change this part of the reproducer?

@KowalskiThomas, now I regret not asking you to send your branch as a PR. Sorry! :(
If you want to get it over the finish line (alas, without the start), now's your chance. Or I can send a fix-up PR myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch by @YvesDup - I didn't realize I was weakening the reproducer. I've submitted a fix-up PR at #145761 that uses return f"ProblematicFactory for {repr(dd)}" to match the original reproducer. Happy to close it if @KowalskiThomas wants to pick it up instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, go ahead @KowalskiThomas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

collections.defaultdict's __repr__ can lead to infinite recursion

6 participants