Memory leak with 'use v5.8'

[p5pRT]

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

Searchable as RT78436$

[p5pRT]

From @ntyni

This is a bug report for perl from Niko Tyni <ntyni@​debian.org>,
generated with the help of perlbug 1.39 running under perl 5.13.5.

---

As reported in <http​://bugs.debian.org/600376> 'use v5.8' leaks
memory.

bash -c 'ulimit -v 70000; ./perl -Ilib -e "eval q{use v5.8} while 1;"'

Proposed patch attached.

---

---

Flags​:
  category=core
  severity=low

---

Site configuration information for perl 5.13.5​:

Configured by niko at Mon Oct 18 13​:12​:14 EEST 2010.

Summary of my perl5 (revision 5 version 13 subversion 5) configuration​:
  Derived from​: df5a381
  Platform​:
  osname=linux, osvers=2.6.32-5-amd64, archname=x86_64-linux-gnu-thread-multi
  uname='linux madeleine 2.6.32-5-amd64 #1 smp fri sep 17 21​:50​:19 utc 2010 x86_64 gnulinux '
  config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.13 -Darchlib=/usr/lib/perl/5.13 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.13.5 -Dsitearch=/usr/local/lib/perl/5.13.5 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -DDEBUGGING=both -Doptimize=-O0 -Dusedevel -Uuseshrplib -des'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O0 -g',
  cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.4.5', gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib /usr/lib /lib64 /usr/lib64
  libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
  libc=/lib/libc-2.11.2.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.11.2'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O0 -g -L/usr/local/lib -fstack-protector'

Locally applied patches​:
 

---

@​INC for perl 5.13.5​:
  lib
  /usr/local/lib/perl/5.13.5
  /usr/local/share/perl/5.13.5
  /usr/lib/perl5
  /usr/share/perl5
  /usr/lib/perl/5.13
  /usr/share/perl/5.13
  /usr/local/share/perl
  /usr/share/perl5
  .

---

Environment for perl 5.13.5​:
  HOME=/home/niko
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LC_CTYPE=fi_FI.UTF-8
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/niko/bin​:/home/niko/bin​:/home/niko/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/local/games​:/usr/games​:/sbin​:/usr/sbin​:/sbin​:/usr/sbin
  PERL_BADLANG (unset)
  SHELL=/bin/zsh

[p5pRT]

From @ntyni

0001-Fix-a-memory-leak-with-use-v5.8.patch

From 8c4c4c5d995b7f34046b0c3a6e391e600eef177c Mon Sep 17 00:00:00 2001
From: Niko Tyni <ntyni@debian.org>
Date: Tue, 19 Oct 2010 10:07:45 +0300
Subject: [PATCH] Fix a memory leak with 'use v5.8'

The leak was originally reported in http://bugs.debian.org/600376
---
 pp_ctl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index a9d92f1..495f0fb 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3463,6 +3463,7 @@ PP(pp_require)
 		PL_hints |= (HINT_STRICT_REFS | HINT_STRICT_SUBS | HINT_STRICT_VARS);
 	    }
 	}
+	SvREFCNT_dec(sv);
 
 	RETPUSHYES;
     }
-- 
1.7.1

[p5pRT]

From @nwc10

On Tue, Oct 19, 2010 at 12​:18​:05AM -0700, Niko Tyni wrote​:

-----------------------------------------------------------------
As reported in <http​://bugs.debian.org/600376> 'use v5.8' leaks
memory.

bash -c 'ulimit -v 70000; ./perl -Ilib -e "eval q{use v5.8} while 1;"'

Proposed patch attached.

Thanks for the report and the proposed patch. I applied this instead​:

commit d086148
Author​: Nicholas Clark <nick@​ccl4.org>
Date​: Tue Oct 19 13​:26​:19 2010 +0200

  Stop 'use v5.8' leaking memory. Fixes #78436.
 
  Based on a patch proposed by Niko Tyni.

Inline Patch

diff --git a/pp_ctl.c b/pp_ctl.c
index a9d92f1..16e7cf9 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3398,7 +3398,7 @@ PP(pp_require)
 
     sv = POPs;
     if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
