add template specializations to avoid DATAPTR use (#1310)

* add template specializations to avoid DATAPTR use

* also implement bounds checking in vector access

* tweak error message

* add tests; implement bounds check for proxy

* finish NEWS

* don't check index when constructing iterators

* avoid invoking wrong 'get'

* don't pass 'ncpus'

* finish NEWS

* test tweaks

* Small robustification for package version check during dev cycle

* use warning instead of stop

* update NEWS

* NEWS once more

* fixup for check_indices in subsetter

---------

Co-authored-by: Dirk Eddelbuettel <edd@debian.org>

Author: kevinushey

ChangeLog

@@ -1,3 +1,13 @@
+2024-06-11  Kevin Ushey  <kevinushey@gmail.com>
+
+	* inst/include/Rcpp/internal/r_vector.h: Use template specializations to
+	avoid DATAPTR usage
+	* inst/include/Rcpp/vector/traits.h: Implement bounds checks in
+	r_vector_cache access
+	* inst/tinytest/cpp/Vector.cpp: Add unit tests
+	* inst/tinytest/test_vector.R: Add unit tests
+	* tests/tinytest.R: Test in serial by default
+
 2024-06-02  Dirk Eddelbuettel  <edd@debian.org>
 
 	* inst/include/Rcpp/internal/export.h: More R_xlen_t switching

inst/NEWS.Rd

@@ -9,9 +9,14 @@
     \itemize{
       \item Set R_NO_REMAP if not already defined (Dirk in \ghpr{1296})
       \item Add variadic templates to be used instead of generated code
-      (Andrew Johnson in \ghpr{1303}
+      (Andrew Johnson in \ghpr{1303})
       \item Count variables were switches to \code{size_t} to avoid warnings
       about conversion-narrowing (Dirk in \ghpr{1307})
+      \item Rcpp now avoids the usage of the (non-API) DATAPTR function when
+      accessing the contents of Rcpp Vector objects where possible. (Kevin in
+      \ghpr{1310})
+      \item Rcpp now emits an R warning on out-of-bounds Vector accesses. This
+      may become an error in a future Rcpp release. (Kevin in \ghpr{1310})
     }
     \item Changes in Rcpp Deployment:
     \itemize{

inst/include/Rcpp/internal/r_vector.h

@@ -32,6 +32,21 @@ typename Rcpp::traits::storage_type<RTYPE>::type* r_vector_start(SEXP x) {
     return reinterpret_cast<pointer>(dataptr(x));
 }
 
+// add specializations to avoid use of dataptr
+#define RCPP_VECTOR_START_IMPL(__RTYPE__, __ACCESSOR__)                                              \
+    template <>                                                                                      \
+    inline typename Rcpp::traits::storage_type<__RTYPE__>::type* r_vector_start<__RTYPE__>(SEXP x) { \
+        return __ACCESSOR__(x);                                                                      \
+    }
+
+RCPP_VECTOR_START_IMPL(LGLSXP,  LOGICAL);
+RCPP_VECTOR_START_IMPL(INTSXP,  INTEGER);
+RCPP_VECTOR_START_IMPL(RAWSXP,  RAW);
+RCPP_VECTOR_START_IMPL(CPLXSXP, COMPLEX);
+RCPP_VECTOR_START_IMPL(REALSXP, REAL);
+
+#undef RCPP_VECTOR_START_IMPL
+
 /**
  * The value 0 statically casted to the appropriate type for
  * the given SEXP type

inst/include/Rcpp/vector/Subsetter.h

@@ -133,9 +133,9 @@ class SubsetProxy {
 
 private:
 
-    #ifndef RCPP_NO_BOUNDS_CHECK
     template <typename IDX>
     void check_indices(IDX* x, R_xlen_t n, R_xlen_t size) {
+#ifndef RCPP_NO_BOUNDS_CHECK
         for (IDX i=0; i < n; ++i) {
             if (x[i] < 0 or x[i] >= size) {
                 if(std::numeric_limits<IDX>::is_integer && size > std::numeric_limits<IDX>::max()) {
@@ -144,11 +144,8 @@ class SubsetProxy {
                 stop("index error");
             }
         }
+#endif
     }
-    #else
-    template <typename IDX>
-    void check_indices(IDX* x, IDX n, IDX size) {}
-    #endif
 
     void get_indices( traits::identity< traits::int2type<INTSXP> > t ) {
         indices.reserve(rhs_n);

inst/include/Rcpp/vector/traits.h

@@ -35,22 +35,36 @@ namespace traits{
 		typedef typename r_vector_const_proxy<RTYPE>::type const_proxy ;
 		typedef typename storage_type<RTYPE>::type storage_type ;
 
-		r_vector_cache() : start(0){} ;
+		r_vector_cache() : start(0), size(0) {} ;
+
 		inline void update( const VECTOR& v ) {
-		    start = ::Rcpp::internal::r_vector_start<RTYPE>(v) ;
+			start = ::Rcpp::internal::r_vector_start<RTYPE>(v) ;
+			size = v.size();
 		}
+
 		inline iterator get() const { return start; }
 		inline const_iterator get_const() const { return start; }
 
-		inline proxy ref() { return *start ;}
-		inline proxy ref(R_xlen_t i) { return start[i] ; }
+		inline proxy ref() { check_index(0); return start[0] ;}
+		inline proxy ref(R_xlen_t i) { check_index(i); return start[i] ; }
+
+		inline proxy ref() const { check_index(0); return start[0] ;}
+		inline proxy ref(R_xlen_t i) const { check_index(i); return start[i] ; }
 
-		inline proxy ref() const { return *start ;}
-		inline proxy ref(R_xlen_t i) const { return start[i] ; }
+	private:
 
-		private:
-			iterator start ;
+		void check_index(R_xlen_t i) const {
+#ifndef RCPP_NO_BOUNDS_CHECK
+			if (i >= size) {
+				warning("subscript out of bounds (index %s >= vector size %s)", i, size);
+			}
+#endif
+		}
+
+		iterator start ;
+		R_xlen_t size ;
 	} ;
+
 	template <int RTYPE, template <class> class StoragePolicy = PreserveStorage>
 	class proxy_cache{
 	public:
@@ -66,17 +80,24 @@ namespace traits{
 			p = const_cast<VECTOR*>(&v) ;
 		}
 		inline iterator get() const { return iterator( proxy(*p, 0 ) ) ;}
-		// inline const_iterator get_const() const { return const_iterator( *p ) ;}
 		inline const_iterator get_const() const { return const_iterator( const_proxy(*p, 0) ) ; }
 
-		inline proxy ref() { return proxy(*p,0) ; }
-		inline proxy ref(R_xlen_t i) { return proxy(*p,i);}
+		inline proxy ref() { check_index(0); return proxy(*p,0) ; }
+		inline proxy ref(R_xlen_t i) { check_index(i); return proxy(*p,i);}
 
-		inline const_proxy ref() const { return const_proxy(*p,0) ; }
-		inline const_proxy ref(R_xlen_t i) const { return const_proxy(*p,i);}
+		inline const_proxy ref() const { check_index(0); return const_proxy(*p,0) ; }
+		inline const_proxy ref(R_xlen_t i) const { check_index(i); return const_proxy(*p,i);}
 
 	private:
 		VECTOR* p ;
+
+		void check_index(R_xlen_t i) const {
+#ifndef RCPP_NO_BOUNDS_CHECK
+			if (i >= p->size()) {
+				warning("subscript out of bounds (index %s >= vector size %s)", i, p->size());
+			}
+#endif
+		}
 	} ;
 
 	// regular types for INTSXP, REALSXP, ...

inst/tinytest/cpp/Vector.cpp

@@ -886,3 +886,13 @@ bool CharacterVector_test_equality_crosspolicy(CharacterVector x, Vector<STRSXP,
 
     return std::equal(x.begin(), x.end(), y.begin());
 }
+
+// [[Rcpp::export]]
+double NumericVector_test_out_of_bounds_read(NumericVector v, R_xlen_t i) {
+    return v[i];
+}
+
+// [[Rcpp::export]]
+SEXP CharacterVector_test_out_of_bounds_read(CharacterVector v, R_xlen_t i) {
+    return v[i];
+}

inst/tinytest/test_packageversion.R

@@ -1,5 +1,5 @@
 
-##  Copyright (C) 2019 - 2022  Dirk Eddelbuettel
+##  Copyright (C) 2019 - 2024  Dirk Eddelbuettel
 ##
 ##  This file is part of Rcpp.
 ##
@@ -30,7 +30,7 @@ v <- as.integer(unlist(strsplit(pvstr, "\\.")))
 relstr <- as.character(as.package_version(paste(v[1:3], collapse=".")))
 
 ## call C++ function returning list of six values, three each for 'release' and 'dev' version
-res <- checkVersion(v)
+res <- checkVersion(v[1:min(4, length(v))])
 
 
 ## basic check: is the #defined version equal to the computed version (issue #1014)

inst/tinytest/test_rcpp_package_skeleton.R

@@ -69,7 +69,7 @@ for (file in grep("RcppExports.R", R_files, invert=TRUE, value=TRUE)) {
     code <- readLines(file)
     fn <- eval(parse(text=paste(code, collapse="\n")))
     fn_name <- gsub(".*/(.*)\\.R$", "\\1", file)
-    checkIdentical(fn, get(fn_name),
+    checkIdentical(fn, base::get(fn_name),
                    sprintf("we parsed the function '%s' correctly", fn_name)
                    )
 }

inst/tinytest/test_vector.R

@@ -694,3 +694,11 @@ expect_equal(data, data2)
 #    test.CharacterVector_test_equality <- function(){
 expect_true( !CharacterVector_test_equality("foo", "bar") )
 expect_true( !CharacterVector_test_equality_crosspolicy("foo", "bar") )
+
+# https://github.com/RcppCore/Rcpp/issues/1308
+# tests disabled since these could trigger UBSAN warnings / crashes
+#expect_warning(NumericVector_test_out_of_bounds_read(numeric(0), 0))
+#expect_warning(NumericVector_test_out_of_bounds_read(numeric(1), 1))
+#expect_warning(CharacterVector_test_out_of_bounds_read(character(0), 0))
+#expect_warning(CharacterVector_test_out_of_bounds_read(character(1), 1))
+

tests/tinytest.R

@@ -43,5 +43,5 @@ if (requireNamespace("tinytest", quietly=TRUE)) {
     ## there are several more granular ways to test files in a tinytest directory,
     ## see its package vignette; tests can also run once the package is installed
     ## using the same command `test_package(pkgName)`, or by director or file
-    tinytest::test_package("Rcpp", ncpu=getOption("Ncpus", 1))
+    tinytest::test_package("Rcpp")
 }