Bugfix for memory leak [rt.cpan.org #57990]

[toddr]

Migrated from rt.cpan.org#57990 (status was 'open')

Requestors:

• akobernik@iponweb.net

Attachments:

• RT7990.patch
• RT7990.patch.txt
• IPC-Run.patch

From akobernik@iponweb.net on 2010-05-31 10:51:07:

Most probably, the ticket is a duplicate of Bug #13660.

Distribution name and version: IPC-Run-0.89
Perl version: v5.10.1 (*) built for i486-linux-gnu-thread-multi
Operating System vendor and version: Linux finland 2.6.32-trunk-686 #1
SMP Sun Jan 10 06:32:16 UTC 2010 i686 GNU/Linux

How to reproduce the memory leak:

use IPC::Run ();

while (1) {
     sleep 1;

     my $stdin  = '';
     my $stdout = '';
     my $stderr = '';

     eval { IPC::Run::run( ["ls"], \$stdin, \$stdout, \$stderr ) };
     die $@ if $@;
}

Patch:

--- lib/IPC/Run/IO.pm.orig	2010-05-31 14:55:01.000000000 +0400
+++ lib/IPC/Run/IO.pm	2010-05-31 14:55:12.000000000 +0400
@@ -351,6 +351,7 @@
  sub _cleanup { ## Called from Run.pm's _cleanup
     my $self = shift;
     undef $self->{FAKE_PIPE};
+   undef $self->{FILTERS};
  }

(found here:
http://download2.3tera.net/oss/files/osm/perl-IPC-Run-0.80-2/IPC-Run-mem-leak.patch)

--
Alexander O. Kobernik

---

From toddr@cpan.org on 2012-08-22 12:48:45:

The internals seem a little dodgy if this is a safe or appropriate thing to do. There's not
spectacular documentation on the _cleanup subroutines.

What I do know is that the patch breaks the test suite ATM....

$>make test               
PERL_DL_NONLAZY=1 /usr/local/cpanel/bin/perl "-MExtUtils::Command::MM" "-e" 
"test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/97_meta.t .................. ok   
t/98_pod.t ................... ok   
t/98_pod_coverage.t .......... ok   
t/99_perl_minimum_version.t .. skipped: no minimum perl version could be determined
t/adopt.t .................... skipped: adopt not implemented yet
t/binmode.t .................. ok     
t/bogus.t .................... ok   
t/eintr.t .................... ok   
t/filter.t ................... ok     
t/harness.t .................. ok       
t/io.t ....................... ok     
t/kill_kill.t ................ ok   
t/parallel.t ................. ok   
t/pty.t ...................... # IO::Tty 1.10, IO::Pty 1.10
t/pty.t ...................... ok     
t/pump.t ..................... ok     
t/run.t ...................... 134/268 ack Can't use an undefined value as a SCALAR reference at 
/Users/toddr/projects/IPC-Run/blib/lib/IPC/Run.pm line 2449.
# Looks like you planned 268 tests but ran 241.
# Looks like your test exited with 9 just after 241.
t/run.t ...................... Dubious, test returned 9 (wstat 2304, 0x900)
Failed 27/268 subtests 
t/signal.t ................... ok   
t/timeout.t .................. 9/26 
#   Failed test at t/timeout.t line 67.
#          got: 'ack Can't use an undefined value as a SCALAR reference at 
/Users/toddr/projects/IPC-Run/blib/lib/IPC/Run.pm line 2449.
# '
#     expected: '(?-xism:IPC::Run: timeout)'

#   Failed test at t/timeout.t line 69.

#   Failed test at t/timeout.t line 70.

#   Failed test at t/timeout.t line 75.
#          got: '0'
#     expected: '>= 1'
t/timeout.t .................. 26/26 # Looks like you failed 4 tests of 26.
t/timeout.t .................. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/26 subtests 
t/timer.t .................... ok     
t/utf8.t ..................... ok   
t/win32_compile.t ............ ok   
t/windows_search_path.t ...... ok     

Test Summary Report
-------------------
t/run.t                    (Wstat: 2304 Tests: 241 Failed: 0)
  Non-zero exit status: 9
  Parse errors: Bad plan.  You planned 268 tests but ran 241.
t/timeout.t                (Wstat: 1024 Tests: 26 Failed: 4)
  Failed tests:  19, 21-23
  Non-zero exit status: 4
Files=22, Tests=684, 13 wallclock secs ( 0.16 usr  0.07 sys +  1.81 cusr  0.58 csys =  2.62 CPU)
Result: FAIL
Failed 2/22 test programs. 4/684 subtests failed.
make: *** [test_dynamic] Error 255

From tom@embt.com on 2012-10-09 00:05:16:

FWIW, this is a solution I'm trying locally and tests do pass. I tried
the other patch but it seems to prevent rerunning a harness. I can't
say for certain if this change is safe, I am not familiar with all of
the IPC::Run internals.

--- a/lib/IPC/Run.pm
+++ b/lib/IPC/Run.pm
@@ -1136,4 +1136,10 @@ sub DESTROY {
    POSIX::close $self->{DEBUG_FD} if defined $self->{DEBUG_FD};
    $self->{DEBUG_FD} = undef;
+
+   for my $kid ( @{$self->{KIDS}} ) {
+      for my $op ( @{$kid->{OPS}} ) {
+         @{$op->{FILTERS}} = ();
+      }
+   }
 }
 
@@ -2656,5 +2662,5 @@ sub _do_kid_and_exit {
       _write $self->{SYNC_WRITER_FD}, $@;
       ## Avoid DESTROY.
-      POSIX::exit 1;
+      POSIX::_exit 1;
    }
 
@@ -2673,10 +2679,10 @@ sub _do_kid_and_exit {
    $kid->{VAL} = undef;
 
-   ## Use POSIX::exit to avoid global destruction, since this might
+   ## Use POSIX::_exit to avoid global destruction, since this might
    ## cause DESTROY() to be called on objects created in the parent
    ## and thus cause double cleanup.  For instance, if DESTROY() unlinks
    ## a file in the child, we don't want the parent to suddenly miss
    ## it.
-   POSIX::exit 0;
+   POSIX::_exit 0;
 }
 
@@ -4345,5 +4351,5 @@ The second problem is that Perl's DESTROY subs and
other on-exit cleanup gets
 run in the child process.  If objects are instantiated in the parent
before the
 child is forked, the the DESTROY will get run once in the parent and
once in
-the child.  When coprocess subs exit, POSIX::exit is called to work
around this,
+the child.  When coprocess subs exit, POSIX::_exit is called to work
around this,
 but it means that objects that are still referred to at that time are not
 cleaned up.  So setting package vars or closure vars to point to
objects that

[kbucheli]

updated patch again for the latest version:

--- lib/IPC/Run.pm.orig	2017-09-18 14:31:36.806085038 +0200
+++ lib/IPC/Run.pm	2017-09-18 14:33:24.282816108 +0200
@@ -1144,6 +1144,12 @@
     my IPC::Run $self = shift;
     POSIX::close $self->{DEBUG_FD} if defined $self->{DEBUG_FD};
     $self->{DEBUG_FD} = undef;
+
+    for my $kid ( @{$self->{KIDS}} ) {
+        for my $op ( @{$kid->{OPS}} ) {
+            delete $op->{FILTERS};
+        }
+    }
 }
 
 ##
@@ -2620,7 +2626,7 @@
     if ($@) {
         _write $self->{SYNC_WRITER_FD}, $@;
         ## Avoid DESTROY.
-        POSIX::exit 1;
+        POSIX::_exit 1;
     }
 
     ## We must be executing code in the child, otherwise exec() would have
@@ -2637,12 +2643,12 @@
     ## this may cause the closure to be cleaned up.  Maybe.
     $kid->{VAL} = undef;
 
-    ## Use POSIX::exit to avoid global destruction, since this might
+    ## Use POSIX::_exit to avoid global destruction, since this might
     ## cause DESTROY() to be called on objects created in the parent
     ## and thus cause double cleanup.  For instance, if DESTROY() unlinks
     ## a file in the child, we don't want the parent to suddenly miss
     ## it.
-    POSIX::exit 0;
+    POSIX::_exit 0;
 }
 
 =pod
