Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak with 'use v5.8' #10737

Closed
p5pRT opened this issue Oct 19, 2010 · 11 comments
Closed

Memory leak with 'use v5.8' #10737

p5pRT opened this issue Oct 19, 2010 · 11 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 19, 2010

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

Searchable as RT78436$

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2010

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

@p5pRT p5pRT closed this as completed Oct 19, 2010
@p5pRT
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 19, 2010

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
Copy link
Author

p5pRT commented Oct 20, 2010

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
Copy link
Author

p5pRT commented Oct 24, 2010

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant