Repository navigation
Memory leak in fwrite #6733
Description
Activity
wow this is great detective work, thanks!
how did you realize there may be a leak?
what code did you use to do the leak check and bisect?
do you think it may be possible to adapt the code to run on GitHub actions?
and maybe for functions other than fwrite?I was testing #6731 on a sanitized build of R. I needed UBSan (
-fsanitize=undefined) to see the integer overflow, but I also had ASan (-fsanitize=address) enabled. Nowadays ASan includes LeakSanitizer, which alerted me to the leak by endingtests/main.Rwith a non-zero exit code.Normally it's more than a bit of a pain due to either false positives or deliberate constant-sized leaks in various dependencies of R, such as
fontconfig, which is why Prof. Brian D. Ripley and Prof. Kurt Hornik run their additional CRAN checks withASAN_OPTIONS=detect_leaks=0. Here we only have one package to check, so there's hope that the false positives will eventually stop.Bisection had to start with a recent commit (the #6602 fix): the previous versions fail on R-devel. After that I ran
R CMD build . && $R_devel_sanitized CMD check data.table_1.6.99.tar.gzandgit bisect goodorgit bisect baddepending on whethertests/main.Rsucceeded. (Due to the above-mentionedfontconfigleaks, overallR CMD checkalways fails because of plots in vignettes. This can probably be worked around using a suppression file.)There are ready-made R-hub v2 actions including the one with the
clangcompiler with sanitizers enabled, but no leak checking both to avoid false positives and to be closer to the CRAN setup (these seem to require an R-hub token, why?).rocker/r-devel-sanalso disables LeakChecker by default, for the same reasons (edit: they seem to have disabled ASan altogether years ago). Definitely worth experimenting, we can likely set the environment variable back on. (There's alsoR CMD check --use-valgrind, which doesn't require a special build of R, only debugging symbols enabled, but is much slower.)Reacted by Ani and Toby Dylan HockingThanks! I've assigned #6393 author for now, PTAL!
Reacted by aitapOne of the issues is easy to fix:
diff --git a/src/fwrite.c b/src/fwrite.c index 547cbf64..66b4573c 100644 --- a/src/fwrite.c +++ b/src/fwrite.c @@ -949,10 +949,11 @@ void fwriteMain(fwriteMainArgs args) #ifndef NOZLIB size_t mylen = 0; int mycrc = 0; - z_stream *mystream = &thread_streams[me]; + z_stream *mystream = NULL; void *myzBuff = NULL; size_t myzbuffUsed = 0; if (args.is_gzip) { + mystream = &thread_streams[me]; myzBuff = zbuffPool + me * zbuffSize; if (init_stream(mystream) != Z_OK) { // this should be thread safe according to zlib documentation failed = true; // # nocov
It doesn't cause any problems on typical CPUs, but pointer arithmetic on a null pointer (when
!args.is_gzip) is a formal violation of the C standard, so UBSan flags it (including on CRAN special checks).
Found while testing the fix for #6731.
Setting aside the leaks from
zlibandfontconfig, we've got the following fortests/main.R:plus a few tens of megabytes from other objects allocated in the same place.
Looks like this allocation:
data.table/src/fwrite.c
Line 792 in 1aa92bc
are not always matched with this deallocation:
data.table/src/fwrite.c
Line 1096 in 1aa92bc
(Some allocations done internally by
zlibare also leaks, but LeakSanitizer doesn't trace them to a specific line indata.table.)We've got quite a few calls to
STOP()between the two lines, and most of those don't release the memory allocated forbuffPool. This bisects to e0abdfc, i.e., #6393. Before this commit,tests/main.Rfinishes without any notes from LeakSanitizer.Edit: the title used to say "leak in
fread", which was not correct at all.