Avoid out-of-bounds access in `overlaps` by aitap · Pull Request #7598 · Rdatatable/data.table · GitHub

When y has no rows, the data pointers of the columns of unique(y[,...]) are poisoned. Instead of trying to dereference them, pretend that all comparisons fail and fill the return value with NAs.

When there are no matches on the non-range columns, the index in from[i] may become 0. So uncomment the (from[i]>0) ? from[i] : 1 checks instead of trusting from[i] and accessing VECTOR_ELT(lookup, -1). I am especially interested in someone double-checking this part because there are other places where from[i] is not checked for being > 0. Are they reachable with from[i] == 0?

Fixes: #7597

If 'ux' contains 0 rows, pretend that all comparisons against its non-existent elements fail.

This used to happen when from[i] was 0. (No match on non-range columns?)

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.01%. Comparing base (0216983) to head (eab8609).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/ijoin.c	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7598      +/-   ##
==========================================
+ Coverage   99.00%   99.01%   +0.01%     
==========================================
  Files          87       87              
  Lines       16893    16896       +3     
==========================================
+ Hits        16725    16730       +5     
+ Misses        168      166       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

HEAD=fix-7597 slower P<0.001 for melt improved in #5054

Generated via commit eab8609

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	3 minutes and 3 seconds
Installing different package versions	22 seconds
Running and plotting the test cases	4 minutes and 33 seconds

Is this ready to merge and be included in 1.18.2 or move to next patch?

I haven't found a chance to review -- @ben-schwen or @jangorecki

FWIW, insurancerating passes R CMD check with data.table from this branch, so merging it would help #7514 as well.

Technically this one was harmless (and thus not caught by sanitizers) because the preceding VECSEXP header always contained a 0, preventing the branch where VECTOR_ELT() would be called with a negative index.

there are other places where from[i] is not checked for being > 0.

Most of these had separate checks for either from[i] > 0 or k > 0. One remaining case, type = "any", mult = "first", really did underflow type_count[j-1], but then did not underflow VECTOR_ELT(type_lookup, j-1) in my testing because the former ended up being 0. This isn't caught by sanitizers because type_count is preceded in memory by the SEXP header and thus technically isn't a buffer underflow.

Suggested change

      
            for (int i=0; i<rows; ++i) {
          
              const int len = totlen;
          
              const int k = from[i];
          
              if (k > 0) {
          
                if (k == to[i]) {
          
                  totlen += count[k-1];
          
                } else if (k < to[i]) {
          
                  int *s = INTEGER(VECTOR_ELT(lookup, k-1));
          
                  int *e = INTEGER(VECTOR_ELT(type_lookup, to[i]-1));
          
                  int scount = count[k-1], ecount = type_count[to[i]-1];
          
                  for (int j=0, m=0; j < scount && m < ecount; ) {
          
                    if (s[j] == e[m]) { ++totlen; ++j; ++m; }
          
                    else if (s[j] > e[m]) ++m; 
          
                    }
          
                }
          
                if (len == totlen) ++totlen;
          
                }
          
            }

We are always so picky about moving INTEGER out of loops, but apparently here we did not care yet. Definitely not for this PR but should be followed-up!

Something coccinelle could flag?

The only line I'm worried about is the creation of the return array, when we skip now (see suggested test).

Besides that LGTM

@aitap feel free to merge once that is cleared

Is this ready to merge and be included in 1.18.2 or move to next patch?

If possible, I would favor this going into 1.18.2. Its a long standing bug of >10 years which only surfaced now when R > 4.5.0 introduced stricter out of bounds checks

Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com>

The underflow is covered by already existing tests.

* Add tests * overlaps: avoid accessing length-0 vectors in ux If 'ux' contains 0 rows, pretend that all comparisons against its non-existent elements fail. * overlaps: avoid 'lookup' list overflow This used to happen when from[i] was 0. (No match on non-range columns?) * NEWS entry * overlaps: uncomment one more underflow test Technically this one was harmless (and thus not caught by sanitizers) because the preceding VECSEXP header always contained a 0, preventing the branch where VECTOR_ELT() would be called with a negative index. * test formatting * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update inst/tests/tests.Rraw * overlaps: uncomment the remaining underflow test The underflow is covered by already existing tests. --------- Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com>

