slapgrid: Do not leak file descriptors to subprocesses when running 'node software'

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 nexedi/slapos.core!42

slapos/grid/utils.py

@@ -97,6 +97,10 @@ class SlapPopen(subprocess.Popen):
     kwargs.update(stdin=subprocess.PIPE)
     if sys.platform == 'cygwin' and kwargs.get('env') == {}:
       kwargs['env'] = None
+
+    # don't leak log & co. filedescriptors to child process
+    kwargs.setdefault('close_fds', True)
+
     subprocess.Popen.__init__(self, *args, **kwargs)
     self.stdin.flush()
     self.stdin.close()

slapos/tests/slapgrid.py

@@ -2683,4 +2683,52 @@ exit 0
       # Assert partition directory is empty
       self.assertInstanceDirectoryListEqual(['0'])
       self.assertItemsEqual(os.listdir(partition.partition_path), [])
-      
+
+
+# test that slapgrid commands, like e.g. 'slapos node software' do not leak
+# file descriptors besides stdin, stdout, stderr to spawned processes.
+class TestSlapgridNoFDLeak(MasterMixin, unittest.TestCase):
+
+  def test_no_fd_leak(self):
+    filev = []
+    try:
+      # open some file descriptors
+      for i in range(4):
+        f = open(os.devnull)
+        filev.append(f)
+        self.assertGreater(f.fileno(), 2)
+
+      # 'node software' with check that buildout does not see opened files
+      self._test_no_fd_leak()
+
+    finally:
+      for f in filev:
+        f.close()
+
+  def _test_no_fd_leak(self):
+    computer = ComputerForTest(self.software_root, self.instance_root, 1, 1)
+    with httmock.HTTMock(computer.request_handler):
+      software = computer.software_list[0]
+
+      software.setBuildout("""#!/bin/bash
+fdleak() {
+  echo "file descriptors: leaked:" "$@"
+  exit 1
+}
+
+# https://unix.stackexchange.com/a/206848
+: >&3 && fdleak 3
+: >&4 && fdleak 4
+: >&5 && fdleak 5
+: >&6 && fdleak 6
+
+echo "file descriptors: ok"
+exit 1  # do not proceed trying to use this software
+""")
+
+      self.launchSlapgridSoftware()
+
+      self.assertEqual(software.sequence,
+                       ['/buildingSoftwareRelease', '/softwareReleaseError'])
+      self.assertNotIn("file descriptors: leaked", software.error_log)
+      self.assertIn("file descriptors: ok", software.error_log)