Fix ordertab() segfault when nunique is incorrectly supplied (r-lib#207)

* Fix: ordertab() segfaults when nunique is smaller than actual

The ordertab() C function was susceptible to a buffer overflow when the
'nunique' parameter passed from R was smaller than the actual number of
unique values in the data. This led to intermittent segfaults due to
out-of-bounds writes to the 'ret' array.

This commit introduces a bounds check in the C implementation of
ordertab() (r_ram_integer64_ordertab_asc). If the number of unique
values encountered exceeds the allocated size of the 'ret' vector,
the loop is now terminated early, preventing any out-of-bounds writes
and thus eliminating the segfault. The function will now return a
truncated result in such cases, consistent with R's usual handling
of mismatched lengths.

A new test case has been added to test-sortuse64.R to specifically
target this scenario, using gctorture() to reliably trigger the
condition. This test now passes, confirming the fix.

Updated NEWS.md to reflect this bug fix.

* To taste

* tighten test

* comment purpose

* citation

Author: MichaelChirico

NEWS.md

@@ -53,6 +53,7 @@
 1. `log(integer64(), base=integer64(1))` no longer warns, consistent with `log(integer(), base=integer())` (#93).
 1. `sortfin(integer64(), 1:10)` no longer segfaults (#164).
 1. `orderfin(as.integer64(10:1), 1:3, 8:11)` enforces that `table` be sorted by `order` instead of segfaulting (#166).
+1. `ordertab()` no longer segfaults when `nunique` is smaller than the actual number of unique values (#168).
 
 ## NOTES
 

src/sortuse64.c

@@ -775,17 +775,21 @@ SEXP r_ram_integer64_ordertab_asc(
               ret[pos++] = ret[i];
           SET_LENGTH(ret_, pos); /* re-allocates ret_ */
       }else{
+        int n_ret = LENGTH(ret_); // allow bailing if user mis-specified nunique (#168)
+        if (n_ret > 0) {
           j = 0;
           ret[j] = 1;
           pos = index[j]-1;
           for(i=1;i<n;i++){
             if (table[index[i]-1]!=table[pos]){
                 pos = index[i]-1;
+                if (j + 1 >= n_ret) break;
                 ret[++j] = 1;
             }else{
                 ret[j]++;
             }
           }
+        }
       }
       PROTECT(ret_); /* Thanks to Tomas Kalibera */
       R_Busy(0);

tests/testthat/test-sortuse64.R

@@ -65,3 +65,17 @@ test_that("ordertab and orderdup work", {
   expect_identical(orderdup(x, idx, method=2L), rep(c(FALSE, TRUE), c(10L, 14L)))
 })
 
+test_that("ordertab handles nunique smaller than actual", {
+  x = as.integer64(1:10)
+  x = c(x, x[1:8], x[1:6])
+  o = order(x)
+
+  # makes operation quite slow, but in my test, this segfaulted on the first invocation every time.
+  #   do this inside 'local' because doing so inside 'expect_identical' causes gctorture to apply
+  #   to _every_ allocation induced by testthat as well as rep(). This isolated form is much faster.
+  res = local({
+    on.exit(gctorture(FALSE)); gctorture(TRUE)
+    ordertab(x, o, 4L)
+  })
+  expect_identical(res, rep(3L, 4L))
+})