Skip to content

gh-144475: Fix a heap buffer overflow in partial_repr#144571

Open
bkap123 wants to merge 22 commits intopython:mainfrom
bkap123:fix-functools_partial_repr_bug
Open

gh-144475: Fix a heap buffer overflow in partial_repr#144571
bkap123 wants to merge 22 commits intopython:mainfrom
bkap123:fix-functools_partial_repr_bug

Conversation

@bkap123
Copy link
Contributor

@bkap123 bkap123 commented Feb 7, 2026

@bkap123 bkap123 requested a review from rhettinger as a code owner February 7, 2026 16:19
@python-cla-bot
Copy link

python-cla-bot bot commented Feb 7, 2026

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Feb 7, 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.

@picnixz
Copy link
Member

picnixz commented Feb 7, 2026

Please:

  • add regression tests and check that the test failed before your change
  • remove blank lines in NEWS

@picnixz
Copy link
Member

picnixz commented Feb 7, 2026

By the way, @dr-carlos already suggested to open a PR. It is polite to ask them if they want to contribute themselves. As such, I'm going to close this one unless they are fine with you making the PR (we don't really want people "sniping" work of others)

@bkap123
Copy link
Contributor Author

bkap123 commented Feb 7, 2026

Thanks for the feedback. I missed that @dr-carlos suggested to fix it.

I’m happy to close this PR if @dr-carlos is already working on it.

Co-authored-by: Bénédikt Tran <[email protected]>
@dr-carlos
Copy link
Contributor

Thanks for the feedback. I missed that @dr-carlos suggested to fix it.

I’m happy to close this PR if @dr-carlos is already working on it.

Hi, thanks for asking!
I'm happy for you to continue with the PR :)

@bkap123
Copy link
Contributor Author

bkap123 commented Feb 8, 2026

Here are the changes I made:

  • I added a kw local pointer, as a similar segfault happens for keywords
  • 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.
    Update: I changed the goto logic to reduce repetitive calls to Py_DECREF
  • I added a test based on @Qanux's original code in issue heap-buffer-overflow in functools.partial.__repr__() #144475. I extended it to also check for changes in the fn and kw arguments.

@bkap123 bkap123 force-pushed the fix-functools_partial_repr_bug branch from e5c7b4e to a196de4 Compare February 9, 2026 21:39
@serhiy-storchaka serhiy-storchaka self-requested a review February 11, 2026 09:43
if (mod == NULL) {
goto error;
}
if (mod == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

PEP 7 requires braces even for single-statement branches in new code. So, please restore the removed braces.

def test_repr_saftey_against_reentrant_mutation(self):
g_partial = None

class Function:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the Function class and cannot use simple function like print?

return f"EvilObject({self.name})"

trigger = EvilObject("trigger", is_trigger=True)
victim = EvilObject("victim")
Copy link
Member

Choose a reason for hiding this comment

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

Why victim is an EvilObject? Why not use a list?

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.

5 participants