Skip to content

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

@tekknolagi tekknolagi commented Jan 28, 2022

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.
@markshannon

Copy link
Copy Markdown
Member

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

@tekknolagi

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/cinder 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
@tekknolagi

Copy link
Copy Markdown
Contributor Author

Lol, did this finally bite someone else?

@serhiy-storchaka

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

python-cla-bot Bot commented Apr 6, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@github-actions

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown
Member

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

@serhiy-storchaka

Copy link
Copy Markdown
Member

Well, it was fixed by GH-118454.

@tekknolagi

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.

@tekknolagi

Copy link
Copy Markdown
Contributor Author

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

@serhiy-storchaka

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.

@serhiy-storchaka

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.

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