gh-144475: Fix a heap buffer overflow in partial_repr

[bkap123]

• Issue: heap-buffer-overflow in functools.partial.__repr__() #144475

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

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]

Please:

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

[picnixz]

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]

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.

[dr-carlos]

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]

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.

[caje731]

I think this return made sense when the label was 'done'.
Do we still want a side-effect of "return" for a label that "frees arguments"?
Or maybe we keep the "done" label?

[bkap123]

I am not exactly sure. I could see arguments for and against adding a done label before the return.

Personally, I don't think it is necessary, as it would never be used. Since the function returns NULL in the case of an error before a new reference is called to the arguments, adding a done label could be a bit misleading. It might suggest that the done label is called if the function needs to return before the fn, args, and kw point to the arguments, as that is the pattern for the other labels.

I would like to get other people's thoughts on this.

[bkap123]

Also, since each of the three labels will only be jumped to on an error, a better solution might be to rename each of the labels to the specific error that occurs rather than what gets freed. That way, it makes more sense why the function returns.