From bill at insightful.com  Thu Jul 10 22:17:03 2008
From: bill at insightful.com (Bill Dunlap)
Date: Thu, 10 Jul 2008 13:17:03 -0700 (PDT)
Subject: [Rd] memory leak in readline code
Message-ID: <Pine.GSO.4.56.0807101311230.21146@devsun3201.insightful.com>

Several folks have previously written that valgrind notices
a memory leak in R's readline code.  It looks like it leaks
a copy of every input line.

% ~/R-svn/r-devel/R/bin/R --debugger=valgrind --debugger-args=--leak-check=full --vanilla
==10725== Memcheck, a memory error detector.
==10725== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al.
==10725== Using LibVEX rev 1658, a library for dynamic binary translation.
==10725== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP.
==10725== Using valgrind-3.2.1, a dynamic binary instrumentation framework.
==10725== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al.
==10725== For more details, rerun with: -v
==10725==

R version 2.8.0 Under development (unstable) (2008-07-07 r46046)
Type 'q()' to quit R.

> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> invisible("hello")
> q()

==10743==
==10743== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 40 from 2)
==10743== malloc/free: in use at exit: 12,591,699 bytes in 5,927 blocks.
==10743== malloc/free: 21,015 allocs, 15,088 frees, 27,744,803 bytes allocated.
==10743== For counts of detected errors, rerun with: -v
==10743== searching for pointers to 5,927 not-freed blocks.
==10743== checked 12,612,676 bytes.
==10743==
==10743== 234 bytes in 13 blocks are definitely lost in loss record 20 of 42
==10743==    at 0x40046EE: malloc (vg_replace_malloc.c:149)
==10743==    by 0x68BFF9: xmalloc (in /usr/lib/libreadline.so.4.3)
==10743==    by 0x6770D5: readline_internal_teardown (in /usr/lib/libreadline.so.4.3)
==10743==    by 0x688992: rl_callback_read_char (in /usr/lib/libreadline.so.4.3)
==10743==    by 0x80E739C: Rstd_ReadConsole (sys-std.c:905)
==10743==    by 0x8057F61: Rf_ReplIteration (main.c:205)
==10743==    by 0x805827E: R_ReplConsole (main.c:306)
==10743==    by 0x8058514: run_Rmainloop (main.c:966)
==10743==    by 0x805676D: main (Rmain.c:33)
==10743==
==10743== LEAK SUMMARY:
==10743==    definitely lost: 234 bytes in 13 blocks.
==10743==      possibly lost: 0 bytes in 0 blocks.
==10743==    still reachable: 12,591,465 bytes in 5,914 blocks.
==10743==         suppressed: 0 bytes in 0 blocks.
==10743== Reachable blocks (those to which a pointer was found) are not shown.
==10743== To see them, rerun with: --show-reachable=yes

Some experiments show that the number of blocks leaked from
readline_internal_teardown is the number of input lines plus 2 (11+2=13
in this case) and the number of leaked bytes is the total number of
bytes in those input lines (including the trailing nulls) plus 40
(10*19+4+40=234 in this case).

I think the readline callback function is expected to free
its 'char *line' argument.  The readline manual says the
input line must be freed when using the simple readline()
interface but is silent about memory management when using
the callback interface.  I have not looked in the readline
source code.

See
   http://www.mail-archive.com/freeciv-dev at gna.org/msg04863.html
for a same problem in other software using readline:
   >  Are you sure about this? Does readline really expect callback to free
   >  the line? (and if it does, can it be bug in the specific readline
   >  version you are using?)
   >   - ML
   A cursory look suggests Elmo is right.
   That you should free the returned line is explicitly mentioned when
   using the readline() call:
   http://www.delorie.com/gnu/docs/readline/rlman_24.html#IDX174
   No further mention at the reference for the alternative interface:
   http://www.delorie.com/gnu/docs/readline/rlman_41.html#IDX288

When I added 'free(line)' to src/unix/sys-std.c:readline_handler()
valgrind stopped complaining about the leaks and did not
complain about using freed memory.

It does start complaining a new leak in the readline code,
but it looks like that may be a constant 2 block, 40 byte
leak, not something that grows as the session goes on:

