• Kirill Smelkov's avatar
    slapgrid: Do not leak file descriptors to subprocesses when running 'node software' · 2f5ade0a
    Kirill Smelkov authored
    Some context: I'm trying to build Go 1.10 under slapos and get failures
    like:
    
    	ok   os      0.782s
    	PASS
    	testing: can't write /tmp/go-build511454695/b682/testlog.txt: write /tmp/go-build511454695/b682/testlog.txt: broken pipe
    	FAIL os/exec 0.947s
    	...
    	ok   cmd/vet/internal/cfg    0.002s
    	2018/04/03 18:13:29 Failed: exit status 1
    	golang1.10: Command failed with exit code 1:   cd src && ls -l /proc/self/fd && ./all.bash && cp -alf .. /srv/slapgrid/slappart5/srv/runner/software/099a7368550e95d120639cb03b6d47c8/parts/golang1.10
    	golang1.10: Compilation error. The package is left as is at /srv/slapgrid/slappart5/srv/runner/software/099a7368550e95d120639cb03b6d47c8/parts/golang1.10__compile__/go where you can inspect what went wrong
    	While:
    	  Installing golang1.10.
    	Error: System error
    
    This was traced to https://github.com/golang/go/issues/24285, which in turn was
    traced by me to the fact that golang os/exec tests close all file descriptors >
    4 assuming that only stdin, stdout, stderr, epoll and testlog file descriptors
    are there:
    
    	https://github.com/golang/go/blob/26e0e8a840/src/os/exec/exec_test.go#L402
    	https://github.com/golang/go/blob/26e0e8a840/src/os/exec/exec_test.go#L415
    
    However when go build is run under slapos it starts with the following
    descriptors inherited in the environment:
    
    	lr-x------ 1 slapuser5 slapuser5 64 Apr  3 17:57 0 -> pipe:[2969903608]
    	l-wx------ 1 slapuser5 slapuser5 64 Apr  3 17:57 1 -> pipe:[2969903609]
    	l-wx------ 1 slapuser5 slapuser5 64 Apr  3 17:57 2 -> pipe:[2969903609]
    	l-wx------ 1 slapuser5 slapuser5 64 Apr  3 17:57 3 -> /srv/slapgrid/slappart5/srv/runner/software.log
    
    i.e. the file descriptor for software.log (under `slapos node software`) is
    passed opened to spawned buildout commands -> and oops - go build failure.
    
    I'm not saying go behaviour is correct. However for slapos it is also not good
    to leak e.g. log file descriptors to spawned processes (e.g. what if spawned
    child writes there by mistake?).
    
    Since fixing on Go side is not very easy, because it will probably require
    non-small testing infrastructure refactoring, I decided to fix on SlapOS side,
    because it is a) easier, and b) correct & good to do in any way.
    
    ----
    
    To fix we just pass close_fds=True to subprocess.Popen run from under
    SlapPopen, which means that popen will close all file descriptors besides
    stdin, stdout & stderr for child. close_fds default is already True for python3
    
    	https://www.python.org/dev/peps/pep-0433/#subprocess-close
    
    for the reason that it is frequent mistake for programmers to not leak file
    descriptors to spawned processes.
    
    However on python2, which we are still using, close_fds default is False.
    
    So let's fix the default on our side.
    
    /reviewed-by @jerome, @rafael
    /reviewed-on !42
    2f5ade0a
slapgrid.py 114 KB