Opened 15 years ago

Closed 14 years ago

#2305 defect closed fixed (fixed)

twisted/internet/process.py leaks file descriptors when os.fork() raises

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: eric.newton, therve, itamarst Branch: branches/pipe-leak-2305-3
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description (last modified by itamarst)

os.pipe() is called before os.fork()

No error handling is done around os.fork() or os.pipe().

Any file descriptors created for pipes before the os.fork() will be leaked indefinitely if os.fork()/pipe() raises an exception.

Change History (20)

comment:1 Changed 15 years ago by Jean-Paul Calderone

#2335, a duplicate of this ticket, includes a patch.

comment:2 Changed 15 years ago by eric.newton

Cc: eric.newton added

comment:3 Changed 15 years ago by itamarst

Description: modified (diff)

Updated to note os.pipe() has no error handling either.

comment:4 Changed 15 years ago by therve

Cc: therve added
Keywords: review added
Owner: Glyph deleted
Priority: normalhighest

Ready to review in pipe-leak-2305.

comment:5 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

This is going to conflict pretty badly with #2341. It also only fixes the problem for spawnProcess(usePTY=False), not spawnProcess(usePTY=True) (and the duplication in code/bugs currently involved in implementing those two variations is one reason #2341 is a good idea).

Recommend holding off on this until #2341 is merged, and then fixing it once.

comment:6 Changed 15 years ago by itamarst

Cc: itamarst added

Suggested improvement for this code: instead of re-raising errors, pass them as Failures to processEnded, so user can handle them.

As far as finishing #2341: at least some of it has been moved out (#2419, awaiting review) and some of it still needs moving out (#2420). The remaining code is probably still useful until we do the full rewrite, but will need some work (removing stuff that is now in other bugs, writing some tests, validating it all works by someone who knows low-level OS details).

My suggested order: #2419, #2420, #2341, then this.

comment:7 Changed 15 years ago by Jean-Paul Calderone

This is perhaps a duplicate of #1410

comment:8 Changed 14 years ago by therve

Priority: highesthigh

comment:9 Changed 14 years ago by therve

author: therve
Branch: branches/pipe-leak-2305-3

(In [23625]) Branching to 'pipe-leak-2305-3'

comment:10 Changed 14 years ago by therve

Keywords: review added
Owner: therve deleted

This is ready to review. Another side effect I solved was with UID management: the original settings were not restored in case of failure.

comment:11 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve
  • test_process.py
    • test_mockForkErrorClosePTY has a typo - C{userPTY}
    • test_mockErrorInPipe has a typo - the creates pipes
    • I think set makes sense for comparing things like self.mockos.closed and [-1, -4, -6, -2, -3, -5] (in `test_mockForErrorClosesFDs) - the order doesn't actually matter here, right? Likewise for the other comparions of lists of file descriptors which appear in several of the other test methods.
    • in the docstring for test_mockErrorInForkRestoreUID, I'd write a UID change rather than an UID change
  • process.py
    • I think the UID/GID should be restored before gc.enable() is called. The order for the non-exceptional case seems wrong to me as well. gc.enable() might call arbitrary application code, it'd be bad to do that with the wrong credentials I bet.
    • Maybe try/finally could be used to avoid duplicating the cleanup code?
    • I'm not sure there's any way the os.close calls at line 544, 545, 825, and 826 could raise an exception, but I'm not sure they certainly can't either. Worst case, they could fail with something like MemoryError. Hmm. I was going to suggest giving them exception handling and being careful that the original exception gets raised at the end, but maybe we should just wait and see if there's an actual reason to do this.

comment:12 Changed 14 years ago by Jean-Paul Calderone

One of the tests fails for me:

[FAIL]: twisted.test.test_process.MockProcessTestCase.test_mockForkErrorClosePTY

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_process.py", line 1577, in test_mockForkErrorClosePTY
    self.assertEqual(self.mockos.closed, [4, 9])
twisted.trial.unittest.FailTest: not equal:
a = [4, 7]
b = [4, 9]

comment:13 Changed 14 years ago by therve

(In [23820]) Process review comments.

Refs #2305

comment:14 Changed 14 years ago by therve

Keywords: review added
Owner: therve deleted

I've fixed every points. I didn't used try/finally because it was a bit complicated to figure out out to fix the tests in client side.

comment:15 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Looks good. No worries about the try/finally thing. I noticed another issue, I think. If childFDs is specified, then file descriptors from it will end up in the fdmap local and then get closed if _fork fails. I think it would be better if they were left open. What do you think?

comment:16 Changed 14 years ago by therve

(In [23831]) Fix the problem with user-specified fd

Refs #2305

comment:17 Changed 14 years ago by therve

Keywords: review added
Owner: therve deleted

That seems like a good suggestion :) Solved now I think.

comment:18 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

In the comment in test_mockForkErrorGivenFDs, creates should be create. That's it, please merge.

comment:19 Changed 14 years ago by therve

Resolution: fixed
Status: newclosed

(In [23846]) Merge pipe-leak-2305-3

Author: therve Reviewer: exarkun Fixes #2305 Fixes #1410

Handle errors raised by os.pipe and os.fork in spawnProcess, to make sure that opened file descriptors are closed and that UID settings are restored.

comment:20 Changed 11 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.