[cpyrt] Only let reshape set dimensions of unknown size - #65
Conversation
| } | ||
| if (nlen == cpyrt::UNKNOWN_SIZE) // store as the creators would | ||
| nlen = idim == 0 ? fakemax : cpyrt::UNKNOWN_SIZE; | ||
| else if (PY_SSIZE_T_MAX / elemsize < nlen) |
There was a problem hiding this comment.
I think there is a pre-existing issue for row-pointer views:
For an int** view view.len is counted in view.itemsize: after reshape((5, 7)) memoryview(arr).nbytes correctly reports 5 * 8 = 40, but the overflow check derives nlen using elemsize, i.e. 5 * 4 (seen on current cppjit main). This mismatch only shows at the boundary, where (sys.maxsize // 4 - 1, -1) is accepted and memoryview(arr).nbytes reports -16. Perhaps we can also fix this pre-existing discrepancy in this PR by considering the length's unit in the check:
Py_ssize_t fakemax = fake_max(elemsize);
+ Py_ssize_t unit0 = view.ndim == 1 ? elemsize : view.itemsize;
...
- else if (PY_SSIZE_T_MAX / elemsize < nlen)
+ else if (PY_SSIZE_T_MAX / (idim == 0 ? unit0 : elemsize) < nlen)
...
- view.len = dims[0] * (view.ndim == 1 ? elemsize : view.itemsize);
+ view.len = dims[0] * unit0;With the m_int2a overflow example, I assume one should expect ValueError with arr.reshape((sys.maxsize // 4, -1))?
There was a problem hiding this comment.
Good catch, thanks! I could reproduce the pre-existing discrepancy on main: for m_int2a (itemsize 8, strides (8, 4)), arr.reshape((sys.maxsize // 4 - 1, -1)) was accepted and memoryview(arr).nbytes then reported -16.
I applied your suggestion essentially verbatim: unit0 (element size for rank-1 views, itemsize otherwise) is now used both in the dim-0 overflow check and in the view.len computation, which previously spelled out the same ternary.
And yes, exactly: arr.reshape((sys.maxsize // 4, -1)) (and // 4 - 1) now raises ValueError: ... the shape is too large. The boundary is where you'd expect it: (sys.maxsize // 8, -1) is still accepted with a positive nbytes, one more row is rejected. Rank-1 views are unaffected since unit0 equals the element size there.
Also added a regression test next to the existing inner-dimension overflow check:
raises(ValueError, arr.reshape, (sys.maxsize // 4 - 1, -1))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.
8174f8c to
18e91c7
Compare
…earch#65) 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.
…earch#65) 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.
…earch#65) 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.
…earch#65) 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.
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.