gh-144475: Fix a heap buffer overflow in partial_repr

[bkap123]

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

[aisk]

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

[encukou]

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.

[encukou]

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?

[bkap123]

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.

The one change we could make now is to replace int i with Py_ssize_t pos as this would make this loop consistent with the docs and other calls to PyDict_Next.

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.

[encukou]

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.

[miss-islington-app]

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

[miss-islington-app]

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

[bedevere-app]

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