-       sv = new_version(sv);
+       sv = sv_2mortal(new_version(sv));
        if (!sv_derived_from(PL_patchlevel, "version"))
            upg_version(PL_patchlevel, TRUE);
        if (cUNOP->op_first->op_type == OP_CONST && cUNOP->op_first->op_private 

because the reference count leak you identified also happens along all the DIE\(\) paths in that block\. However\, it doesn't seem to be the only cause of leakage\, as I still see this​:

$ time bash -c 'ulimit -v 70000; ./perl -Ilib -e "eval q{use v5.16} while 1;"'
Out of memory!

real 0m0.985s
user 0m0.930s
sys 0m0.040s

I guess I had better open a new ticket for that.

Nicholas Clark

[p5pRT]

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

[p5pRT]

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

[p5pRT]

From @toddr

On Oct 19, 2010, at 6​:35 AM, Nicholas Clark wrote​:

On Tue, Oct 19, 2010 at 12​:18​:05AM -0700, Niko Tyni wrote​:

-----------------------------------------------------------------
As reported in <http​://bugs.debian.org/600376> 'use v5.8' leaks
memory.

bash -c 'ulimit -v 70000; ./perl -Ilib -e "eval q{use v5.8} while 1;"'

Proposed patch attached.

Thanks for the report and the proposed patch. I applied this instead​:

commit d086148

This seems like it might be a security issue waiting to happen. Will this be back ported to 5.12?

Todd

[p5pRT]

From @JohnPeacock

On 10/19/2010 07​:35 AM, Nicholas Clark wrote​:

diff --git a/pp_ctl.c b/pp_ctl.c
index a9d92f1..16e7cf9 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@​@​ -3398,7 +3398,7 @​@​ PP(pp_require)

  sv = POPs;
  if \( \(SvNIOKp\(sv\) || SvVOK\(sv\)\)&&  PL\_op\->op\_type \!= OP\_DOFILE\) \{

- sv = new_version(sv);
+ sv = sv_2mortal(new_version(sv));

That marks the RV that is returned from new_version() as mortal. But
that doesn't mark the object's HV and the contain AV and PV's, right?
So those objects are what is leaking I think. I suspect the correct way
to deal with this is to create a new_mortal_version that is used
internally for just this purpose.

Thoughts?

John

[p5pRT]

From zefram@fysh.org

John Peacock wrote​:

That marks the RV that is returned from new_version() as mortal. But
that doesn't mark the object's HV and the contain AV and PV's, right?

Right, and this is the correct way to do it. The save stack deletes
the mortal ref to the RV, thus destroying the RV, thus deleting the RV's
ref to the HV, thus destroying the HV, and so on.

-zefram

[p5pRT]

From @JohnPeacock

On 10/19/2010 03​:44 PM, Zefram wrote​:

Right, and this is the correct way to do it. The save stack deletes
the mortal ref to the RV, thus destroying the RV, thus deleting the RV's
ref to the HV, thus destroying the HV, and so on.

That's what I thought, but using Test​::LeakTrace, it is apparent that
the AV and PV contained in temporary version objects (e.g. the ones
automatically created in XS_VERSION_BOOTCHECK) are leaking. There is
even an RT ticket about it​:

  http​://rt.perl.org/rt3/Public/Bug/Display.html?id=72686

That patch has effectively already been committed, FWIW...

John

[p5pRT]

From @nwc10

On Tue, Oct 19, 2010 at 10​:07​:24AM -0500, Todd Rinaldo wrote​:

On Oct 19, 2010, at 6​:35 AM, Nicholas Clark wrote​:

On Tue, Oct 19, 2010 at 12​:18​:05AM -0700, Niko Tyni wrote​:

-----------------------------------------------------------------
As reported in <http​://bugs.debian.org/600376> 'use v5.8' leaks
memory.

bash -c 'ulimit -v 70000; ./perl -Ilib -e "eval q{use v5.8} while 1;"'

Proposed patch attached.

Thanks for the report and the proposed patch. I applied this instead​:

commit d086148

This seems like it might be a security issue waiting to happen. Will this be back ported to 5.12?

I don't see why this is any more security issue than any other resource leak.
In particular, I believe it's not possible to exploit this for a denial of
service attack without first having control of what code is run, and if you
have that, you can do far worse, far faster, far more easily.

I suspect that actually most other resource leaks are more serious than this
one, as the nature of a use statement for a perl version is such that it's
unlikely to be written in loops. Instead, it's likely to be in run-once load-
time code, which would mean that for typical code it's just an unfortunate
extra static overhead, rather than a dynamic drain.

(But if Debian which to backport it for their distribution, that's their call.)

Nicholas Clark

[p5pRT]

From @cpansprout

On Tue Oct 19 13​:17​:19 2010, john.peacock@​havurah-software.org wrote​:

On 10/19/2010 03​:44 PM, Zefram wrote​:

Right, and this is the correct way to do it. The save stack deletes
the mortal ref to the RV, thus destroying the RV, thus deleting the RV's
ref to the HV, thus destroying the HV, and so on.

That's what I thought, but using Test​::LeakTrace, it is apparent that
the AV and PV contained in temporary version objects (e.g. the ones
automatically created in XS_VERSION_BOOTCHECK) are leaking. There is
even an RT ticket about it​:

http​://rt.perl.org/rt3/Public/Bug/Display.html?id=72686

That patch has effectively already been committed, FWIW...

Does that mean 72686 can be closed?