GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-144475: Fix a heap buffer overflow in partial_repr by brijkapadia · Pull Request #145362 · python/cpython

Conversation

@brijkapadia

@brijkapadia brijkapadia commented Feb 28, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This is a cleaner version of PR #144571. I am not exactly sure what happened in #144571 so I would appreciate if anyone could tell me so I don't make the same mistake again.

Here are the changes I made:

  • I added an args and kw local pointer so that both live long enough during the call to repr to prevent a segfault
  • I added an fn local pointer so that repr uses its original state when generating its final representation.
  • I got rid of the error goto and merged it with the done goto as I needed to decrement the reference count of fn, args, and kw, and I found that decrementing them in the done goto was the easiest.
  • I added a test based on @Qanux's original code. I extended it to also check for changes in the fn and kw arguments.
  • Issue: heap-buffer-overflow in functools.partial.__repr__() #144475

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@brijkapadia
pythongh-144475: Fix a heap buffer overflow in partial_repr (v2)
7a5456c
@brijkapadia
brijkapadia requested a review from rhettinger as a code owner February 28, 2026 16:02
aisk
aisk approved these changes Feb 28, 2026
View reviewed changes
Comment thread Lib/test/test_functools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_functools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/C_API/2026-02-07-16-37-42.gh-issue-144475.8tFEXw.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_functoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting core review label Feb 28, 2026
aisk
aisk reviewed Feb 28, 2026
View reviewed changes

@aisk aisk left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, the approval is by accidant. I don't mean to approve this, just have comments this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brijkapadia reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot mentioned this pull request Feb 28, 2026
heap-buffer-overflow in functools.partial.__repr__() #144475
Closed
@aisk
aisk self-requested a review February 28, 2026 17:23
@brijkapadia
brijkapadia force-pushed the partial-repr branch from b498bbe to 7b4955f Compare February 28, 2026 17:42
brijkapadia and others added 2 commits February 28, 2026 12:44
@brijkapadia
small changes
7b4955f
@brijkapadia
Update test_functools.py
543d44c
@StanFromIreland StanFromIreland changed the title gh-144475: Fix a heap buffer overflow in partial_repr (v2) gh-144475: Fix a heap buffer overflow in partial_repr Feb 28, 2026
encukou
encukou reviewed Mar 2, 2026
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am not exactly sure what happened in #144571 so I would appreciate if anyone could tell me so I don't make the same mistake again.

Looks like a bad git rebase.
You don't need rebase in CPython, since the PRs are squashed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/_functoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_functoolsmodule.c
for (i = 0; PyDict_Next(pto->kw, &i, &key, &value);) {
for (i = 0; PyDict_Next(kw, &i, &key, &value);) {
/* Prevent key.__str__ from deleting the value. */
Py_INCREF(value);

@encukou encukou Mar 2, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If we're covering all the bases:

Here, key can be an arbitrary object as well.

Also, the iteration should have a critical section around it -- see PyDict_Next docs.

But perhaps the best way to solve that would be switching to always use frozendict with string keys, so let's leave this to a future PR?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brijkapadia brijkapadia Mar 2, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also, the iteration should have a critical section around it -- see PyDict_Next docs.

I think that adding a critical section here is best for a future PR. This PR is more about fixing a mutation during repr. In contrast, adding a critical section is intended for the free-threaded build. Additionally, I realized that there are quite a few other times in this file where PyDict_Next is called without a critical section, so a new PR is needed anyway. I can create an issue for this change, although, should we wait on a PR until this one is merged to avoid any conflicts? I'm fine adding a critical section now, though, if you think that is better.

But perhaps the best way to solve that would be switching to always use frozendict with string keys, so let's leave this to a future PR?

I agree, enforcing that kw has only string keys seems like the best solution. Just to make sure I understand correctly, when you mean switch to using a frozendict, are you saying that the kw entry in the partialobject struct should be a frozendict instead of a dict, or are you referring to something only in this function (partial_repr)? If the former, I would like to work on making that change in a new PR. I’m fairly new to CPython, so I’d appreciate any suggestions you have for that change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@encukou encukou Mar 3, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think that adding a critical section here is best for a future PR.

OK!

are you saying that the kw entry in the partialobject struct should be a frozendict instead of a dict, or are you referring to something only in this function (partial_repr)?

The former seems worth looking into. As you said, there's a lot of PyDict_Next without critical sections (and other possibilities for possible mutation than threads); frozendict could solve these neatly.

I can create an issue for this change, although, should we wait on a PR until this one is merged to avoid any conflicts?

Yes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brijkapadia reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aisk aisk mentioned this pull request Mar 2, 2026
gh-144475: Fix use-after-free in functools.partial.__repr__() #145395
Closed
@brijkapadia
move declaration
74b98ca
@brijkapadia brijkapadia mentioned this pull request Mar 3, 2026
Missing critical sections for PyDict_Next calls in _functoolsmodule.c #145446
Closed
@encukou
Reword NEWS & move to Library
ef0ffb3
encukou
encukou approved these changes Mar 3, 2026
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Mar 3, 2026
@encukou
encukou enabled auto-merge (squash) March 3, 2026 13:20
@encukou encukou added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes labels Mar 3, 2026
@encukou
encukou merged commit 671a953 into python:main Mar 3, 2026
55 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 3, 2026
@miss-islington-app

miss-islington-app Bot commented Mar 3, 2026

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 3, 2026
@brijkapadia @miss-islington
pythongh-144475: Fix reference management in partial_repr (pythonGH-1…
cac1a91
…45362)

(cherry picked from commit 671a953)

Co-authored-by: bkap123 <[email protected]>
@miss-islington-app

miss-islington-app Bot commented Mar 3, 2026

Copy link
Copy Markdown

Sorry, @bkap123 and @encukou, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 671a953dd65292a5b69ba7393666ddcac93dbc44 3.13
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned encukou Mar 3, 2026
@bedevere-app

bedevere-app Bot commented Mar 3, 2026

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Mar 3, 2026
@brijkapadia
brijkapadia deleted the partial-repr branch March 3, 2026 13:56
@brijkapadia brijkapadia mentioned this pull request Mar 3, 2026
Implement frozendict for partialobject in functools module #145478
Closed
encukou pushed a commit that referenced this pull request Mar 3, 2026
@miss-islington @brijkapadia
[3.14] gh-144475: Fix reference management in partial_repr (GH-145362) (
3c99c16
GH-145470)

(cherry picked from commit 671a953)

Co-authored-by: bkap123 <[email protected]>
brijkapadia added a commit to brijkapadia/cpython that referenced this pull request Mar 12, 2026
@brijkapadia
[3.13] pythongh-144475: Fix reference management in partial_repr (pyt…
bcce928
…honGH-145362)

(cherry picked from commit 671a953)

Co-authored-by: bkap123 <[email protected]>
@bedevere-app

bedevere-app Bot commented Mar 12, 2026

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Mar 12, 2026
Yhg1s pushed a commit that referenced this pull request Mar 24, 2026
@brijkapadia
[3.13] gh-144475: Fix reference management in partial_repr (GH-145362) (
4e37207
#145882)

(cherry picked from commit 671a953)
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
@brijkapadia @ljfp
pythongh-144475: Fix reference management in partial_repr (pythonGH-1…
30fb67d
…45362)
@brijkapadia brijkapadia mentioned this pull request Jul 22, 2026
gh-154189: Fix use-after-free in functools.partial_vectorcall #154508
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
@aisk aisk Awaiting requested review from aisk

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@encukou encukou

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@brijkapadia @encukou @aisk
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.