classification
Title: popen2.Popen3 and popen2.Popen4 leaks filedescriptors
Type: Stage:
Components: Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: nnorwitz Nosy List: gaul, gvanrossum, mwh, nnorwitz, troels
Priority: normal Keywords:

Created on 2003-06-27 14:58 by troels, last changed 2010-08-18 16:39 by BreamoreBoy. This issue is now closed.

Files
File name Uploaded Description Edit
popen2f.diff nnorwitz, 2003-06-29 03:09 full patch with whitespace 1
popen2m.diff nnorwitz, 2003-06-29 03:09 minimal patch/unified diff (no whitespace) 1
popen2.diff nnorwitz, 2003-07-01 03:40 new patch 3
Messages (8)
msg16629 - (view) Author: Troels Walsted Hansen (troels) Date: 2003-06-27 14:58
The code below (from Lib/popen2.py) appears to leak no
less then 4 filedescriptors if os.fork() raises an
exception (say "OSError: [Errno 12] Not enough space"
on a Solaris box running out of swap).

Popen3.__init__() appears to leak 6 filedescriptors.

class Popen4(Popen3):
    def __init__(self, cmd, bufsize=-1):
        p2cread, p2cwrite = os.pipe()
        c2pread, c2pwrite = os.pipe()
        self.pid = os.fork()
msg16630 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2003-06-29 03:09
Logged In: YES 
user_id=33168

The attached patch should fix the problem, I'd appreciate if
you could test this.  There are 2 files attached, one is a
context diff with whitespace modifications, the other is a
minimal (no whitespace differences) patch to show what's
changed (ie, the try/except).
msg16631 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2003-06-29 03:10
Logged In: YES 
user_id=33168

Tim, for 2.3b2 or 2.3.1?
msg16632 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2003-06-30 15:57
Logged In: YES 
user_id=6380

I haven't reviewed the code or the fix, but as a bugfix this
could go into 2.3.

The two patch files are confusing -- why two?
msg16633 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2003-07-01 03:40
Logged In: YES 
user_id=33168

I tried to make it easier to review the patches by providing
2.  They were wrong though.  It was still possible to leak
file descriptors.  I believe the new patch 3 should not
allow any file descriptors to leak.

This solution is really ugly, but I can't come up with
anything cleaner.  I tried having a list of the fds that
need to be closed, but it really isn't any better.

Would it be possible (in 2.4) to make an fd type which
derives from int, so that the fd can be closed on deallocation?
msg16634 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2003-07-09 16:39
Logged In: YES 
user_id=6380

The fd-as-object idea has dangers too:
there may be situations where you might be
passing a fd to some extension code and never
use it again yourself -- but losing the fd would
close it! Not a good thing.  (The simples
example of this would be os.close() itself. :-)

I don't think I can review the patch adequately;
as it is quite complex I recommend that you check
it in and the point python-dev and c.l.py.ann to it.
msg16635 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2003-07-10 16:30
Logged In: YES 
user_id=6656

"new patch 3" looks ok, ish.

wouldn't the last try block be better written as

try:
 os.fork()
execpt OSError:
 cleanup
 raise
else:
 ...

?  It's possible I'm missing something, of course.
msg16636 - (view) Author: Andrew Gaul (gaul) Date: 2003-10-01 18:50
Logged In: YES 
user_id=139865

Patch #816059 includes a less ugly fix for this bug.
History
Date User Action Args
2010-08-18 16:39:14BreamoreBoysetstatus: open -> closed
resolution: out of date
2009-02-12 00:42:41ajaksu2linkissue816059 dependencies
2008-01-20 18:57:45christian.heimessetversions: + Python 2.6, - Python 2.3
2003-06-27 14:58:01troelscreate