BUG: avoid segfaults when legacy copyswap slot is not defined by ngoldbaum · Pull Request #32151 · numpy/numpy

ngoldbaum · 2026-07-30T17:48:13Z

PR summary

Towards addressing #24859 (see also #30795 ping @SwayamInSync). This avoids segfaults in many operations if copyswap or copyswapn isn't defined. Also see #32150 which does the same thing along with a small behavior change in np.byteswap.

AI Disclosure

I used an AI model to search the codebase and debug/test the fixes.

SwayamInSync · 2026-07-30T18:03:26Z

This was needed (I had it in my next todos too) I think prevention is good for backporting, and maybe for future releases should we also think about adding a slot?
@seberg has thoughts that user-dtypes having byteorder parameter might be more useful but he is also okay with adding new slot

ngoldbaum · 2026-07-30T18:12:24Z

maybe for future releases should we also think about adding a slot?

Sounds good to me if you want to take that on!

ngoldbaum · 2026-07-30T18:13:01Z

And can you give this an official code review pass? We're a bit short on reviewers right now so every review helps.

SwayamInSync

Maybe need to tight some areas, specially around introducing an error-producing recursive VOID_copyswap without updating this success-returning caller.

SwayamInSync · 2026-07-30T19:32:42Z

    PyArray_CopySwapNFunc *copyswapn;
    PyArrayIterObject *it;

    copyswapn = PyDataType_GetArrFuncs(PyArray_DESCR(self))->copyswapn;


The recursive structured-dtype paths are fixed here, but the top-level PyArray_Byteswap path still calls a potentially NULL copyswapn. I verified that direct SF array byteswap still exits with SIGSEGV:

SF = _get_sfloat_dtype() np.array([1., 2.]).view(SF(1.)).byteswap()

I see that #32150 handles this case. Since this PR is marked as a backport candidate while #32150 is a separate enhancement, should they be treated as dependent for release/backport purposes? Otherwise, should the title or summary clarify that this PR covers the structured/recursive call paths rather than all missing-copyswap[n] cases?

I can do a second backport-only version that doesn't change any behavior and unconditionally raises an error. But that can land directly on the 2.5 branch.

Actually on second thought, to make the backport easier, I'll add just the null-check and TypeError on this branch.

ngoldbaum

I addressed three of @SwayamInSync's comments and left a comment about how to handle backporting the fix in #32150

ngoldbaum · 2026-07-30T21:09:54Z

    PyArray_CopySwapNFunc *copyswapn;
    PyArrayIterObject *it;

    copyswapn = PyDataType_GetArrFuncs(PyArray_DESCR(self))->copyswapn;


I can do a second backport-only version that doesn't change any behavior and unconditionally raises an error. But that can land directly on the 2.5 branch.

SwayamInSync

LGTM

ngoldbaum · 2026-07-31T15:59:03Z

