Add protection against integer overflows in sugar functions

[Enchufa2]

Closes #1454

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[kevinushey]

Do these handle NAs? Or would these only ever be invoked for the "not NA" cases?

[Enchufa2]

In principle, sugar functions already handle NAs, so these will be invoked for non-NA cases. But I haven't triaged all the cases yet, so we'll see. If this implementation looks good so far, I'll continue with that.

[Enchufa2]

BTW @eddelbuettel I don't see the sugar directory listed in Codecov. Isn't the covr workflow running all the tests?

[eddelbuettel]

I don't think we exclude Sugar from tests or coverage. There is a specific entry point test_sugar.R with matching cpp file.

[Enchufa2]

Yes, and it requires RunAllRcppTests set to yes, so I was wondering if the covr task is not setting it?

[eddelbuettel]

BTW I was 'this month old' when I heard about the boo-boo with min here. We need to be careful.

> Rcpp::evalCpp("std::numeric_limits<int>::min()") # normal
[1] NA
> Rcpp::evalCpp("std::numeric_limits<int>::max()") # normal
[1] 2147483647
> Rcpp::evalCpp("std::numeric_limits<int>::min() * 1.0") # also normal
[1] -2147483648
> Rcpp::evalCpp("std::numeric_limits<int>::min()") # R special
[1] NA
> 
> 
> Rcpp::evalCpp("std::numeric_limits<double>::min()") # WTF: positive number
[1] 2.22507e-308
> Rcpp::evalCpp("std::numeric_limits<double>::lowest()") # normal
[1] -1.79769e+308
> 
> Rcpp::evalCpp("std::numeric_limits<int>::lowest()") 
[1] NA
> Rcpp::evalCpp("std::numeric_limits<int>::lowest() * 1.0") 
[1] -2147483648
> 

Mind you we are not running overflow for double, are we? If it is only for int aka int32_t it may be less complex than I feared.

[eddelbuettel]

covr runs R CMD check which goes via tests/tinytest.R as usual so it should. I recently added another clause there because we had to break mold and upload 'four part' fix up versions like 1.1.0.8 to CRAN. Maybe I borked the logic / started listening to the wrong environment variable. I will debug in a side thread.

[Enchufa2]

Mind you we are not running overflow for double, are we?

No, we are not. The overflow checks are enabled only for integral types. Because doubles do not overflow, they become Inf.

[eddelbuettel]

only for integral types

So int as we don't really do addition on character or bool. And given that Sugar is on SEXP we do not worry about, say, uint8_t do we?

[eddelbuettel]

I validated the assumptions in tests/tinytest.R in another debug repo I use for CI, and found no issue. So if coverage really does not run for Sugar then I do not know why. There is one exclusion line in codecov.yml but I do not think it should bite. Hm.

[Enchufa2]

So int as we don't really do addition on character or bool. And given that Sugar is on SEXP we do not worry about, say, uint8_t do we?

Correct.

[Enchufa2]

I validated the assumptions in tests/tinytest.R in another debug repo I use for CI, and found no issue. So if coverage really does not run for Sugar then I do not know why. There is one exclusion line in codecov.yml but I do not think it should bite. Hm.

I think that the coverage we see is just what calls into Rcpp.so, and nothing else. Because all tests compile cpp code via sourceCpp into a temporary directory, and this doesn't even get the required -O0 --coverage flags. So the current percentage is very misleading, and fixing this is far from trivial: we would need to compile all cpp code in tests in the package tree with the coverage flags, and then instruct covr to include additional paths to look for gcov files (because I think it only looks into the src folder by default).

But anyway, this is another matter. We can discuss this further in another issue.

[eddelbuettel]

Good thinking. Sounds like a testing / coverage package having all the tests in src/ may help. If we care.

I never know what to of coverage. It's a metric, and as Goodhart told us decades ago, of course it is being gamed.

[Copilot]

Pull request overview

This PR adds protection against integer overflows in Rcpp sugar functions by implementing safe arithmetic operations for addition, subtraction, and multiplication.

Changes:

• Introduces new safe_math.h header with safe arithmetic macros that detect and prevent integer overflows
• Updates sugar operators and functions (plus, minus, times, sum, cumsum, cumprod, diff, rowSums, etc.) to use the safe operations
• Adds comprehensive tests to verify overflow detection in various contexts

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
inst/include/Rcpp/sugar/tools/safe_math.h	Implements safe arithmetic operations with overflow detection using compiler builtins or fallback logic
inst/include/Rcpp/sugar/sugar.h	Includes the new safe_math.h header
inst/include/Rcpp/sugar/operators/plus.h	Replaces direct addition with RCPP_SAFE_ADD macro
inst/include/Rcpp/sugar/operators/minus.h	Replaces direct subtraction with RCPP_SAFE_SUB macro
inst/include/Rcpp/sugar/operators/times.h	Replaces direct multiplication with RCPP_SAFE_MUL macro
inst/include/Rcpp/sugar/functions/sum.h	Uses safe addition for sum operations
inst/include/Rcpp/sugar/functions/cumsum.h	Uses safe addition for cumulative sum
inst/include/Rcpp/sugar/functions/cumprod.h	Uses safe multiplication for cumulative product
inst/include/Rcpp/sugar/functions/diff.h	Uses safe subtraction for diff operations
inst/include/Rcpp/sugar/functions/rowSums.h	Uses safe addition and refactors NA handling logic
inst/tinytest/test_sugar.R	Adds overflow detection tests for all modified operations
inst/tinytest/cpp/sugar.cpp	Adds test functions for overflow scenarios
inst/tinytest/cpp/sugar_safe_math.cpp	Tests safe math operations with builtin support
inst/tinytest/cpp/sugar_safe_math_fallback.cpp	Tests safe math operations with fallback implementation
ChangeLog	Documents the changes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[eddelbuettel]

