rbindlist protection stack overflow #5448
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5448 +/- ##
==========================================
+ Coverage 97.51% 99.51% +1.99%
==========================================
Files 80 80
Lines 14979 14772 -207
==========================================
+ Hits 14607 14700 +93
+ Misses 372 72 -300 ☔ View full report in Codecov by Sentry. |
|
Thank you for fix. Adding code from issue that this PR closes won't fit for unit test for some reason? |
Didn't want to add that one since it takes over 1s and testing already takes some time. |
|
Generated via commit 0dd0c38 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 12 minutes and 8 seconds Time taken to run |
| } | ||
| // else coerces if needed within memrecycle; with a no-alloc direct coerce from 1.12.4 (PR #3909) | ||
| const char *ret = memrecycle(target, R_NilValue, ansloc, thisnrow, thisCol, 0, -1, idcol+j+1, foundName); | ||
| if (ret) warning(_("Column %d of item %d: %s"), w+1, i+1, ret); | ||
| if (listprotect) UNPROTECT(1); // earlier unprotect rbindlist calls with lots of lists #4536 |
There was a problem hiding this comment.
Should the UNPROTECT() precede the warning() for the case when options(warn=2)?
There was a problem hiding this comment.
True, but I guess we have a lot of cases where we either error or warn before calling UNPROTECT. Should we check with R-devel for the recommended handling of these cases?
There was a problem hiding this comment.
Yes, I think we're meant to always UNPROTECT() before signalling error() or warning(), e.g. from WRE:
#include <R.h>
#include <Rinternals.h>
#include <R_ext/Parse.h>
SEXP menu_ttest3()
{
char cmd[256];
SEXP cmdSexp, cmdexpr, ans = R_NilValue;
ParseStatus status;
...
if(done == 1) {
cmdSexp = PROTECT(allocVector(STRSXP, 1));
SET_STRING_ELT(cmdSexp, 0, mkChar(cmd));
cmdexpr = PROTECT(R_ParseVector(cmdSexp, -1, &status, R_NilValue));
if (status != PARSE_OK) {
UNPROTECT(2);
error("invalid call %s", cmd);
}
/* Loop is needed here as EXPSEXP will be of length > 1 */
for(int i = 0; i < length(cmdexpr); i++)
ans = eval(VECTOR_ELT(cmdexpr, i), R_GlobalEnv);
UNPROTECT(2);
}
return ans;
}IINM {rchk} is supposed to help ferret those out, so I'm not sure we have many violations for the case of error(), can you easily find any?
We might want to report to {rchk} maintainer T. Kalibera about possibly catching usages like warning(); UNPROTECT(); as well by that package.
cc @jangorecki as well who has worked more extensively here.
There was a problem hiding this comment.
I just took a glance at assign.c and between.c but both contain those "violations".
There was a problem hiding this comment.
ok let's file a follow-up
Closes #4536