@TysonStanley I cherry picked this into 1.18.2

I guess we are good to go to submit

@TysonStanley I cherry picked this into 1.18.2

I guess we are good to go to submit

@ben-schwen I'm waiting on #7607 and possibly #7606 since the PRs are there, just waiting for it to be merged. Unless we want to push those to 1.18.4 instead.

* 1.18.0 on CRAN. Bump to 1.18.99 * bump versions * Fix milestone link for 1.18.2 * Replace `ATTRIB`, `SET_ATTRIB` (#7487) * frev: drop SET_ATTRIB Instead, backport and use CLEAR_ATTRIB (R >= 4.5). * mergeIndexAttrib: drop SET_ATTRIB Use SHALLOW_DUPLICATE_ATTRIB (R >= 3.3) for the simple case. Also, Backport ANY_ATTRIB (R >= 4.5) instead of testing !isNull(ATTRIB(.)). * cbindlist: use ANY_ATTRIB * nafillR: use ANY_ATTRIB * Backport R_mapAttrib * anySpecialStatic: switch to R_mapAttrib * dogroups: construct rownames anew Instead of trying to walk ATTRIB in search of the compact 'rownames' attribute to modify, install it anew, take note of the returned reference to the value being installed (a different one!) and modify that. * mergeIndexAttrib: switch to R_mapAttrib * assign: factor out index fixup Instead of walking the attribute list directly, use R_mapAttrib(). Create a hash table of index names instead of relying on chin() and a temporary string vector. Move all temporary allocations onto the R heap. * assign: drop indexLength * assign: fix index unmarking * Comments, better field names * Update src/dogroups.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * mapAttrib: protect the attribute value Otherwise the callback could remove the attribute and end up with the value unprotected. Protect the attribute tag as well for uniformity. Co-Authored-By: HughParsonage <hugh.parsonage@gmail.com> * dogroups: look up rownames using mapAttrib This solution is closer to the working approach previously taken by the code. * Fix comment, function name * Protect the newly found rownames attribute * add NEWS entry --------- Co-authored-by: HughParsonage <hugh.parsonage@gmail.com> Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> Co-authored-by: Michael Chirico <chiricom@google.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> * use getVar over findVar (#7575) * remove unused vars (#7578) * Fix code blocks in NEWS.md (#7518) Add a missing triple-backtick separator. Separate the indented code blocks from the preceding paragraph because otherwise Pandoc fails to realise that the whitespace followed by triple-backtick denotes a fenced code block. * make rchk happy (#7520) Co-authored-by: Michael Chirico <chiricom@google.com> * escape one frollsd tests for valgrind (#7548) * escape one tests for valgrind * increment skipped count * escape proper one * make test robust to valgrind numerical issues --------- Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> * fix(7571): bug fix for narm issue on gforce in int64 case (#7572) * fix(7571): bug fix for narm issue on gforce in int64 case * fix(7571): test sequencing * fix(7571): updated the NEWS.md * trailing newline * Use $V1 * fix(7571): added db optimize 2L * refine NEWS * fix(7571): add more tests and change to code similar to int for gsum * fix(7571): added more tests for mean * eliminate intermediate variable * NEWS again --------- Co-authored-by: Michael Chirico <chiricom@google.com> * add @manmita (#7573) * `utils.c`: include `<signal.h>` for `siginfo_t` (#7517) * utils.c: include <signal.h> for siginfo_t POSIX says: > The <signal.h> header shall define the siginfo_t type as a structure So <sys/wait.h> is not enough to see the definition (not just a forward declaration) of siginfo_t. * NEWS entry * Amend NEWS * more robustly define _POSIX_C_SOURCE (h/t Hugh) * tidy up NEWS * -D_POSIX_C_SOURCE=200809L in gitlab CI job for regression test * revert gitlab-ci change --------- Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Michael Chirico <chiricom@google.com> * Remove unrelated release notes Amend 1c05f11: remove the notes not relevant to the 1.18.2 release. * Fix http -> https link that is now a redirect (#7588) Found the following (possibly) invalid URLs: URL: http://stereopsis.com/radix.html (moved to https://stereopsis.com/radix.html) From: man/setkey.Rd man/setorder.Rd Status: 200 Message: OK * Fix latest rchk issues (#7585) * attempt PROTECT for new rchk issues * different approach for longestLevels * use nprotect? * no longer using names SEXP * only UNPROTECT near exit * no, that cant be it... * move assignment into loop * REPROTECT approach * reduce diff * reduce diff * set automatically allocates new column slots if needed (#7538) * set automatically allocates new column slots if needed * use GetOption1 instead of GetOption * fix test * change froll test * remove assign change * add output statements to test loop * add helper function * frollmedianFast: avoid reading uninitialised array (#7589) The 'n' array is only initialised if 'even' is true, so skip the comparisons otherwise. Detected by checking with --use-valgrind or performing a frollmedian() with an odd window size under R -d valgrind. Fixes: #7546 * `setlevels()`: avoid crash on missing factor values (#7596) Check for missing or out of bounds values and set them to NA. * Avoid out-of-bounds access in `overlaps` (#7598) * Add tests * overlaps: avoid accessing length-0 vectors in ux If 'ux' contains 0 rows, pretend that all comparisons against its non-existent elements fail. * overlaps: avoid 'lookup' list overflow This used to happen when from[i] was 0. (No match on non-range columns?) * NEWS entry * overlaps: uncomment one more underflow test Technically this one was harmless (and thus not caught by sanitizers) because the preceding VECSEXP header always contained a 0, preventing the branch where VECTOR_ELT() would be called with a negative index. * test formatting * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update src/ijoin.c Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Update inst/tests/tests.Rraw * overlaps: uncomment the remaining underflow test The underflow is covered by already existing tests. --------- Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * Only export R_init_data_table (#7607) This will avoid name clashes between data.table functions (now hidden) and other functions in the global namespace visible to the shared library loader. Fixes: #7605 * `set()`: only reallocate the table if resizing would fail otherwise (#7606) * Regression tests * set(): only reallocate if resizing would fail * Update R/data.table.R Co-authored-by: Michael Chirico <chiricom@google.com> * Rename test variables Co-Authored-By: Michael Chirico <michaelchirico4@gmail.com> * Cache j %chin% names(x) Co-Authored-By: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com --------- Co-authored-by: Michael Chirico <chiricom@google.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> * NEWS entry for #7607 (#7608) * NEWS entry for #7607 * More about the problem being solved Co-Authored-By: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com> * `tests/froll.R`: disable `mcparallel` under Valgrind (#7621) * update NEWS * fix merge --------- Co-authored-by: Tyson Barrett <t.barrett88@gmail.com> Co-authored-by: aitap <krylov.r00t@gmail.com> Co-authored-by: HughParsonage <hugh.parsonage@gmail.com> Co-authored-by: Michael Chirico <chiricom@google.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Jan Gorecki <J.Gorecki@wit.edu.pl> Co-authored-by: Manmita Das <34617961+manmita@users.noreply.github.com>

aitap added 4 commits January 15, 2026 14:56

Add tests

327849e

overlaps: avoid accessing length-0 vectors in ux

e2b19ea

If 'ux' contains 0 rows, pretend that all comparisons against its non-existent elements fail.

overlaps: avoid 'lookup' list overflow

354c3db

This used to happen when from[i] was 0. (No match on non-range columns?)

NEWS entry

9688a35

aitap requested a review from MichaelChirico as a code owner January 15, 2026 12:28

aitap added this to the 1.18.2 milestone Jan 15, 2026

aitap added 2 commits January 18, 2026 22:37

overlaps: uncomment one more underflow test

bc73eba

Technically this one was harmless (and thus not caught by sanitizers) because the preceding VECSEXP header always contained a 0, preventing the branch where VECTOR_ELT() would be called with a negative index.

test formatting

d113939