BUG: avoid possible stack overflow in arraydescr_dealloc - #32133

Use Py_CLEAR instead.

I don't think that's right. I'm stealing the reference without deallocating immediately on purpose.

I could INCREF then Py_CLEAR to avoid that but that adds an unnecessary incref. Would you prefer that for clarity? I can also add a comment explaining that I'm setting the point to NULL and explicitly not deallocating yet.

The CLEAR macro does exactly that, though.

It would DECREF base, not lbase. I added a comment explaining the reference stealing so hopefully it's clearer.

Yeah, nvm. I thought the concern was Py_CLEAR not being careful enough, but this isn't a Py_CLEAR pattern at all.

Oh, I missed this as well. thanks for adding the comment

Not quite sure, but the Py_REFCNT doesn't smell strictly thread-safe to me (if true maybe you are OK with ignoring that, but considering that this is a niche failure)?

Just enabling GC tracking feels reasonable to me (we should do this more anyway, maybe even for arrays, I even did once but that PR just withered away), but I am not sure about implications... dtypes are practically immutable (unless metadata is an actual dict as that could contain mutable objects), though, you'd want to untrack them mostly.
(We can do that now, although I think the NumPy internal singletons may still be static structs.)

For the Py_REFCNT==1, I looked at using PyUnstable_Object_IsUniquelyReferenced instead but that is actually strictly worse because it also has a check that the current thread is the owning thread and it's possible (if weird) for a remote thread to call the deallocator.

The Py_REFCNT==1 check is fine because this is in a deallocator, the base array is not exposed to Python, and because dtypes aren't GC-tracked and don't support weakrefs. All of those facts combine to mean that it's not possible to observe Py_REFCNT==1 here and not be genuinely the only possible reference.

We could make dtypes be GC types and add trashcan tracking here but we would also have to figure out how to manage a migration story for user dtypes and their deallocators, which would need to be updated. We would also need to figure out how to create all the statically initialized singletons like LONG_Descr. All do-able but a much bigger -- and I'd argue riskier -- change. Also not backportable like this PR.

the base array is not exposed to Python

That part isn't strictly true, though?

import numpy as np
d = np.dtype("f", metadata={"a": 3})  # just to have a unique one.
d1 = np.dtype((d, 234))
d2 = np.dtype((d1, 235))
assert d2.base is d1
assert d1.base is d

The same thing can probably also happen for fields, fields that get cleared could recurse just the same.

All do-able but a much bigger -- and I'd argue riskier -- change. Also not backportable like this PR.

Yap, agreed, should be doable, but probably tedious.

You're right, I overstated my case. The thing that matters here is because we're in a deallocator and there's no gc tracking and no weakref support, it's not possible to observe a refcnt of 1 if there are external references, even on remote threads.

Your example is like the held variable in the test I added.

For fields, you're right that they can create recursion, but all the objects in the fields dict are types that CPython handles via the trashcan mechanism. The subarray field has to deal with the statically defined dtype types.

If we do migrate dtypes to heap types (maybe a numpy 3.0 thing along with some of the issues @prathamhole14 is identifying with static data in the C API), we'd need to update this code path to use the trashcan macros. I'll try to add comments so future grep searches find it.

The CLEAR macro does exactly that, though.

I don't really see the point of the subprocess. This seems much like the previous one (but yes won't fail due to recursion).
But the previous test already segfaults without the fix? Although I guess this one may segfault on more platforms :).