@SwayamInSync sorry to bother you but can I get you to do one more pass over this? I made it so the helper always errors but I left in-place the infrastructure for relaxing that (in #32150). Does that make sense to you? Do you see any issues in how I did it?

SwayamInSync

Some nits, would be quick to resolve.

SwayamInSync · 2026-08-01T11:05:36Z

    PyArrayIterObject *it;

    copyswapn = PyDataType_GetArrFuncs(PyArray_DESCR(self))->copyswapn;
+    if (copyswapn == NULL) {


Could the existing in-place writeability check remain before this missing-slot check?

A read-only new-style array previously returned from PyArray_FailUnlessWriteable before reaching the NULL copyswapn call. With this ordering it now raises the dtype TypeError instead:

arr = np.zeros(2, dtype=SF(1.)) arr.flags.writeable = False arr.byteswap(inplace=True)

Previously this raised ValueError: array to be byte-swapped is read-only this branch now raises TypeError: dtype ... does not implement copyswap
Moving the inplace && PyArray_FailUnlessWriteable(...) check before the slot check would preserve that existing behavior. It would also ensure that the retained no-op path still rejects read-only arrays when is relaxed in #32150.

I'll take a look at this suggestion

SwayamInSync · 2026-08-01T11:15:34Z

            Py_DECREF(descr);
            return PyErr_NoMemory();
        }
+        else if (copyswap == NULL) {


Since this branch is being retained for the later relaxation in #32150, I think scalar reconstruction will need different handling when check_missing_copyswap starts returning success.

For a normal new-style scalar, this branch reaches:

PyArray_Scalar(newmem, descr, NULL)

That dispatches through legacy_getitem_using_DType, which rejects the NULL array argument. I reproduced this with a quaddtype scalar when the earlier helper treated byteorder == '|' as a no-op:

RuntimeError: Using legacy SETITEM with NULL array object is only supported for basic NumPy DTypes.

The current always-error policy does not reach this code, so this is not a blocker for the backportable fix. But #32150 should either handle the immutable scalar as a direct no-op or reconstruct it through the new-style getitem slot rather than activating this path unchanged.

I'll just delete this dead branch in this PR and deal with this issue in #32150, thanks for pointing it out.

SwayamInSync · 2026-08-01T11:15:40Z

+                }
+                continue;
+            }
+            copyswapn(dst+offset, dstride,


One non-blocking behavior question: with the new always-error policy, an in-place structured byteswap can modify supported fields encountered before an unsupported field and then raise TypeError, leaving the array partially byteswapped.

For example:

arr = np.zeros( 2, dtype=[("before", "i4"), ("v", SF(1.)), ("after", "i4")] ) arr["before"] = [1, 2] arr["after"] = [3, 4] arr.byteswap(inplace=True)

After the TypeError, before has been byteswapped to [16777216, 33554432], while after remains [3, 4].

Is partial mutation acceptable for this in-place API, or should the fields be preflighted for missing copyswap slots before mutation? I do not know of an atomicity guarantee here, so this is non-blocking.

I guess I can live this it... Would be nicer if we just checked things ahead of time, but that seems tedious. (The point of the PR is in the end to avoid a segfault, so...)

I'll take a look at whether checking first makes the diff complicated, if it does I'll defer this to the followup PR.

SwayamInSync · 2026-08-01T11:17:06Z

@SwayamInSync sorry to bother you but can I get you to do one more pass over this?

Hey no worries, I will be happy to review more in the case we are being short on reviewers

seberg

Thanks, generally seems fine and should also be fine to backport.

Part of me wonders if we should abuse the _former_flags field or so to flag support for void compare and copyswap (others?) at creation time, but I dunno if that's worth it.

I would be tempted to delete at least the setitem fast-path, but for the "just get this fixed" part, probably not worth it, even if the try/except logic is ugly.

I guess my only real request is to rename check_missing_copyswap and I don't really see the point of the swap_only (at least call it inplace_swap)?!

seberg · 2026-08-03T12:07:07Z


+/* See common.h for the contract of this helper. */
+NPY_NO_EXPORT int
+check_missing_copyswap(PyArray_Descr *dtype, int swap_only)


Can we rename this or actually make it check?

The check is coming in a followup (#32150). I'm setting up the internal API just to make the diff of the followup smaller.

But I will change the name to inplace_swap, you're right it's better.

Sorry, late and not blocking - but if this checks in #32150, would we always keep chaining with == NULL &&? But then it is already found missing, and the function should not need to check?

Since we're restructuring all the call sites, we could just output copyswap, as in get_copyswap_or_raise(PyArray_Descr *dtype, int inplace_swap, PyArray_CopySwapFunc** copyswap)? Seems a bit 'out there', but this is a narrow use-case. Probably too much for this PR except for a rename if needed.

seberg · 2026-08-03T12:12:03Z

+            }
+        }
+        if (i == names_size) {
+            return 0;


I almost wonder if we should just hope that TRIVIALLY_COPYABLE s now typically set (on the destination I guess), add a fast-path that makes use of that, and just delete this whole fast-path... (In part, because this try/except style s annoying as well, but also because the fast-path should be hit often and actually be fast as promised.)

But yeah, looks OK here.

Thanks! I'll think about folding this into #32150.

seberg · 2026-08-03T12:13:40Z

+                }
+                continue;
+            }
+            copyswapn(dst+offset, dstride,


I guess I can live this it... Would be nicer if we just checked things ahead of time, but that seems tedious. (The point of the PR is in the end to avoid a segfault, so...)

[2.4.x backport: rebased onto the numpy#32151 backport — replaces the strict check_missing_copyswap() guard in PyArray_Byteswap with the no-op fallback, and removes test_byteswap_raises, whose TypeError expectation for byteorder-less dtypes this commit deliberately changes to a no-op; the read-only inplace ValueError path stays covered by the StringDType test_byteswap.] (cherry picked from commit 18f6dfa)

MaanasArora · 2026-08-06T02:35:08Z

Looks good and correct, thanks! Also just minor question about the work check_missing_copyswap is doing (comment in thread).

ngoldbaum · 2026-08-06T20:26:35Z

OK, let's get this in so it ends up in the bugfix release this weekend.

BUG: avoid segfaults when legacy copyswap slot is not defined (#32151)

…32151)

BUG: avoid segfaults when legacy copyswap slot is not defined

599cbed

ngoldbaum added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported component: numpy.dtype labels Jul 30, 2026

SwayamInSync suggested changes Jul 30, 2026

View reviewed changes

MAINT: address review comments

cb863ac

ngoldbaum commented Jul 30, 2026

View reviewed changes

SwayamInSync approved these changes Jul 31, 2026

View reviewed changes

MAINT: Make all operations that segfaulted raise TypeError

dec685f

ngoldbaum mentioned this pull request Jul 31, 2026

ENH: add no-op fallback in np.byteswap for types with no byte order #32150

Merged

SwayamInSync reviewed Aug 1, 2026

View reviewed changes

seberg reviewed Aug 3, 2026

View reviewed changes

ngoldbaum added 3 commits August 3, 2026 16:22

MAINT: address review comments and delete dead code branches

2b44ffe

DOC: strengthen comment

72d44c4

BUG: early-exit on error for .flat and .place

fbabf73

MAINT: call it raise_missing_copyswap

6af11ac

ngoldbaum merged commit e117b3c into numpy:main Aug 6, 2026
115 of 128 checks passed

charris mentioned this pull request Aug 7, 2026

BUG: avoid segfaults when legacy copyswap slot is not defined (#32151) #32221

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 7, 2026

charris pushed a commit that referenced this pull request Aug 7, 2026

BUG: avoid segfaults when legacy copyswap slot is not defined (#32151)

23b30f4

charris added a commit that referenced this pull request Aug 7, 2026

Merge pull request #32221 from charris/backport-32151

ecf599c

BUG: avoid segfaults when legacy copyswap slot is not defined (#32151)

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

BUG: avoid segfaults when legacy copyswap slot is not defined (numpy#…

a7828c8

…32151)

Conversation

ngoldbaum commented Jul 30, 2026

PR summary

AI Disclosure

Uh oh!

SwayamInSync commented Jul 30, 2026

Uh oh!

ngoldbaum commented Jul 30, 2026

Uh oh!

ngoldbaum commented Jul 30, 2026

Uh oh!

SwayamInSync left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SwayamInSync left a comment

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Jul 31, 2026

Uh oh!

SwayamInSync 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!

SwayamInSync commented Aug 1, 2026

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!

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!

MaanasArora commented Aug 6, 2026

Uh oh!

ngoldbaum commented Aug 6, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development