gh-126119: fix some crashes in code objects if co_stacksize is absurdly large

[picnixz]

• Issue: Overflow when computing __sizeof__ of objects with underlying code objects #126119

[picnixz]

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

[markshannon]

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

[picnixz]

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;
    }

[picnixz]

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) - 10
        evil = f().gi_frame.f_code.__replace__(co_stacksize=smallest_evil_co_stacksize - 1) # ok
        evil_gi = types.FunctionType(evil, {})()  # crashes
        self.assertRaises(OverflowError, evil_gi.__sizeof__)  # crashes

With smallest_evil_co_stacksize, __replace__ would raise as expected but with smallest_evil_co_stacksize - 1 it's FunctionType that is crashing.

[markshannon]

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

Something like co_nlocals + co_stacksize < INT_MAX/2/SIZEOF_VOID_P co_nlocals + co_stacksize < INT_MAX/8

[picnixz]

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 of co_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).

[picnixz]

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

import types
def f(): yield
code = f().gi_frame.f_code
evil_code = code.__replace__(co_stacksize=268435444)  # OK
evil_gi_func = types.FunctionType(evil_code, {})      # OK
evil_gi = evil_gi_func()  							  # not ok!	

[ZeroIntensity]

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

[bedevere-app]

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 phrase I 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.

And if you don't make the requested changes, you will be put in the comfy chair!

[picnixz]

Ok, with this new limit, I don't trigger the crash on free-threaded builds (it's just that the code (with another evil value) takes forever to complete but that's expected). I've removed the tests because they won't work anymore (it's not possible to bypass the check in code objects).

However, for safety, I kept the checks when computing __sizeof__ if Py_SSIZE_T_MAX < INT_MAX which may happen on some platforms (POSIX does not specify any restriction or relation between ssize_t and int ranges) and the upper limit may be too crude (it's not really possible to refine it when constructing code objects for frames).

I have made the requested changes; please review again.

[bedevere-app]

Thanks for making the requested changes!

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

[markshannon]

We should be able to remove the changes to frame_sizeof and gen_sizeof as _PyCode_Validate() protects from overflow.

[bedevere-app]

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 phrase I 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.

[picnixz]

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.

[bedevere-app]

Thanks for making the requested changes!

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

[picnixz]

Friendly ping @markshannon

[picnixz]

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

[gaogaotiantian]

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 the INT_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 has INT_MAX level locals?

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

[picnixz]

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 of co_stacksize being too large because we're doing some arithmetic with ints behind and use it in a loop.

cc @markshannon