Memory leaks in threaded Perl (cloning PVGVs and PL_my_cxt_list)

[p5pRT]

Migrated from rt.perl.org#77352 (status was 'resolved')

Searchable as RT77352$

[p5pRT]

From jirka@fud.cz

Created by jirka@fud.cz

Hi all,

I would like to report two memory leaks in the threaded Perl
interpreter. They are not very important in an average script,
but in certain scenarios (like mine, running a long-term script
on a semi-embedded device with limited RAM), they can be deadly.

First problem is in S_sv_dup_common() [sv.c] - in the 2nd switch,
if sv_type is SVt_PVGV and isGV_with_GP(source) is not true,
Perl_rvpv_dup() is called to duplicate the value. This is wrong,
results in a memory leak and the statement shall be removed.
The value is first duplicated a bit above in an if statement​:
  if (sv_type != SVt_PVAV && sv_type != SVt_PVHV // (1)
  && !isGV_with_GP(dstr) && (...)) // (2)
  Perl_rvpv_dup(aTHX_ dstr, sstr, param);
And then duplicated again, leaking the previous copy​:
  switch (sv_type) {
  case SVt_PVGV​: // (1)
  if(isGV_with_GP(sstr)) { // (2)
  ...
  } else
  Perl_rvpv_dup(aTHX_ dstr, sstr, param); // !!!

The second issue deals with PL_my_cxt_list. As I understood,
this list is initialized when needed during module load and
correctly cloned when duplicating an interpreter for another
thread, however it is never freed. Because I didn't find a way
to release the contents properly (is there any?), I added at
least a simple free of the array itself - seemed to work well
enough (I probably haven't used any modules with context data).

Patch with my modifications against blead Perl from git should
be attached. The code has been running fine for months now on the
remote box, I also tested the changes on the internal test suite
and it seemed I did not break anything. On the contrary, valgrind
happily confirmed that some memory usage issues have been solved.

The problem can be easily replicated using the following command​:
  valgrind --leak-check=full ./perl -we 'use threads; use CGI; \
  sub foo { }; threads->create(\&foo)->join;'
Note the CGI module is loaded only to add more symbols to the
interpreter to demonstrate the leak. Before my patches​:
  definitely lost​: 7,844 bytes in 65 blocks

After the patch​:
  definitely lost​: 0 bytes in 0 blocks

I hope my report is OK and will help to improve Perl.

Best regards,
Jiri Hruska

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.13.4:

Configured by jirka at Fri May 14 00:16:55 CEST 2010.

Summary of my perl5 (revision 5 version 13 subversion 4) configuration:
  Derived from: ae5391ad3eac034928d0ad9aeb57e8d9f625142f
  Platform:
    osname=linux, osvers=2.6.26-2-686, archname=i686-linux-thread-multi
    uname='linux saturn 2.6.26-2-686 #1 smp tue mar 9 17:35:51 utc
2010 i686 gnulinux '
    config_args='-Dusedevel -Duseithreads'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_F
ILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.3.2', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.7.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.7'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E
-Wl,-rpath,/home/jirka/dev/test-perl/lib/5.13.4/i686-linux-thread-multi
/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector'

Locally applied patches:



@INC for perl 5.13.4:
    /home/jirka/dev/test-perl/lib/site_perl/5.13.4/i686-linux-thread-multi
    /home/jirka/dev/test-perl/lib/site_perl/5.13.4
    /home/jirka/dev/test-perl/lib/5.13.4/i686-linux-thread-multi
    /home/jirka/dev/test-perl/lib/5.13.4
    .


Environment for perl 5.13.4:
    HOME=/home/jirka
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/bash

[p5pRT]

From jirka@fud.cz

ithreads-clone-leaks.patch

diff --git a/sv.c b/sv.c
index c9ab89e..a882034 100644
--- a/sv.c
+++ b/sv.c
@@ -11306,8 +11306,7 @@ S_sv_dup_common(pTHX_ const SV *const sstr, CLONE_PARAMS *const param)
 			Perl_sv_add_backref(aTHX_ MUTABLE_SV(GvSTASH(dstr)), dstr);
 		    GvGP(dstr)	= gp_dup(GvGP(sstr), param);
 		    (void)GpREFCNT_inc(GvGP(dstr));
-		} else
-		    Perl_rvpv_dup(aTHX_ dstr, sstr, param);
+		}
 		break;
 	    case SVt_PVIO:
 		/* PL_parser->rsfp_filters entries have fake IoDIRP() */
diff --git a/perl.c b/perl.c
index 57be5d2..ff1c94f 100644
--- a/perl.c
+++ b/perl.c
@@ -1070,6 +1070,9 @@ perl_destruct(pTHXx)
 	    Perl_ck_warner_d(aTHX_ packWARN(WARN_INTERNAL),"Unbalanced context: %ld more PUSHes than POPs\n",
 			     (long)cxstack_ix + 1);
     }
+    
+    /* Not ideal, doesn't care about contents, but better than nothing */
+    Safefree(PL_my_cxt_list);
 
     /* Now absolutely destruct everything, somehow or other, loops or no. */
 

[p5pRT]

From @iabyn

On Sat, Aug 21, 2010 at 08​:02​:14PM -0700, Jirka HruÅ¡ka wrote​:

I would like to report two memory leaks in the threaded Perl
interpreter. They are not very important in an average script,
but in certain scenarios (like mine, running a long-term script
on a semi-embedded device with limited RAM), they can be deadly.

First problem is in S_sv_dup_common() [sv.c]

Fix looks good thanks, applied as commit

  61e14cb

with minor comment tweaks

The second issue deals with PL_my_cxt_list. As I understood,
this list is initialized when needed during module load and
correctly cloned when duplicating an interpreter for another
thread, however it is never freed. Because I didn't find a way
to release the contents properly (is there any?), I added at
least a simple free of the array itself - seemed to work well
enough (I probably haven't used any modules with context data).

The contents don't need to be freed as they point to the PVX fields
of SVs which get cleanup up later. So I've applied your fix with a change
to the comment, as commit

  57bb245

--
"You may not work around any technical limitations in the software"
  -- Windows Vista license

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

@iabyn - Status changed from 'open' to 'resolved'

[p5pRT]

From jirka@fud.cz

Dave,

Fix looks good thanks, applied as commit
Great, thanks!

The contents don't need to be freed as they point to the PVX fields
of SVs which get cleanup up later. So I've applied your fix with a change
to the comment, as commit
Ah, thanks for the insight - I can feel better about that lone free now :-)