Skip to content

Patch integer overflow in 'Rcpp::wrap(<Eigen::Matrix>)' #105

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.

Sign up for GitHub

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 2 commits into from
Jan 15, 2022
Merged

Patch integer overflow in 'Rcpp::wrap(<Eigen::Matrix>)' #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96d9e1c
Patch integer overflow in 'Rcpp::wrap(<Eigen::Matrix>)'
jaganmn Jan 15, 2022
a1f7dad
whitespace changes, added ChangeLog entry
eddelbuettel Jan 15, 2022
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
4 changes: 4 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
2022-01-15 Mikael Jagan <jaganmn@mcmaster.ca>

* inst/include/RcppEigenWrap.h: Use R_xlen_t for vectors rows + cols

2021-12-29 Dirk Eddelbuettel <edd@debian.org>

* README.md: Add total downloads badge
Expand Down
25 changes: 14 additions & 11 deletions inst/include/RcppEigenWrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// RcppEigenWrap.h: Rcpp wrap methods for Eigen matrices, vectors and arrays
//
// Copyright (C) 2011 - 2012 Douglas Bates, Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2011 - 2022 Douglas Bates, Dirk Eddelbuettel and Romain Francois
//
// This file is part of RcppEigen.
//
Expand Down Expand Up @@ -80,16 +80,19 @@ namespace Rcpp{

// for plain dense objects
template <typename T>
SEXP eigen_wrap_plain_dense( const T& obj, Rcpp::traits::true_type ){
typename Eigen::internal::conditional<T::IsRowMajor,
Eigen::Matrix<typename T::Scalar,
T::RowsAtCompileTime,
T::ColsAtCompileTime>,
const T&>::type objCopy(obj);
int m = obj.rows(), n = obj.cols();
R_xlen_t size = static_cast<R_xlen_t>(m) * n;
SEXP ans = PROTECT(::Rcpp::wrap(objCopy.data(), objCopy.data() + size));
if( T::ColsAtCompileTime != 1 ) {
SEXP eigen_wrap_plain_dense( const T& obj, Rcpp::traits::true_type ) {
typename Eigen::internal::conditional<
T::IsRowMajor,
Eigen::Matrix<typename T::Scalar,
T::RowsAtCompileTime,
T::ColsAtCompileTime>,
const T&>::type objCopy(obj);
R_xlen_t m = obj.rows(), n = obj.cols(), size = m * n;
SEXP ans = PROTECT(::Rcpp::wrap(objCopy.data(), objCopy.data() + size));
if ( T::ColsAtCompileTime != 1 ) {
if (m > INT_MAX || n > INT_MAX) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be immediately after the dimension definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... I've just committed again to my fork to address that, but it won't appear here until the PR is reopened. (I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tacking on: Is objCopy really a copy of obj? Why do we need objCopy.data() here, rather than obj.data()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it may needs a cleanup as the Rcpp:;stop() also breaks the protect/unprotect dance.

The const T& obj to <.... const T&>::type objCopy(obj) dance is likely due to some TMP magic we need.

I'll move the check up a line now.

Copy link
Contributor Author

@jaganmn jaganmn Jan 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my latest commit, unless you are suggesting something different?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no commits, this has been merged to the repo. See the purple 'Merged' button here.

Note that I also just committed so if you want to change relative to the repo you need to update your for by pulling the main repo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change seems perfectly fine - I often forget how GitHub works past midnight... Glad this was caught and fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh - I spoke too soon. See my PR #106.

Rcpp::stop("array dimensions cannot exceed INT_MAX");
}
SEXP dd = PROTECT(::Rf_allocVector(INTSXP, 2));
int *d = INTEGER(dd);
d[0] = m;
Expand Down