gh-144475: Fix a heap buffer overflow in partial_repr#145362
gh-144475: Fix a heap buffer overflow in partial_repr#145362bkap123 wants to merge 4 commits intopython:mainfrom
Conversation
Misc/NEWS.d/next/C_API/2026-02-07-16-37-42.gh-issue-144475.8tFEXw.rst
Outdated
Show resolved
Hide resolved
aisk
left a comment
aisk
left a comment
There was a problem hiding this comment.
Sorry, the approval is by accidant. I don't mean to approve this, just have comments this.
bkap123
force-pushed
the
partial-repr
branch
from
b498bbe to
7b4955f
Compare
StanFromIreland
changed the title
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
3 participants
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:
argsandkwlocal pointer so that both live long enough during the call toreprto prevent a segfaultfnlocal pointer so thatrepruses its original state when generating its final representation.errorgoto and merged it with thedonegoto as I needed to decrement the reference count offn,args, andkw, and I found that decrementing them in thedonegoto was the easiest.fnandkwarguments.