From 18e91c7c40ae35903a2d9e299a9201e330ccb4f8 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 1 Sep 2026 14:59:20 +0000 Subject: [PATCH] [cpyrt] Only let reshape set dimensions of unknown size LowLevelView.reshape() verified sizes by comparing the sum of the dimensions instead of the number of elements, their product: reshaping a five element array to (2, 3) was accepted because 5 == 2 + 3, while (1, 5) was rejected. The check is skipped for arrays of unknown size, the ones that typically get reshaped, which is why this went unnoticed. A correct size check is not enough, though: the strides and the converter projecting sub-views are chosen for the rank and layout of the view's C++ type and are not re-derived when reshaping. Any rank change produced a view reading garbage, and even the identity reshape of a fixed int[3][5] corrupted its strides. Reshape therefore now does what it is actually used for: providing the extent of dimensions the type leaves open, such as the size of an array behind a pointer. The rank must match, and only an unknown or empty dimension may be set, with -1 still standing for "unknown"; anything else raises ValueError, including dimensions whose byte size would overflow. The byte length is counted in strides of the outermost dimension as the creators count it, which for views with an itemsize override (const char*[], notably) differs from the itemsize, and the strides themselves are left as the creator laid them down. Also share the "fake max" marking an unknown outermost dimension between the creators and reshape (it was rederived from the itemsize, mistaking the unknown size of row-pointer and itemsize-overridden views for a known one), give the shape property a proper setter (reshape was installed directly despite its mismatching signature, so assignment misreported its result and deletion crashed), refuse to reshape a view without dimensions instead of reading through its null strides, and check allocations in the shape getter. Same issue as root-project/root#22512 (159ee7a1); its rework of rank-changing reshapes is left for a follow-up. --- src/cpyrt/LowLevelViews.cxx | 143 ++++++++++++++++++++++++------------ test/test_datatypes.py | 52 +++++++++++++ test/test_lowlevel.py | 58 +++++++++++++++ 3 files changed, 207 insertions(+), 46 deletions(-) diff --git a/src/cpyrt/LowLevelViews.cxx b/src/cpyrt/LowLevelViews.cxx index b4b5be1..532bc9f 100644 --- a/src/cpyrt/LowLevelViews.cxx +++ b/src/cpyrt/LowLevelViews.cxx @@ -35,6 +35,12 @@ static inline void set_strides(Py_buffer& view, size_t itemsize, bool isfix) { } } +// The creators mark an outermost dimension of unknown extent with this cap +// rather than with UNKNOWN_SIZE, keeping byte lengths and loops non-negative. +static inline Py_ssize_t fake_max(size_t elemsize) { + return INT_MAX / (Py_ssize_t)elemsize; +} + //= cpyrt low level view construction/destruction ========================= static cpyrt::LowLevelView* ll_new(PyTypeObject* subtype, PyObject*, PyObject*) { @@ -685,15 +691,44 @@ static PyObject* ll_shape(cpyrt::LowLevelView* self) { Py_buffer& view = self->fBufInfo; PyObject* shape = PyTuple_New(view.ndim); - for (Py_ssize_t idim = 0; idim < view.ndim; ++idim) - PyTuple_SET_ITEM(shape, idim, PyInt_FromSsize_t(view.shape[idim])); + if (!shape) + return nullptr; + for (Py_ssize_t idim = 0; idim < view.ndim; ++idim) { + PyObject* pydim = PyInt_FromSsize_t(view.shape[idim]); + if (!pydim) { + Py_DECREF(shape); + return nullptr; + } + PyTuple_SET_ITEM(shape, idim, pydim); + } return shape; } +//--------------------------------------------------------------------------- +static PyObject* ll_reshape_error(cpyrt::LowLevelView* self, PyObject* shape, + const char* why) { + PyObject* current = ll_shape(self); + if (!current) + return nullptr; + PyErr_Format(PyExc_ValueError, + "cannot reshape array of shape %S into shape %S: %s", current, + shape, why); + Py_DECREF(current); + return nullptr; +} + //--------------------------------------------------------------------------- static PyObject* ll_reshape(cpyrt::LowLevelView* self, PyObject* shape) { - // Allow the user to fix up the actual (type-strided) size of the buffer. + // Fill in the dimensions of the buffer that are not known from its type. + // + // A view is created with the rank and layout of its C++ type: a flat block + // for a rank-1 or fixed-size array, an array of row pointers otherwise. The + // strides and the converter projecting sub-views are derived from that and + // cannot be re-derived from a shape alone, so the rank of a view never + // changes and a dimension, once known, stays what it is. What reshaping can + // do is provide the extent of a dimension that the type leaves open, such as + // the size of an array behind a pointer. if (!PyTuple_Check(shape)) { if (shape) { PyObject* pystr = PyObject_Str(shape); @@ -709,58 +744,74 @@ static PyObject* ll_reshape(cpyrt::LowLevelView* self, PyObject* shape) { } Py_buffer& view = self->fBufInfo; - - // verify size match - Py_ssize_t oldsz = 0; - for (Py_ssize_t idim = 0; idim < view.ndim; ++idim) { - Py_ssize_t nlen = view.shape[idim]; - if (nlen == cpyrt::UNKNOWN_SIZE || - nlen == INT_MAX / view.itemsize /* fake 'max' */) { - oldsz = -1; // meaning, unable to check size match - break; - } - oldsz += view.shape[idim]; + if (view.ndim < 1 || !view.shape || !view.strides) { + PyErr_SetString(PyExc_TypeError, + "this low level view has no dimensions to set"); + return nullptr; } - if (0 < oldsz) { - Py_ssize_t newsz = 0; - for (Py_ssize_t idim = 0; idim < PyTuple_GET_SIZE(shape); ++idim) - newsz += PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim)); - if (oldsz != newsz) { - PyObject* tas = PyObject_Str(shape); - PyErr_Format(PyExc_ValueError, - "cannot reshape array of size %ld into shape %s", - (long)oldsz, cpyrt_PyText_AsString(tas)); - Py_DECREF(tas); + // An unknown outermost dimension holds the fake max for the element size + // (the innermost stride), any other unknown dimension UNKNOWN_SIZE. An + // empty dimension of a non-fixed view may also be filled in: it came from + // a pointer, with nothing behind it yet that a size could contradict. + bool isfix = (intptr_t)view.internal & cpyrt::LowLevelView::kIsFixed; + Py_ssize_t elemsize = view.strides[view.ndim - 1]; + Py_ssize_t fakemax = fake_max(elemsize); + Py_ssize_t unit0 = view.ndim == 1 ? elemsize : view.itemsize; + + Py_ssize_t ndim = PyTuple_GET_SIZE(shape); + cpyrt::dims_t dims(ndim); + for (Py_ssize_t idim = 0; idim < ndim; ++idim) { + Py_ssize_t nlen = PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim)); + if (nlen == -1 && PyErr_Occurred()) + return nullptr; + if (nlen < cpyrt::UNKNOWN_SIZE) { + PyErr_SetString(PyExc_ValueError, "negative dimensions are not allowed"); return nullptr; } + if (nlen == cpyrt::UNKNOWN_SIZE) // store as the creators would + nlen = idim == 0 ? fakemax : cpyrt::UNKNOWN_SIZE; + else if (PY_SSIZE_T_MAX / (idim == 0 ? unit0 : elemsize) < nlen) + return ll_reshape_error(self, shape, "the shape is too large"); + dims[idim] = nlen; } - // reshape - size_t itemsize = view.strides[view.ndim - 1]; - if (view.ndim != PyTuple_GET_SIZE(shape)) { - PyMem_Free(view.shape); - PyMem_Free(view.strides); - - view.ndim = (int)PyTuple_GET_SIZE(shape); - view.shape = (Py_ssize_t*)PyMem_Malloc(view.ndim * sizeof(Py_ssize_t)); - view.strides = (Py_ssize_t*)PyMem_Malloc(view.ndim * sizeof(Py_ssize_t)); + if (ndim != view.ndim) + return ll_reshape_error( + self, shape, + "the number of dimensions of a low level view is fixed by its type"); + + for (Py_ssize_t idim = 0; idim < ndim; ++idim) { + Py_ssize_t cur = view.shape[idim]; + bool unknown = cur == cpyrt::UNKNOWN_SIZE || + (idim == 0 && cur == fakemax) || (cur == 0 && !isfix); + if (!unknown && dims[idim] != cur) + return ll_reshape_error(self, shape, + "only dimensions of unknown size can be set"); } - for (Py_ssize_t idim = 0; idim < PyTuple_GET_SIZE(shape); ++idim) { - Py_ssize_t nlen = PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim)); - if (nlen == -1 && PyErr_Occurred()) - return nullptr; + for (Py_ssize_t idim = 0; idim < ndim; ++idim) + view.shape[idim] = dims[idim]; - if (idim == 0) - view.len = nlen * view.itemsize; + // the byte length counts the outermost dimension as the creators set it; + // the strides depend only on the layout and stay as they were laid down + view.len = dims[0] * unit0; - view.shape[idim] = nlen; - } + Py_RETURN_NONE; +} - set_strides(view, itemsize, false /* by definition not fixed */); +//--------------------------------------------------------------------------- +static int ll_setshape(cpyrt::LowLevelView* self, PyObject* value, void*) { + if (!value) { + PyErr_SetString(PyExc_TypeError, "cannot delete the shape of a view"); + return -1; + } - Py_RETURN_NONE; + PyObject* result = ll_reshape(self, value); + if (!result) + return -1; + Py_DECREF(result); + return 0; } //--------------------------------------------------------------------------- @@ -864,7 +915,7 @@ static PyGetSetDef ll_getset[] = { (char*)"If true, this array was allocated with C++\'s new[]", nullptr}, {(char*)"format", (getter)ll_typecode, nullptr, nullptr, nullptr}, {(char*)"typecode", (getter)ll_typecode, nullptr, nullptr, nullptr}, - {(char*)"shape", (getter)ll_shape, (setter)ll_reshape, nullptr, nullptr}, + {(char*)"shape", (getter)ll_shape, (setter)ll_setshape, nullptr, nullptr}, {(char*)nullptr, nullptr, nullptr, nullptr, nullptr}}; namespace cppjit::cpyrt { @@ -1038,9 +1089,9 @@ CreateLowLevelViewT(T* address, cpyrt::cdims_t shape, Py_ssize_t itemsize = -1) { using namespace cppjit::cpyrt; Py_ssize_t nx = - (shape.ndim() != UNKNOWN_SIZE) ? shape[0] : INT_MAX / sizeof(T); + (shape.ndim() != UNKNOWN_SIZE) ? shape[0] : fake_max(sizeof(T)); if (nx == UNKNOWN_SIZE) - nx = INT_MAX / sizeof(T); + nx = fake_max(sizeof(T)); PyObject* args = PyTuple_New(0); LowLevelView* llp = (LowLevelView*)LowLevelView_Type.tp_new( &LowLevelView_Type, args, nullptr); diff --git a/test/test_datatypes.py b/test/test_datatypes.py index f8a0f81..e5f5f56 100644 --- a/test/test_datatypes.py +++ b/test/test_datatypes.py @@ -1302,6 +1302,58 @@ def test23_buffer_reshaping(self): for i in range(self.N): assert arr[i] == l[i] + # a fixed-size array accepts only its own shape, and stays intact + arr = c.m_int_array + assert arr.shape == (self.N,) + arr.reshape((self.N,)) + arr.shape = (self.N,) + assert arr.shape == (self.N,) + assert list(arr) == list(c.m_int_array) + + # (2, 3) was accepted when sizes were compared as sums (2 + 3 == N) + raises(ValueError, arr.reshape, (2, 3)) + raises(ValueError, arr.reshape, (self.N + 1,)) + raises(ValueError, arr.reshape, (-1,)) + assert arr.shape == (self.N,) + assert list(arr) == list(c.m_int_array) + + # shapes that do not describe an array (-1 means "unknown" and is fine) + arr = c.get_int_array() + raises(ValueError, arr.reshape, ()) + raises(ValueError, arr.reshape, (-2,)) + raises(ValueError, arr.reshape, (sys.maxsize,)) + raises(TypeError, arr.reshape, (1.5, 2)) + + # sizing to (0,) does not fix the size of an unknown view, others do + arr = c.get_int_array() + arr.reshape((-1,)) + assert arr.shape[0] > 0 + assert len(arr) == arr.shape[0] + arr.reshape((0,)) + assert len(arr) == 0 + assert list(arr) == [] + arr.shape = (self.N,) + assert arr.shape == (self.N,) + assert len(list(arr)) == self.N + with raises(ValueError): + arr.shape = (2 * self.N,) + with raises(TypeError): + del arr.shape + assert arr.shape == (self.N,) + + # a flexible array member is of unknown size whatever its type; sizing + # it counts bytes in element strides, not the (overridden) itemsize + cppjit.cppdef("""\ + namespace ReshapeFlex { + struct Names { int n; const char* names[]; }; + }""") + + names = cppjit.gbl.ReshapeFlex.Names() + arr = names.names + arr.reshape((3,)) + assert arr.shape == (3,) + assert memoryview(arr).nbytes == 3 * memoryview(arr).strides[0] + def test24_voidp(self): """Test usage of void* data""" diff --git a/test/test_lowlevel.py b/test/test_lowlevel.py index 7a4be36..8bb395e 100644 --- a/test/test_lowlevel.py +++ b/test/test_lowlevel.py @@ -866,6 +866,7 @@ def test01_2D_arrays(self): data2c = self._data_m("2c") for m, tp in data2c: arr = getattr(h, m) + arr.reshape((3, 5)) # its own shape, the only one it accepts assert arr.shape == (3, 5) elem_tp = getattr(cppjit.gbl, tp) for i in range(3): @@ -1101,3 +1102,60 @@ def test07_3D_custom_struct(self): for j in range(gbl.S + 3): for k in range(gbl.S + 7): assert gbl.consume_klass(gbl.klasses[i][j][k], i, j, k) + + def test08_reshape_sets_unknown_dimensions_only(self): + """Reshaping fills in the dimensions the type leaves open""" + + import cppjit + import cppjit.ll + + h = cppjit.gbl.MultiDimArrays.DataHolder() + + # a fixed-size array accepts only its own shape, and that must leave + # its strides intact (a plain reshape used to corrupt them) + arr = h.m_int2c + assert arr.shape == (3, 5) + strides = memoryview(arr).strides + arr.reshape((3, 5)) + assert arr.shape == (3, 5) + assert memoryview(arr).strides == strides + for i in range(3): + for j in range(5): + assert arr[i][j] == 3 * i + j + assert arr[i, j] == 3 * i + j + + raises(ValueError, arr.reshape, (5, 3)) + raises(ValueError, arr.reshape, (15,)) + assert arr.shape == (3, 5) + assert arr[2][4] == 3 * 2 + 4 + + # unknown dimensions can be set one at a time and, once set, stay + arr = h.m_int2a + assert len(arr.shape) == 2 + assert arr.shape[1] == -1 + raises(ValueError, arr.reshape, (35,)) + arr.reshape((5, -1)) + assert arr.shape[0] == 5 and arr.shape[1] == -1 + raises(ValueError, arr.reshape, (7, -1)) + arr.reshape((5, 7)) + assert arr.shape == (5, 7) + for i in range(5): + for j in range(7): + assert arr[i][j] == h.m_int2a[i, j] + + # a rank-1 pointer view cannot become multi-dimensional either + buf = cppjit.ll.malloc["int"](6) + assert buf.shape == (6,) + raises(ValueError, buf.reshape, (2, 3)) + assert buf.shape == (6,) + cppjit.ll.free(buf) + + # a dimension that would overflow the byte size is rejected, too; the + # outermost one counts row pointers here, not ints (used to slip by) + arr = cppjit.gbl.MultiDimArrays.DataHolder().m_int2a + raises(ValueError, arr.reshape, (5, sys.maxsize)) + raises(ValueError, arr.reshape, (sys.maxsize // 4 - 1, -1)) + + # a freshly constructed view has no dimensions to set + v = cppjit._backend.LowLevelView() + raises(TypeError, v.reshape, ())