FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-46561: Ensure operands to __get__ survive the call by tekknolagi · Pull Request #30979 · python/cpython · GitHub

/ cpython Public

bpo-46561: Ensure operands to __get__ survive the call - #30979

Closed
tekknolagi wants to merge 3 commits into
python:mainfrom
tekknolagi:mb-fix-__get__
Closed

bpo-46561: Ensure operands to __get__ survive the call#30979
tekknolagi wants to merge 3 commits into
python:mainfrom
tekknolagi:mb-fix-__get__

Conversation

tekknolagi commented Jan 28, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Callees can assume their parameters survive for the entire call. This
violates that assumption and can cause a use-after-free.

https://bugs.python.org/issue46561

Callees can assume their parameters survive for the entire call. This
violates that assumption and can cause a use-after-free.

This is not an issue in CPython right now because later on in the
interpreter __get__ fastcall path, the whole vector of arguments get
INCREFed. However, if a program provides a different entrypoint for a
vectorcall, it may crash.

Copy link
Copy Markdown
Member

The changes look good, could you add some test cases?

Copy link
Copy Markdown
Contributor Author

I am working on making a C-API equivalent for your sample Python test code. Unfortunately, it is not so easy as making a C extension class with Py_tp_descr_get because that path appears to do the right thing. So instead I have to make a class which has something (a function or C extension callable) with a tp_vectorcall for __get__.

facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Mar 1, 2022
Summary:
Callees can assume their parameters survive for the entire
call. This violates that assumption and can cause a use-after-free.
Similar to D27254519. See python/cpython#30979.

Reviewed By: swtaarrs

Differential Revision: D33699901

fbshipit-source-id: 677d97d

Copy link
Copy Markdown
Contributor Author

Lol, did this finally bite someone else?

Copy link
Copy Markdown
Member

I am on a mission to review old PRs that were not reviewed by anybody.

LGTM, but please fix the NEWS entry (and its text is not very clear, it could be improved). It would be nice to add tests, but if it is too complicated, it is not necessary.

python-cla-bot Bot commented Apr 6, 2025
edited
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 10, 2026

Copy link
Copy Markdown
Member

@tekknolagi, please sign the CLA. This is needed to merge this PR.

Copy link
Copy Markdown
Member

Well, it was fixed by GH-118454.

Copy link
Copy Markdown
Contributor Author

I feel like I've signed the CLA like 3 times. I'll sign it again if needed. But also, I'm not sure if Dino's patch does address it? It looks like a SETREF after the call.

Copy link
Copy Markdown
Contributor Author

Oh. I had signed the old CLA. I signed the new one.

Copy link
Copy Markdown
Member

Thank you. That bug has been fixed in other PR after I approved this PR and before I have opportunity to return to it (and I didn't because you didn't update the entry in the NEWS section). But this will help with your other PRs.

Copy link
Copy Markdown
Member

I'm not sure if Dino's patch does address it? It looks like a SETREF after the call.

Before your patch, descr and res were borrowed references (returned by _PyType_Lookup()). You added Py_INCREF to make it a strong reference. Now we use _PyType_LookupRef() which returns a strong reference, so that Py_INCREF no longer needed. Py_SETREF is equivalent to Py_DECREF and assignment in right order. So these changes were equivalent.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL