Memory leak in `fwrite`

[aitap]

Found while testing the fix for #6731.

Setting aside the leaks from zlib and fontconfig, we've got the following for tests/main.R:

Direct leak of 125829120 byte(s) in 15 object(s) allocated from:
    #1 0x7f43e5ea224a in fwriteMain src/fwrite.c:792
    #2 0x7f43e5ea7638 in fwriteR src/fwriteR.c:310

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

char *buffPool = malloc(alloc_size);

are not always matched with this deallocation:

data.table/src/fwrite.c

Line 1096 in 1aa92bc

free(buffPool);

(Some allocations done internally by zlib are also leaks, but LeakSanitizer doesn't trace them to a specific line in data.table.)

We've got quite a few calls to STOP() between the two lines, and most of those don't release the memory allocated for buffPool. This bisects to e0abdfc, i.e., #6393. Before this commit, tests/main.R finishes without any notes from LeakSanitizer.

Edit: the title used to say "leak in fread", which was not correct at all.

[tdhock]

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?

[aitap]

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 ending tests/main.R with 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 with ASAN_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.gz and git bisect good or git bisect bad depending on whether tests/main.R succeeded. (Due to the above-mentioned fontconfig leaks, overall R CMD check always 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 clang compiler 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-san also 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 also R CMD check --use-valgrind, which doesn't require a special build of R, only debugging symbols enabled, but is much slower.)

[MichaelChirico]

Thanks! I've assigned #6393 author for now, PTAL!

[aitap]

One 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).