==11246== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 40 from 2)
==11246== malloc/free: in use at exit: 12,591,505 bytes in 5,916 blocks.
==11246== malloc/free: 21,015 allocs, 15,099 frees, 27,744,803 bytes allocated.
==11246== For counts of detected errors, rerun with: -v
==11246== searching for pointers to 5,916 not-freed blocks.
==11246== checked 12,612,676 bytes.
==11246==
==11246== 40 bytes in 2 blocks are definitely lost in loss record 10 of 42
==11246==    at 0x40046EE: malloc (vg_replace_malloc.c:149)
==11246==    by 0x68BFF9: xmalloc (in /usr/lib/libreadline.so.4.3)
==11246==    by 0x68F65B: sh_set_lines_and_columns (in /usr/lib/libreadline.so..3)
==11246==    by 0x688AEE: _rl_get_screen_size (in /usr/lib/libreadline.so.4.3)
==11246==    by 0x68918C: _rl_init_terminal_io (in /usr/lib/libreadline.so.4.3)
==11246==    by 0x677A77: rl_initialize (in /usr/lib/libreadline.so.4.3)
==11246==    by 0x6888C6: (within /usr/lib/libreadline.so.4.3)
==11246==    by 0x80E7303: Rstd_ReadConsole (sys-std.c:505)
==11246==    by 0x8057F61: Rf_ReplIteration (main.c:205)
==11246==    by 0x805827E: R_ReplConsole (main.c:306)
==11246==    by 0x8058514: run_Rmainloop (main.c:966)
==11246==    by 0x805676D: main (Rmain.c:33)
==11246==
==11246== LEAK SUMMARY:
==11246==    definitely lost: 40 bytes in 2 blocks.
==11246==      possibly lost: 0 bytes in 0 blocks.
==11246==    still reachable: 12,591,465 bytes in 5,914 blocks.
==11246==         suppressed: 0 bytes in 0 blocks.
==11246== Reachable blocks (those to which a pointer was found) are not shown.
==11246== To see them, rerun with: --show-reachable=yes

My change is:

Index: sys-std.c
===================================================================
--- sys-std.c	(revision 46046)
+++ sys-std.c	(working copy)
@@ -550,6 +550,7 @@
 	rl_top->readline_buf[0] = '\n';
 	rl_top->readline_buf[1] = '\0';
     }
+    free(line) ;
     rl_top->readline_gotaline = 1;
 }

Here is the info on the session and the shared libraries currently loaded
(readline 4.3 in particular):

> sessionInfo()
R version 2.8.0 Under development (unstable) (2008-07-07 r46046)
i686-pc-linux-gnu

locale:
LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=en_US.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=C;LC_MESSAGES=en_US.UTF-8;LC_PAPER=en_US.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=en_US.UTF-8;LC_IDENTIFICATION=C

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base
> system(paste("pldd", Sys.getpid()))
11304:  /homes/bill/R-svn/r-devel/R/bin/exec/R --vanilla /a/homer.insightful.com/users/bill/R-svn/r-devel/R/library/grDevices/libs/grDevices.so
/a/homer.insightful.com/users/bill/R-svn/r-devel/R/lib/libRblas.so
/usr/lib/libreadline.so.4.3
/lib/ld-2.3.4.so
/lib/tls/libc-2.3.4.so
/lib/tls/libm-2.3.4.so
/lib/libdl-2.3.4.so
/lib/libnss_files-2.3.4.so
/a/homer.insightful.com/users/bill/R-svn/r-devel/R/library/stats/libs/stats.so
/a/homer.insightful.com/users/bill/R-svn/r-devel/R/library/methods/libs/methods.so
/lib/libnsl-2.3.4.so
/usr/lib/gconv/ISO8859-1.so
/lib/libnss_nis-2.3.4.so
/usr/lib/libg2c.so.0.0.0
/usr/lib/libncurses.so.5.4

/usr/lib/gconv/gconv-modules.cache
/a/homer.insightful.com/users/bill/R-svn/r-devel/R/share/locale/en/LC_MESSAGES/R.mo
/usr/lib/locale/locale-archive



----------------------------------------------------------------------------
Bill Dunlap
Insightful Corporation
bill at insightful dot com
360-428-8146

 "All statements in this message represent the opinions of the author and do
 not necessarily reflect Insightful Corporation policy or position."



[Rd] memory leak in readline code
Bill Dunlap