Merge pull request #1097 from lionel-/fix-cast

Fix UBSAN error
r-lib · May 9, 2020 · 570a00c · 570a00c
2 parents 5b71d88 + a33d116
commit 570a00c
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 7 deletions.
diff --git a/cran-comments.md b/cran-comments.md
@@ -1,6 +1,5 @@
 
-* The UBSAN error is fixed.
-* The r-devel warning is fixed.
+This is a resubmission to fix the SAN error.
 
 
 ## Test environments

diff --git a/src/subscript.c b/src/subscript.c
@@ -165,17 +165,21 @@ static SEXP dbl_cast_subscript(SEXP subscript,
       continue;
     }
 
-    int out_elt = (int) elt;
-
-    // Detect out-of-bounds and fractional numbers
-    if (elt > INT_MAX || out_elt != elt) {
+    if (!isfinite(elt) || elt <= INT_MIN || elt > INT_MAX) {
       // Once we throw lazy errors from the cast method, we should
       // throw the error here as well
       UNPROTECT(1);
       return dbl_cast_subscript_fallback(subscript, opts, err);
     }
 
-    out_p[i] = out_elt;
+    int elt_int = (int) elt;
+
+    if (elt != elt_int) {
+      UNPROTECT(1);
+      return dbl_cast_subscript_fallback(subscript, opts, err);
+    }
+
+    out_p[i] = elt_int;
   }
 
   UNPROTECT(1);

diff --git a/tests/testthat/error/test-subscript-loc.txt b/tests/testthat/error/test-subscript-loc.txt
@@ -169,6 +169,14 @@ i It must be numeric or character.
 Error: Must extract element with a single valid subscript.
 x Can't convert from <double> to <integer> due to loss of precision.
 
+> vec_as_location2(Inf, 10L)
+Error: Must extract element with a single valid subscript.
+x Can't convert from <double> to <integer> due to loss of precision.
+
+> vec_as_location2(-Inf, 10L)
+Error: Must extract element with a single valid subscript.
+x Can't convert from <double> to <integer> due to loss of precision.
+
 > # Idem with custom `arg`
 > vec_as_location2(foobar(), 10L, arg = "foo")
 Error: Must extract element with a single valid subscript.

diff --git a/tests/testthat/test-subscript-loc.R b/tests/testthat/test-subscript-loc.R
@@ -11,6 +11,8 @@ test_that("vec_as_location2() requires integer or character inputs", {
     expect_error(vec_as_location2(env(), 10L), class = "vctrs_error_subscript_type")
     expect_error(vec_as_location2(foobar(), 10L), class = "vctrs_error_subscript_type")
     expect_error(vec_as_location2(2.5, 10L), class = "vctrs_error_subscript_type")
+    expect_error(vec_as_location2(Inf, 10L), class = "vctrs_error_subscript_type")
+    expect_error(vec_as_location2(-Inf, 10L), class = "vctrs_error_subscript_type")
 
     "Idem with custom `arg`"
     expect_error(vec_as_location2(foobar(), 10L, arg = "foo"), class = "vctrs_error_subscript_type")
@@ -481,6 +483,8 @@ test_that("conversion to locations has informative error messages", {
     vec_as_location2(env(), 10L)
     vec_as_location2(foobar(), 10L)
     vec_as_location2(2.5, 3L)
+    vec_as_location2(Inf, 10L)
+    vec_as_location2(-Inf, 10L)
     "Idem with custom `arg`"
     vec_as_location2(foobar(), 10L, arg = "foo")
     vec_as_location2(2.5, 3L, arg = "foo")