Potential segfault in forder

[dkutner]

I've been hitting an issue running dt[, .(a = mean(a)), keyby = .(b, c, d)] leading to an error INTEGER() can only be applied to a 'integer', not a 'NULL'. Unfortunately it's difficult to reproduce without sharing proprietary data and the error only happens intermittently, but I think I've traced it back to this line https://github.com/Rdatatable/data.table/blob/1.16.4/src/forder.c#L545. If I try to run Rf_PrintValue(ascArg); immediately before this line, I eventually hit a segfault. My knowledge of R internals is limited, but it may be related to unprotecting the value in line 469.

It's worth noting that I've only hit this issue when running my code in a foreach loop with doParallel, but this may be leading to increased garbage collection.

[MichaelChirico]

Thanks @dkutner. Are you able to reproduce the issue on current master?

If you're unable to install non-CRAN releases, you might wait a short while until 1.17.0 is released: #6616

[dkutner]

@MichaelChirico Yes, I just reproduced it using 1.16.99 in master.

[MichaelChirico]

Indeed this looks fishy to me:

data.table/src/forder.c

Lines 494 to 495 in 4f7f567

	ascArg = recycleAscArg;
	UNPROTECT(1); // recycleAscArg

Are you able to test if this patch fixes your issue?

-    SEXP recycleAscArg = PROTECT(allocVector(INTSXP, LENGTH(by)));
+    SEXP recycleAscArg = PROTECT(allocVector(INTSXP, LENGTH(by))); n_protect++;
    for (int j=0; j<LENGTH(recycleAscArg); j++)
      INTEGER(recycleAscArg)[j] = INTEGER(ascArg)[0];
    ascArg = recycleAscArg;
-    UNPROTECT(1); // recycleAscArg

[MichaelChirico]

That was introduced in #4386, so cc @jangorecki

[dkutner]

I'll try that and let you know. It happens very sporadically so assuming it's fixed, I'll have to run many times to be confident. I've struggled to come up with a reprex.

[aitap]

It might help to enable gctorture(TRUE) immediately before testing the problematic expression. The cost of increasing the chance of memory protection bugs to announce themselves is a very significant slowdown of R.

[dkutner]

Thanks @aitap. I had tried gctorture when attempting to make a reprex but still couldn't reproduce it more simply.

@MichaelChirico I haven't hit the issue after implementing your suggestion, so that likely fixes it.

[ben-schwen]

@dkutner does your problem also appear when you try do forderv(dt, by=c("b", "c", "d")) and could you at least provide us some info about the underlying data.table e.g. number of rows and number of columns?

[dkutner]

@ben-schwen I haven't hit an issue when running dt[, .(a = mean(a)), by = .(b, c, d)], if that answers your question.

The data.table has 6 columns and 4.86M rows. b takes on 1000 different values while c and d each take on 30 different values such that c != d, leading to a total of 870K different groups.

[jangorecki]

Indeed this looks fishy to me:

data.table/src/forder.c

Lines 494 to 495 in 4f7f567

ascArg = recycleAscArg;
UNPROTECT(1); // recycleAscArg
Are you able to test if this patch fixes your issue?

• SEXP recycleAscArg = PROTECT(allocVector(INTSXP, LENGTH(by)));

• SEXP recycleAscArg = PROTECT(allocVector(INTSXP, LENGTH(by))); n_protect++;
  for (int j=0; j<LENGTH(recycleAscArg); j++)
  INTEGER(recycleAscArg)[j] = INTEGER(ascArg)[0];
  ascArg = recycleAscArg;

• UNPROTECT(1); // recycleAscArg

Looks very relevant, unprotect of something that is needed later on. So segfault happens randomly depending on GC.