From 8f37d4d4b5e1e401f2bf9383caba3f2dd1ccd0d9 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 2 Jan 2026 22:27:42 -0800 Subject: [PATCH 1/5] 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. --- NEWS.md | 1 + src/sortuse64.c | 9 ++++++++- tests/testthat/test-sortuse64.R | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 3e5c327e..06f895fd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,6 +47,7 @@ ## BUG FIXES +1. `ordertab()` no longer segfaults when `nunique` is smaller than the actual number of unique values (#BUG_REPORT_1). 1. `min.integer64`, `max.integer64` and `range.integer64` now support `na.rm=TRUE` correctly when combining across mutliple inputs like `min(x, NA_integer64_, na.rm=TRUE)` (#142). 1. `as.integer64.integer64` is consistent with `as.integer.integer` in terms or returning a plain integer64 vector (i.e., stripped of attributes; #188). Thanks @hcirellu. 1. `log(integer64(), base=integer64(1))` no longer warns, consistent with `log(integer(), base=integer())` (#93). diff --git a/src/sortuse64.c b/src/sortuse64.c index 0a1bc56c..1ccfe16d 100644 --- a/src/sortuse64.c +++ b/src/sortuse64.c @@ -775,17 +775,24 @@ SEXP r_ram_integer64_ordertab_asc( ret[pos++] = ret[i]; SET_LENGTH(ret_, pos); /* re-allocates ret_ */ }else{ + int n_ret = LENGTH(ret_); + if (n_ret > 0) { j = 0; ret[j] = 1; pos = index[j]-1; for(i=1;i= n_ret) { + break; + } + j++; + ret[j] = 1; }else{ ret[j]++; } } + } } PROTECT(ret_); /* Thanks to Tomas Kalibera */ R_Busy(0); diff --git a/tests/testthat/test-sortuse64.R b/tests/testthat/test-sortuse64.R index 5240b333..1285d09a 100644 --- a/tests/testthat/test-sortuse64.R +++ b/tests/testthat/test-sortuse64.R @@ -65,3 +65,18 @@ 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) + # this should not segfault, even though nunique is wrong + gctorture(TRUE) + for (ii in 1:10) { + # with the fix, this will not crash, but return a truncated result + # without the fix, this is likely to crash + res <- ordertab(x, o, 4) + } + gctorture(FALSE) + # The result is truncated, but we can check it's not a crash and has the expected length + expect_length(res, 4) +}) \ No newline at end of file From 62c8baee9194af7cf35a48cb9500e985bff8ce88 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 2 Jan 2026 22:32:12 -0800 Subject: [PATCH 2/5] To taste --- NEWS.md | 2 +- src/sortuse64.c | 7 ++----- tests/testthat/test-sortuse64.R | 8 +++----- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/NEWS.md b/NEWS.md index 06f895fd..6b728c8b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,12 +47,12 @@ ## BUG FIXES -1. `ordertab()` no longer segfaults when `nunique` is smaller than the actual number of unique values (#BUG_REPORT_1). 1. `min.integer64`, `max.integer64` and `range.integer64` now support `na.rm=TRUE` correctly when combining across mutliple inputs like `min(x, NA_integer64_, na.rm=TRUE)` (#142). 1. `as.integer64.integer64` is consistent with `as.integer.integer` in terms or returning a plain integer64 vector (i.e., stripped of attributes; #188). Thanks @hcirellu. 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 diff --git a/src/sortuse64.c b/src/sortuse64.c index 1ccfe16d..cd739ec1 100644 --- a/src/sortuse64.c +++ b/src/sortuse64.c @@ -783,11 +783,8 @@ SEXP r_ram_integer64_ordertab_asc( for(i=1;i= n_ret) { - break; - } - j++; - ret[j] = 1; + if (j + 1 >= n_ret) break; + ret[++j] = 1; }else{ ret[j]++; } diff --git a/tests/testthat/test-sortuse64.R b/tests/testthat/test-sortuse64.R index 1285d09a..61cc9690 100644 --- a/tests/testthat/test-sortuse64.R +++ b/tests/testthat/test-sortuse64.R @@ -70,13 +70,11 @@ test_that("ordertab handles nunique smaller than actual", { x = c(x, x[1:8], x[1:6]) o = order(x) # this should not segfault, even though nunique is wrong - gctorture(TRUE) for (ii in 1:10) { # with the fix, this will not crash, but return a truncated result # without the fix, this is likely to crash - res <- ordertab(x, o, 4) + res <- ordertab(x, o, 4L) } - gctorture(FALSE) # The result is truncated, but we can check it's not a crash and has the expected length - expect_length(res, 4) -}) \ No newline at end of file + expect_length(res, 4L) +}) From 97e861ffb1df3a8428cd8bd48b93f2a266e2e2ff Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 2 Jan 2026 22:49:12 -0800 Subject: [PATCH 3/5] tighten test --- tests/testthat/test-sortuse64.R | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/testthat/test-sortuse64.R b/tests/testthat/test-sortuse64.R index 61cc9690..f7f1c615 100644 --- a/tests/testthat/test-sortuse64.R +++ b/tests/testthat/test-sortuse64.R @@ -69,12 +69,13 @@ 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) - # this should not segfault, even though nunique is wrong - for (ii in 1:10) { - # with the fix, this will not crash, but return a truncated result - # without the fix, this is likely to crash - res <- ordertab(x, o, 4L) - } - # The result is truncated, but we can check it's not a crash and has the expected length - expect_length(res, 4L) + + # 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)) }) From 950cd0253d00dff86b68db0205af5fb2b64d7026 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 2 Jan 2026 22:52:13 -0800 Subject: [PATCH 4/5] comment purpose --- src/sortuse64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sortuse64.c b/src/sortuse64.c index cd739ec1..d15db239 100644 --- a/src/sortuse64.c +++ b/src/sortuse64.c @@ -775,7 +775,7 @@ SEXP r_ram_integer64_ordertab_asc( ret[pos++] = ret[i]; SET_LENGTH(ret_, pos); /* re-allocates ret_ */ }else{ - int n_ret = LENGTH(ret_); + int n_ret = LENGTH(ret_); // allow bailing if user mis-specified nunique if (n_ret > 0) { j = 0; ret[j] = 1; From 1b04d67249ccca400104d5c3ca1da129ffc28bce Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 2 Jan 2026 22:52:38 -0800 Subject: [PATCH 5/5] citation --- src/sortuse64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sortuse64.c b/src/sortuse64.c index d15db239..cbac9bb0 100644 --- a/src/sortuse64.c +++ b/src/sortuse64.c @@ -775,7 +775,7 @@ 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 + int n_ret = LENGTH(ret_); // allow bailing if user mis-specified nunique (#168) if (n_ret > 0) { j = 0; ret[j] = 1;