gh-143635: Fix crash in _Py_typing_type_repr

[sobolevn]

I also added one more test case for a second possible crash in the same function.

• Issue: Use-after-free in _Py_typing_type_repr via re-entrant __origin__ lookup during GenericAlias repr #143635

[JelleZijlstra]

This sort of feels like the wrong place, shouldn't callers of the function ensure they have a reference to p that remains valid?

[picnixz]

I think so and we should just say that the function takes a strong reference (the function is not documented but it would be good to indicate this).

That means changing some PyList_GET_ITEM/PyTuple_GET_ITEM and adding some Py_INCREF for non-sequences.

[sobolevn]

Hm, I am not quite sure about it. Technically, we don't need a strong reference in regular cases, it is only needed for strange self-destructing ones.

We have multiple places where we just use Py_INCREF / Py_DECREF on a object before calling some python callbacks with it. Changing this function to require a strong refenrence will require quite a lot of refactoring for a very niche thing (and might be a breaking change for anyone using this function, even though it is not documented).

Let's say we want to refactor this place:

cpython/Objects/genericaliasobject.c

Lines 103 to 105 in e22b685

	if (_Py_typing_type_repr(writer, alias->origin) < 0) {
	goto error;
	}

It would be:

Py_INCREF(alias->origin);
if (_Py_typing_type_repr(writer, alias->origin) < 0) {
     Py_DECREF(alias->origin);
     goto error;
}
Py_DECREF(alias->origin);

This does not seem like a good API to me :/

What are the potential limitations of the current approach?

[picnixz]

Changing this function to require a strong refenrence will require quite a lot of refactoring for a very niche thing (and might be a breaking change for anyone using this function, even though it is not documented).

It's not exported as part of the public API so I'm not sure it's available by "others" (it's only an extern symbol in the internal API). Since it's internal we can also keep your change as it reduces the diff (I didn't think there would be that many places though)

[sergey-miryanov]

Maybe it is worth to open a separate PR for adding a strong ref?

[picnixz]

No I think it should be decided here.

[picnixz]

No I think it should be decided here.

[JelleZijlstra]

This needs NEWS since it fixes a user-visible crash.

[JelleZijlstra]

Actually this is incorrect. The crash can be triggered if the list is cleared while we're iterating over it in ga_repr_items_list. INCREFing here is insufficient to protect us because the item may not be valid in the first place any more by the time we receive it.

My repro case in #143823 makes this clearer.

[bedevere-app]

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