[GitHub] [arrow] pitrou commented on a change in pull request #12699: ARROW-16013: [C++][Python] Signed overflow when using negative stride in NumPyStridedConverter

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #12699:
URL: https://github.com/apache/arrow/pull/12699#discussion_r836438769



##########
File path: cpp/src/arrow/python/numpy_to_arrow.cc
##########
@@ -404,8 +404,13 @@ class NumPyStridedConverter {
     ARROW_ASSIGN_OR_RAISE(buffer_, AllocateBuffer(sizeof(T) * length_, pool_));
 
     const int64_t stride = PyArray_STRIDES(arr)[0];
-    if (stride % sizeof(T) == 0) {
-      const int64_t stride_elements = stride / sizeof(T);
+    // ARROW-16013: covert sizeof(T) to signed int64 first, otherwise dividing by it would
+    // do an unsigned division. This cannot be caught by tests without ubsan, since common
+    // signed overflow behavoir and the fact that the sizeof(T) is currently always a
+    // power of two here cause CopyStridedNatural to still produce correct results

Review comment:
       ```suggestion
       // ARROW-16013: convert sizeof(T) to signed int64 first, otherwise dividing by it would
       // do an unsigned division. This cannot be caught by tests without ubsan, since common
       // signed overflow behavior and the fact that the sizeof(T) is currently always a
       // power of two here cause CopyStridedNatural to still produce correct results
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org