pymad / cpymad

cPyMAD is a python interface to Mad-X using cython and libmadx
http://cern.ch/pymad
Apache License 2.0
3 stars 3 forks source link

Usage of multiprocessing causes handle leakage #43

Closed coldfix closed 10 years ago

coldfix commented 10 years ago

When spawning a new process via multiprocessing.Process the child receives a copy of all handles in the parent process. This has problematic side effects for any communication protocol in the parent process that relies on closing handles at some point. Furthermore, this is a security issue: suppose the parent process has obtained a handle to a secure resource (e.g. SSL socket) and somebody might succeed to use faulty MADX commands to access the handle (I'd say that is quite possible).

The following example demonstrates that it is not sufficient to close a handle in the parent process, since the child has a copy which it can still use:

import multiprocessing

_global = []

def child(conn):
    txt = conn.recv()
    _global[0].write(txt)
    _global[0].close()
    conn.send(txt)

def parent():
    _global.append(open('x.txt', 'w'))
    lconn, rconn = multiprocessing.Pipe(True)
    process = multiprocessing.Process(target=child, args=(rconn,))
    process.start()
    _global[0].close()
    lconn.send('Hello World!')
    lconn.recv()
    print(open('x.txt').read())    # "Hello World!"

if __name__ == '__main__':
    parent()

I have looked into solutions for this problem. I do not think there is any nice fix if sticking to multiprocessing. The subprocess.Popen(..., close_fds=True) command allows to close all inherited handles. The problem is, that this also closes any opened Pipe which makes it necessary to use another mechanism for IPC.

The next thing that comes to mind is using sockets. This creates the next issue: how to make sure that the connection is authenticated and secure against tempering? Possibly, SSL could be used, but this is even more additional complexity (worth?).

Please let me know if you have any suggestions for reliable/secure local IPC.

coldfix commented 10 years ago

It seems subprocess.Popen(..., stdin=PIPE, stdout=PIPE, close_fds=False) behaves well for python>=2.7. This is due to a patch that makes all opened file descriptors non-inheritable by default. To be more precise, they will be automatically closed on execv (but not on fork), see PEP 446.

Since I am not using python2.6, I'd be okay using STDIN/STDOUT for communication with the subprocess in order to avoid leakage of handles. For python2.6 itself it wouldn't make any difference when compared to the current implementation. Would you rather try to find a solution that works for 2.6 as well?

If you have concerns because MAD-X uses STDOUT - it should be possible to remap STDOUT to a different file descriptor before starting MAD-X (=importing the cython module) in the subprocess, via os.dup2.

To test that no handles are leaked, execute the following example (after saving it to a file):

import sys
import subprocess
import time

def parent():
    c1 = start_server(0)
    time.sleep(1)
    c2 = start_server(1)
    time.sleep(1)
    print("Closing: 0")
    c1.stdin.close()
    time.sleep(1)
    print("Closing: 1")
    c2.stdin.close()
    time.sleep(1)

def start_server(num):
    return subprocess.Popen([sys.executable, __file__, str(num)],
                            stdin=subprocess.PIPE,
                            stdout=subprocess.PIPE,
                            close_fds=False)

def child(num):
    sys.stderr.write("Started: %d\n" % num)
    sys.stdin.read()   # blocks until the last fd is closed
    sys.stderr.write("Finished: %d\n" % num)

if __name__ == '__main__':
    if len(sys.argv) > 1:
        child(int(sys.argv[1]))
    else:
        parent()

The output on python>=2.7 should be:

Started: 0
Started: 1
Closing: 0
Finished: 0
Closing: 1
Finished: 1

While when leaking handles, the output should be like (last 2 lines may be interchanged):

Started: 0
Started: 1
Closing: 0
Closing: 1
Finished: 1
Finished: 0
coldfix commented 10 years ago

Realized today, that PEP446 is implemented in python3.4 not in python2.7. I got the impression, things were good since the test above started working on python2.7, not sure why now, possibly because of some garbage collection. It seems, we need os.closerange after all, to close at least some of the open handles.

coldfix commented 10 years ago

Okay to merge?