I found out that this won't be sufficient so converting it to a draft for now until I patch more attackable entrypoints.

How about rejecting excessively largeco_stacksize when creating code objects?
It will be a much simpler fix.

How about rejecting excessively large co_stacksize when creating code objects?

That's what I'm actually doing (or am I not?) [It's just that the expression to check is not the best one and I'm trying to see if I can make other place of the code overflow with:

if ((size_t)con->stacksize        >= (INT_MAX /sizeof(PyObject*))-FRAME_SPECIALS_SIZE-nlocalsplus)    {PyErr_SetString(PyExc_OverflowError,"code: co_stacksize is too large");return-1;    }

Ok, I found different issues. It's possible to bypass the check in the code object validator that I added when attacking generator's frames. I'll need a bit more time for that but I won't be coding anymore today.

The evil size that should be checked in code constructor is "((_testcapi.INT_MAX - 10) // sizeof(PyObject *))" but the following crashes:

ps=ctypes.sizeof(ctypes.c_void_p)# sizeof(PyObject *)smallest_evil_co_stacksize= (_testcapi.INT_MAX//ps)-10evil=f().gi_frame.f_code.__replace__(co_stacksize=smallest_evil_co_stacksize-1)# okevil_gi=types.FunctionType(evil, {})()# crashesself.assertRaises(OverflowError,evil_gi.__sizeof__)# crashes

Withsmallest_evil_co_stacksize,__replace__ would raise as expected but withsmallest_evil_co_stacksize - 1 it's FunctionType that is crashing.

I meant choose a large value, rather than checking for overflow.
That way we won't need to check for overflow anywhere else.

Something likeco_nlocals + co_stacksize < INT_MAX/2/SIZEOF_VOID_Pco_nlocals + co_stacksize < INT_MAX/8

That way we won't need to check for overflow anywhere else.

Do you have a good value in mind? I'm not sure if it could affect recursion limit for instance or tracebacks if you change the stacksize (clearly not an expert here). I'm going offline now but we can continue this discussion tomorrow.

Also, maybe I could find a way to pass the check in the code object but make it crash when creating fake generators. So the choice ofco_nlocals + co_stacksize < INT_MAX / 8 may not be sufficient I think (or am I messing up my arithmetic?). I'm a bit tired so I'll check tomorrow.

EDIT (30th October): I'm not sure I'll be able to work on that today (I have some IRL stuff to do and won't be available today a lot).

The free-threaded build crashes (just with "Segmentation fault (core dumped)", no traceback) but I don't know why...@colesbury do you have some idea? The bad code is

importtypesdeff():yieldcode=f().gi_frame.f_codeevil_code=code.__replace__(co_stacksize=268435444)# OKevil_gi_func=types.FunctionType(evil_code, {})# OKevil_gi=evil_gi_func()# not ok!

I'll take a look at the feee-threaded failures.

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.

I've updated the comment and removed the un-necessary checks. Please tell me if the comment is still imprecise or if you have a preferred formulation.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

Friendly ping@markshannon

Friendly ping@markshannon (I don't really want to merge this without your explicit approval)

Hmm, I still think Mark should be the one that approves this PR before it's merged. I don't see obvious issues for the code change. However, is theINT_MAX / 16 a precise limit? Both our checks and tests are calculating very precisely for this number, the test even does +1/-1 level validation. Not saying it's bad, but is it necessary? We want to prevent an artificial code object that hasINT_MAX level locals?

Again, I'm not opposing the current code, that's just the first thought I saw it.

Both our checks and tests are calculating very precisely for this number, the test even does +1/-1 level validation

The +1/-1 validation is simply to check that we didn't change the formula. We can take any bound we want instead of just INT_MAX / 16, as soon as it's a valid one. But the bound is not tight. Mark said that we could have a tighter bound (so no, it's not a precise limit, it can be a bit bigger in general), namely

We could increase the limit to something like$INT_MAX/8 - K$ where$K$ is roughly the base frame size, but I don't see any advantage to doing so and it would be more fragile.

want to prevent an artificial code object that has INT_MAX level locals?

We just want to prevent a value ofco_stacksize being too large because we're doing some arithmetic with ints behind and use it in a loop.

cc@markshannon

When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

Sorry for the delay in the review, and thanks for doing this.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.