[arrow-nanoarrow] branch main updated: fix(r): Don't invoke undefined behaviour in conversions to/from Arrow (#167)

[pa...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

paleolimbot pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-nanoarrow.git


The following commit(s) were added to refs/heads/main by this push:
     new 7f5f358  fix(r): Don't invoke undefined behaviour in conversions to/from Arrow (#167)
7f5f358 is described below

commit 7f5f358cfec806ecde088f001aa8121e600bcaf4
Author: Dewey Dunnington <de...@dunnington.ca>
AuthorDate: Sat Mar 25 21:08:42 2023 -0400

    fix(r): Don't invoke undefined behaviour in conversions to/from Arrow (#167)
    
    Closes #159.
    
    Checked via:
    
    ```bash
    PKG_CFLAGS=-fsanitize=undefined PKG_LIBS=-fsanitize=undefined R CMD INSTALL . --preclean
    ```
    
    Then:
    
    ```r
    devtools::test()
    ```
---
 r/src/as_array.c                      | 12 ++++++++++--
 r/tests/testthat/test-as-array.R      |  6 +++---
 r/tests/testthat/test-convert-array.R |  4 ++--
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/r/src/as_array.c b/r/src/as_array.c
index e8eac8c..592e83b 100644
--- a/r/src/as_array.c
+++ b/r/src/as_array.c
@@ -236,7 +236,12 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
 
     int64_t* buffer_data = (int64_t*)buffer->data;
     for (int64_t i = 0; i < len; i++) {
-      buffer_data[i] = x_data[i];
+      // UBSAN warns for buffer_data[i] = nan
+      if (R_IsNA(x_data[i]) || R_IsNaN(x_data[i])) {
+        buffer_data[i] = 0;
+      } else {
+        buffer_data[i] = x_data[i];
+      }
     }
 
     buffer->size_bytes = len * sizeof(int64_t);
@@ -254,7 +259,10 @@ static void as_array_dbl(SEXP x_sexp, struct ArrowArray* array, SEXP schema_xptr
     // It's easy to accidentally overflow here, so make sure to warn
     int64_t n_overflow = 0;
     for (int64_t i = 0; i < len; i++) {
-      if (x_data[i] > INT_MAX || x_data[i] < INT_MIN) {
+      // UBSAN warns for buffer_data[i] = nan
+      if (R_IsNA(x_data[i]) || R_IsNaN(x_data[i])) {
+        buffer_data[i] = 0;
+      } else if (x_data[i] > INT_MAX || x_data[i] < INT_MIN) {
         n_overflow++;
         buffer_data[i] = 0;
       } else {
diff --git a/r/tests/testthat/test-as-array.R b/r/tests/testthat/test-as-array.R
index 6beff31..fac5e65 100644
--- a/r/tests/testthat/test-as-array.R
+++ b/r/tests/testthat/test-as-array.R
@@ -163,10 +163,10 @@ test_that("as_nanoarrow_array() works for double() -> na_int32()", {
     as.raw(array$buffers[[1]]),
     packBits(c(rep(TRUE, 10), FALSE, rep(FALSE, 5)))
   )
-  # The last element here is (int)NaN not NA_integer_
+  # The last element here is 0 because (int)nan is undefined behaviour
   expect_identical(
-    head(as.raw(array$buffers[[2]]), 10 * 4L),
-    as.raw(as_nanoarrow_buffer(1:10))
+    as.raw(array$buffers[[2]]),
+    as.raw(as_nanoarrow_buffer(c(1:10, 0L)))
   )
 
   # With overflow
diff --git a/r/tests/testthat/test-convert-array.R b/r/tests/testthat/test-convert-array.R
index 4175c10..7b982b7 100644
--- a/r/tests/testthat/test-convert-array.R
+++ b/r/tests/testthat/test-convert-array.R
@@ -329,13 +329,13 @@ test_that("convert to vector works for null -> logical()", {
 })
 
 test_that("convert to vector warns for invalid integer()", {
-  array <- as_nanoarrow_array(.Machine$double.xmax)
+  array <- as_nanoarrow_array(.Machine$integer.max + 1)
   expect_warning(
     expect_identical(convert_array(array, integer()), NA_integer_),
     "1 value\\(s\\) outside integer range set to NA"
   )
 
-  array <- as_nanoarrow_array(c(NA, .Machine$double.xmax))
+  array <- as_nanoarrow_array(c(NA, .Machine$integer.max + 1))
   expect_warning(
     expect_identical(convert_array(array, integer()), c(NA_integer_, NA_integer_)),
     "1 value\\(s\\) outside integer range set to NA"