This looks really really solid. Excellent stuff.

PS Rev.deps kicked off (after ensuring machine is Debian and CRAN current, which always takes a moment on old metal like that).

[kevinushey]

Here's what Claude has to say:

  1. Pre-existing bug preserved in diff.h (NA=false specialization)

  inst/include/Rcpp/sugar/functions/diff.h:105-112 (current master):

  STORAGE diff = RCPP_SAFE_SUB(y, previous);  // computes result
  previous = y ;                               // overwrites previous
  previous_index = i+1 ;
  return RCPP_SAFE_SUB(y, previous);           // returns y - y == 0

  The local diff is computed but never returned. After previous = y, the return re-computes y - previous which is
  always 0. This is a pre-existing bug in Diff<RTYPE,false,LHS_T> (integer, no-NA path). The fix is trivial: return
  diff; instead of the second subtraction. Since this PR is touching these exact lines, it would be good to fix it
  here.

  2. Three NA=false scalar-vector operator specializations not converted

  These generic-RTYPE (i.e. used for INTSXP) classes still use raw arithmetic:

  - plus.h ~line 311: Plus_Vector_Primitive<RTYPE,false,T>::operator[] -- rhs + lhs[i] not converted to RCPP_SAFE_ADD
  - times.h ~line 294: Times_Vector_Primitive<RTYPE,false,T>::operator[] -- rhs * lhs[i] not converted to
  RCPP_SAFE_MUL
  - minus.h ~line 380: Minus_Primitive_Vector<RTYPE,false,T>::operator[] -- lhs - rhs[i] not converted to
  RCPP_SAFE_SUB

  These have separate REALSXP specializations that handle double, so the generic template does get instantiated for
  INTSXP. Integer overflow is possible here regardless of the NA=false guarantee.

  3. Bonus bug fix in minus.h (good catch)

  The PR fixes Minus_Primitive_Vector<RTYPE,false,T> from Extractor<REALSXP,false,T> to Extractor<RTYPE,false,T>.
  This is a genuine pre-existing bug -- the wrong Extractor type was being used when RTYPE != REALSXP. Worth calling
  out in the PR description / ChangeLog.

[Enchufa2]

Here's what Claude has to say:

1. Pre-existing bug preserved in diff.h (NA=false specialization)

inst/include/Rcpp/sugar/functions/diff.h:105-112 (current master):

STORAGE diff = RCPP_SAFE_SUB(y, previous); // computes result
previous = y ; // overwrites previous
previous_index = i+1 ;
return RCPP_SAFE_SUB(y, previous); // returns y - y == 0

The local diff is computed but never returned. After previous = y, the return re-computes y - previous which is
always 0. This is a pre-existing bug in Diff<RTYPE,false,LHS_T> (integer, no-NA path). The fix is trivial: return
diff; instead of the second subtraction. Since this PR is touching these exact lines, it would be good to fix it
here.

Good catch, will fix.

2. Three NA=false scalar-vector operator specializations not converted

These generic-RTYPE (i.e. used for INTSXP) classes still use raw arithmetic:

• plus.h ~line 311: Plus_Vector_Primitive<RTYPE,false,T>::operator[] -- rhs + lhs[i] not converted to RCPP_SAFE_ADD
• times.h ~line 294: Times_Vector_Primitive<RTYPE,false,T>::operator[] -- rhs * lhs[i] not converted to
  RCPP_SAFE_MUL
• minus.h ~line 380: Minus_Primitive_Vector<RTYPE,false,T>::operator[] -- lhs - rhs[i] not converted to
  RCPP_SAFE_SUB

These have separate REALSXP specializations that handle double, so the generic template does get instantiated for
INTSXP. Integer overflow is possible here regardless of the NA=false guarantee.

Not sure about that. This code is a bit of a mess, and I would lie if I say I completely understand it, but empirically all cases are covered by tests (vector-vector, primitive-vector, vector-primitive), and these are instantiated for sequences. If I add the safe functions here, the sequence tests don't compile.

3. Bonus bug fix in minus.h (good catch)

The PR fixes Minus_Primitive_Vector<RTYPE,false,T> from Extractor<REALSXP,false,T> to Extractor<RTYPE,false,T>.
This is a genuine pre-existing bug -- the wrong Extractor type was being used when RTYPE != REALSXP. Worth calling
out in the PR description / ChangeLog.

Yeap, fixed that, forgot to mention it. Will do.