A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@1st1, would it make sense to

• move the test to a separate file (test_asyncio/test_regressions.py perhaps)?
• add a comment that the test uses internals, and can be removed if the internals change?

@encukou@1st1 I agree that these tests are overly complicated and would be better in its own test file. I also think that the tests would benefit from better naming (ridiculous setup isn't very descriptive for future maintainers). I also think a one or two line docstring of what the test is testing would be helpful too.

I also think that the tests would benefit from better naming (ridiculous setup isn't very descriptive for future maintainers).

I can find a better name for that :)

I also think a one or two line docstring of what the test is testing would be helpful too.

I assumed that referencing the issue was sufficient but I can add some comments!

I agree that these tests are overly complicated and would be better in its own test file.

Would it be possible to delay that one until we fix all UAFs or make it the priority for now (either way is fine for me).@Nico-Posada has one PR that fixes another UAF and may be working on a second one for an even more contrived crash. If you want it to be now, I can create a PR just to isolate the existing tests (and to moveReachableCode exception to the test.support file because it could be a good place for future usage?).

@encukou@willingc

move the test to a separate file (test_asyncio/test_regressions.py perhaps)?
add a comment that the test uses internals, and can be removed if the internals change?

I'm really not sure this all is worth the hassle. It's only a matter of time until someone stumbles upon this test and will not understand why it's there :)

This PR tests something very abstract and not occurring in real life scenarios, so I personally think it's OK to commit the fix for the sake of having cleaner code, but on the other hand there's no need to overthink it.

I've removed the contrived test as well as the small refactorization I needed. It's much cleaner and for posterity, people can just look at02415df to get a test in the future.

I'm curious if it's viable to do a refcount-based test to make sure that the refcount of our evil object is<num here> in__getattribute__. Can't test for UAF directly but it'll test to make sure the refcount is being increased properly which means the UAF is being prevented. The test case for this should be much shorter and easier to understand/implement.

I'm curious if it's viable to do a refcount-based test to make sure that the refcount of our evil object is ingetattribute

Could be possible and we could transform previous tests like that but it might still require some globals to know when we need to trigger the check and I suspect it'll be ugly to read =/ (haven't tried though)

Here's something basic I was able to put together. On a build where this bug isn't patched, it saysrefcount 6, but on this version it saysrefcount 7. So in the test we would do something to assert the refcount at that spot >= 7`

Obviously there's a lot to clean up in this, but it's MUCH smaller now and we can probably add comments explaining why it's testing for refcounts in that spot.

importasyncioimporttypesimportsysfromcontextvarsimportContextasyncdefnone_coroutine():@types.coroutinedefgen():whileTrue:yieldNoneawaitgen()classTestDoneException(BaseException):passclassTestLoop:get_debug=staticmethod(lambda:False)is_running=staticmethod(lambda:True)def__getattribute__(self,name):ifname=="call_soon":print("refcount",sys.getrefcount(my_context))raiseTestDoneException()returnobject.__getattribute__(self,name)coro=none_coroutine()my_context=Context()try:task=asyncio.Task(coro,loop=TestLoop(),context=my_context,eager_start=True)exceptException:print("done")

@1st1 Are you willing to consider this kind of test or do you prefer no test at all? if you want, we can try to remove previous tests that were also using intricate constructions just to trigger the UAF and make them adopt that kind of testing?

Refcount values can change when code is refactored, and if the refactoradds a refcount, a>= 7 assertion would pass even if this regresses. An ineffective test is worse than a failing one, IMO.

Oh right. Then I think no test is fine in this case.

Thanks@picnixz for the PR, and@1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

GH-126250 is a backport of this pull request to the3.13 branch.

GH-126251 is a backport of this pull request to the3.12 branch.