@@ -4304,7 +4310,7 @@
 The second problem is that Perl's DESTROY subs and other on-exit cleanup gets
 run in the child process.  If objects are instantiated in the parent before the
 child is forked, the DESTROY will get run once in the parent and once in
-the child.  When coprocess subs exit, POSIX::exit is called to work around this,
+the child.  When coprocess subs exit, POSIX::_exit is called to work around this,
 but it means that objects that are still referred to at that time are not
 cleaned up.  So setting package vars or closure vars to point to objects that
 rely on DESTROY to affect things outside the process (files, etc), will

[pabs3]

We are running into this memory leak issue at work, it would be great if the patch could be merged and a new fixed version released.

I've also tested that the test suite does not fail with the latest version of the patch.

[pabs3]

Ping, any chance this could be merged?

[toddr]

A pull request was outstanding to look at test results. Ideally I'd like a test that shows this problem. Not sure do that at this time.

[toddr]

@pabs3 Travis doesn't seem to like the pull request I produced to try to take this patch on. Can you see anything wrong with it from your original submission? #102

[toddr]

@pabs3 I'm 90% certain the issue is that you can't just blanket clear FILTERS. First of all it's a array ref so you need to set it back to that not undef. But more importantly it looks like you need to re-set it to whatever was passed into new. To be honest it almost looks like you can't count on cleanup to work right WRT filters.

I'll have to look at this later.

[toddr]

Actually I don't think the bug is what you think it is. There's a known problem with autovivification of Scalar SVs when the reference to them is passed in and then later set in a subroutine. The refcount is never sufficiently reduced.

[pabs3]

I'm not the author of the patch, I think you should be pinging @kbucheli or someone else.

[kbucheli]

The current PR is rather different from my original patch (which we use in production).
What jumped into my eye is that the for loop resetting FILTERS is in sub _debug_fd and after the return.
That will not have any effect.

As it is now in Github, I created now a PR for my patch: #115

[toddr]

Does #115 merged in master fix your problem? Be aware that newer Perls leak more slowly because of COW

[toddr]

I tested with this modified code, using Devel::Peek. I could see the COW refcount going up and the SV addresses changing every iteration. I assume that means that every loop was causing a new PAD to get created which probably fails at some point not just running out of memory.

Now with #115 it's not happening so I think we're good to merge for now.

use IPC::Run ();
use Devel::Peek;

while (1) {
     sleep 1;

     my $stdin  = '';
     my $stdout = '';
     my $stderr = '';

     eval { IPC::Run::run( ["ls"], \$stdin, \$stdout, \$stderr ) };
     die $@ if $@;
     Dump( $stdin);
     Dump( $stdout);
     Dump( $stderr);
}

[toddr]

Merged.