Skip to content

add template specializations to avoid DATAPTR use #1310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jun 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
2024-06-11 Kevin Ushey <[email protected]>

* 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 <[email protected]>

* inst/include/Rcpp/internal/export.h: More R_xlen_t switching
Expand Down
7 changes: 6 additions & 1 deletion inst/NEWS.Rd
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
15 changes: 15 additions & 0 deletions inst/include/Rcpp/internal/r_vector.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 2 additions & 5 deletions inst/include/Rcpp/vector/Subsetter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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);
Expand Down
47 changes: 34 additions & 13 deletions inst/include/Rcpp/vector/traits.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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, ...
Expand Down
10 changes: 10 additions & 0 deletions inst/tinytest/cpp/Vector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
4 changes: 2 additions & 2 deletions inst/tinytest/test_packageversion.R
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

## Copyright (C) 2019 - 2022 Dirk Eddelbuettel
## Copyright (C) 2019 - 2024 Dirk Eddelbuettel
##
## This file is part of Rcpp.
##
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion inst/tinytest/test_rcpp_package_skeleton.R
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
}
Expand Down
8 changes: 8 additions & 0 deletions inst/tinytest/test_vector.R
Original file line number Diff line number Diff line change
Expand Up @@ -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))

2 changes: 1 addition & 1 deletion tests/tinytest.R
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Loading