BUG: fix StringDType distinct-allocator bugs and add tests by ngoldbaum · Pull Request #31609 · numpy/numpy

ngoldbaum · 2026-06-11T20:52:15Z

Fixes bugs when multiple distinct stringdtype arrays are passed to np.choose, np.put, np.putmask, np.place, flatiter reads, and a bug that could lead to unsafe views being created for StringDType and object arrays.

Also exposes a new C helper function _is_safe_view_cast which takes view_offset into account and uses it in the Python helper functions for view safety. It only uses this function for StringDType. We could use it more generally but that might lead to breakage, so just fixing StringDType seemed like a practical first pass.

The C changes all work out to fixing the same couple patterns: legacy paths that used copyswap (e.g. #24859, which this might obviate the need for?) or code paths that assumed all input arrays have the same descriptor instance.

Also adds tests for functionality that is fixed here or is poorly tested currently. It makes use of a new helper function that allocates two distinct arrays of strings with allocations that live in the arena buffer to trigger these bugs.

I milestoned this for 2.5.0 and marked it for backport because these are all bugs that can cause segfaults.

I used Claude Fable 5 to work on this, including for the implementation and new tests.

ngoldbaum · 2026-06-11T21:42:44Z

that debug interpreter test crash looks real

ngoldbaum · 2026-06-12T15:14:48Z

I think this is ready for review now. @seberg @MaanasArora @mhvk - hope you don't mind the ping. The C diff isn't so huge, most of the changes are new tests.

MaanasArora · 2026-06-13T08:52:25Z

     * Convert all inputs to arrays of a common type
     * Also makes them C-contiguous
     */
    mps = PyArray_ConvertToCommonType(op, &n);


For the PyArray_Choose fix here, I noticed we actually discussed this bug in #28206 (comment). Is it better to fix the PyArray_ConvertToCommonType function directly to convert all arrays to use an identical descriptor? Then I think we will lose the complexity below, as we only have one descriptor to cast from!

(have to catch up with actual review still; we forgot to open a follow-up issue back then, I guess.)

Theoretically, but it would require copying all the inputs even if they have the right dtypes already basically; and it would be a bit strange to force that (i.e. the point normally is for all arrays to have their own dtype for StringDType).

But without that, each casts needs the identity dtype passed in and that requires one cast per array.

OK yes, might be a bit strange to copy and then cast separately later, thanks.

have to catch up with actual review still; we forgot to open a follow-up issue back then, I guess.

Sorry for dumping a bunch of code on you on a Friday. Take your time!

seberg

Thanks, a few comments. In some places I think you/the bot thought the RefCheck was a pattern to generalize, but it is used in a few places due to bare memcpy use not to avoid copyswap (although it may be fine to do so additionally).

Otherwise a few small nits, many take-it-or-leave it. I do also think the view safety check should be used generally if we create it, there is nothing special about StringDType (and it can short-circuit the ==, I think.

seberg · 2026-06-17T11:53:00Z

-            if (j >= nv) {
-                j = 0;
+    if (PyDataType_REFCHK(PyArray_DESCR(array))) {
+        /* dtypes with references may not define copyswap */


Please change this to an explicit check for ->copyswap being NULL, while it may be true in practice, I don't think the comment really adds up.

seberg · 2026-06-17T11:56:27Z

+        NPY_cast_info cast_info;
+        NPY_ARRAYMETHOD_FLAGS flags;
+        const npy_intp one = 1;
+        const npy_intp strides[2] = {chunk, chunk};


This is better as elsize (zero is also OK, but elsize is probably better in practice to hit the contiguous loops).
chunk just doesn't make sense here at all.

seberg · 2026-06-17T11:56:49Z

+        NPY_cast_info_init(&cast_info);
+        if (PyArray_GetDTypeTransferFunction(
+                PyArray_ISALIGNED(values) && PyArray_ISALIGNED(array),
+                chunk, chunk,


Should be identical to strides above, just use:

Suggested change

chunk, chunk,

strides[0], strides[1],

seberg · 2026-06-17T12:00:49Z

+ * separate internal state (e.g. StringDType allocators).
+ */
+NPY_NO_EXPORT PyObject *
+_is_view_safe_cast(PyObject *NPY_UNUSED(module), PyObject *args)


Thanks, I really like this helper! I do have a bit of a dislike for PyArg_ParseTuple (it's just silly slow API...), but I don't mind keeping it.

Changed it to METH_FASTCALL.

seberg · 2026-06-17T12:04:34Z

-    swap = PyArray_ISNOTSWAPPED(self) != PyArray_ISNOTSWAPPED(arr);
-    copyswap = PyDataType_GetArrFuncs(PyArray_DESCR(self))->copyswap;
    if (PyDataType_REFCHK(PyArray_DESCR(self))) {
+        /* dtypes with references may not define copyswap */


Again, check explicitly for copyswap.

Effectively it means that StringDType shouldn't implement it even if we allowed that (it's far better off with this pattern due to lock handling) but that seems reasonable.

Now... what is completely crazy here is the whole manual PyArray_Item_XDECREF nonsense, which looks like copyswap() would handle it just fine for objects (and structured dtypes).

For structured dtypes the new path may well be better (for larger arrays). So I would be fine with keeping the REFCHCK additionally to copyswap == NULL.

(Another interesting thing, but let's not act on it, I guess... This function should release the GIL for larger inputs but doesn't seem to.)

seberg · 2026-06-17T12:23:44Z

    dest = PyArray_DATA(self);

    if (PyDataType_REFCHK(PyArray_DESCR(self))) {
        NPY_cast_info cast_info;


Just to note that I am not insane about the copyswap thing above. This path is different, because we assume that memcpy is sufficient (copyswap isn't even used).

(There is a new flag for optimizing structured dtypes that we should possibly check here as well, NOT_TRIVIALLY_COPYABLE but not this PR really... Won't complain if you add it, it's basically an extremely niche fix on structured dtypes.)

seberg · 2026-06-17T12:29:01Z

-                    dtype,
-                    obj_dtype, 0, &cast_info,
-                    &transfer_flags);
+        cast_infos = PyMem_New(NPY_cast_info, n);


Could use NPY_ALLOC_WORKSPACE or... just use NPY_MAXARGS the MultiIter is limited to that anyway (although of course it isn't explitily from here).

(I don't care though, I doubt micro-optimizations matter much for choose.)

seberg · 2026-06-17T12:31:42Z

+                        obj_dtype->elsize,
+                        PyArray_DESCR(mps[i]),
+                        obj_dtype, 0, &cast_infos[i],
+                        &transfer_flags) < 0) {


We could release the GIL here based on transfer_flags (although, I suppose while it can't matter, in principle we should check all transfer flags).

This doesn't matter for objects, but it does matter a bit for strings.

seberg · 2026-06-17T12:42:53Z

                        if (y_cast_info.func(
                                &y_cast_info.context, args, &one,
                                y_strides, y_cast_info.auxdata) < 0) {
+                            NPY_END_THREADS;


Seems nicer to me to just put it into fail: here (requires moving the THREADS_DEF to the start of the function).

seberg · 2026-06-17T12:49:00Z

+        raise TypeError(
+            "Cannot view StringDType array data through a different dtype "
+            "instance; use astype to copy the data to the new dtype "
+            "instance")


If you care for the StringDType error that is fine, but I don't think you need to guard for it here, in theory StringDType isn't special.
Maybe _is_view_safe_cast is not the best name here, but that function can completely replace the newtype == oldtype, I think?

ngoldbaum · 2026-06-18T21:42:16Z

I think I applied all your suggestions in the last commit. If you could give it another look I'd appreciate it.

seberg

Thanks, only had a brief look through the new changes, but looks all good. (If you feel like it and haven't done so, I might have thrown an agent once more but don't have it set up here right now.)

Please go ahead and merge!

ngoldbaum · 2026-06-20T15:37:26Z

In it goes, thanks for taking a look and giving me feedback Sebastian!

BUG: fix StringDType distinct-allocator bugs and add tests (#31609)

(cherry picked from commit 706b103)

ngoldbaum added this to the 2.5.0 Release milestone Jun 11, 2026

ngoldbaum added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported component: numpy.strings String dtypes and functions labels Jun 11, 2026

ngoldbaum mentioned this pull request Jun 12, 2026

BUG: fix incorrect alignment handling and error handling in several code paths #31615

Merged

MaanasArora reviewed Jun 13, 2026

View reviewed changes

charris mentioned this pull request Jun 13, 2026

BUG: fix incorrect alignment handling and error handling in several code paths (#31615) #31631

Merged

ngoldbaum added 3 commits June 16, 2026 08:39

BUG: fix StringDType distinct-allocator bugs and add tests

2bd344c

BUG: fix incorrect NPY_END_THREADS placement

46513a5

BUG: fix error case not re-acquiring the GIL in PyArray_Where

ef88174

ngoldbaum force-pushed the fix-stringdtype-legacy-api branch from 5915100 to ef88174 Compare June 16, 2026 14:40

seberg reviewed Jun 17, 2026

View reviewed changes

MNT: respond to Sebastian's suggestions

4236639

seberg approved these changes Jun 20, 2026

View reviewed changes

ngoldbaum merged commit 706b103 into numpy:main Jun 20, 2026
85 of 87 checks passed

charris mentioned this pull request Jun 20, 2026

BUG: fix StringDType distinct-allocator bugs and add tests (#31609) #31703

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 20, 2026

charris pushed a commit that referenced this pull request Jun 20, 2026

BUG: fix StringDType distinct-allocator bugs and add tests (#31609)

3de6203

charris added a commit that referenced this pull request Jun 20, 2026

Merge pull request #31703 from charris/backport-31609

ce17c81

BUG: fix StringDType distinct-allocator bugs and add tests (#31609)

rgommers mentioned this pull request Jun 24, 2026

BUG: segfaults in macOS arm64 wheel builds #31733

Closed

ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Aug 4, 2026

BUG: fix StringDType distinct-allocator bugs and add tests (numpy#31609)

3224893

(cherry picked from commit 706b103)

ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Sep 8, 2026

BUG: fix StringDType distinct-allocator bugs and add tests (numpy#31609)

ec4164d

Conversation

ngoldbaum commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaanasArora Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Jun 18, 2026

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Jun 20, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ngoldbaum commented Jun 11, 2026 •

edited

Loading

ngoldbaum commented Jun 11, 2026 •

edited

Loading

ngoldbaum commented Jun 12, 2026 •

edited

Loading

MaanasArora Jun 13, 2026 •

edited

Loading

seberg Jun 13, 2026 •

edited

Loading