[PATCH] Avoid inheriting *.stdout and *.stderr into every child process

[Andrew Bartlett]

On Mon, 2018-08-27 at 22:43 +1200, Andrew Bartlett via samba-technical
wrote:
> On Mon, 2018-08-27 at 12:29 +0200, Stefan Metzmacher wrote:
> > Hi Andrew,
> > 
> > > > I hope this clarifies,
> > > 
> > > You can see that --tail still works on any job in the CI link:
> > > https://gitlab.com/catalyst-samba/samba/pipelines/28743555
> > > 
> > > eg specifically:
> > > https://gitlab.com/catalyst-samba/samba/-/jobs/92279525
> > > 
> > > A clarified patch attached.
> > 
> > I don't have access there, can you use the public samba-team pipelines?
> 
> Sorry, I've re-submitted the job there:
> 
> https://gitlab.com/samba-team/devel/samba/pipelines/28752233
> 
> > It's still unclear for the cases, where we don't explicitly
> > pass in all three values for stdout, stderr and stdin.
> 
> The documentation states:
> 
> https://docs.python.org/2.6/library/subprocess.html
> 
> "If close_fds is true, all file descriptors except 0, 1 and
> 2 will be closed before the child process is executed. (Unix only)."
> 
> And regarding the passed in parameters:
> 
> "stdin, stdout and stderr specify the executed programs’ standard
> input,
> standard output and standard error file handles, respectively.  "
> ...
> 
> "With None, no redirection will occur;
> the child’s file handles will be inherited from the parent. "
> 
> We can also see that from the fact that the tail operates in the output
> above.
> 
> I trust this addresses your concerns.  These are otherwise just random
> high FDs.

A further updated commit message is in the attached.

> If you are still not convinced, we can with much less fuss ignore this
> and just have the buildnice script changed to allow 1024 FDs.  I have
> found these represented in the child Samba processes (via lsof and
> /proc/$pid/fd), and up to 480 FDs consumed at the time of my
> inspection. 

On reflection, even if we save 20 FDs or so, we are too close to our
limit so we need this raised anyway.

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

-------------- next part --------------
From fb86c8619747cee8a908de862bd1ce5eb37e9e7d Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 27 Aug 2018 21:00:58 +1200
Subject: [PATCH 1/2] autobuild: use close_fds=True to avoid *.stderr and
 *.stdout inheriting into every process

This closes fds other than 0, 1, 2.

This ensures only the correct *.stderr and *.stdout is attached, via
the stdout/stderr parameter to Popen(), but not every other FD
currently open in python at the time Popen is called.

For the tail invocation and other calls to Popen(), because fds 0, 1,
2 are still attached, these function as before.

(The unwanted inherited files would be on a random high FD, where the
program wouldn't know what to do with them, but counting towards the
process FD limit).

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13591

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
 script/autobuild.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/script/autobuild.py b/script/autobuild.py
index 9f20b65f692..6ad1941890b 100755
--- a/script/autobuild.py
+++ b/script/autobuild.py
@@ -407,7 +407,7 @@ def run_cmd(cmd, dir=".", show=None, output=False, checkfail=True):
     if show:
         do_print("Running: '%s' in '%s'" % (cmd, dir))
     if output:
-        return Popen([cmd], shell=True, stdout=PIPE, cwd=dir).communicate()[0]
+        return Popen([cmd], shell=True, stdout=PIPE, cwd=dir, close_fds=True).communicate()[0]
     elif checkfail:
         return check_call(cmd, shell=True, cwd=dir)
     else:
@@ -473,6 +473,7 @@ class builder(object):
         cwd = os.getcwd()
         os.chdir("%s/%s" % (self.sdir, self.dir))
         self.proc = Popen(self.cmd, shell=True,
+                          close_fds=True,
                           stdout=self.stdout, stderr=self.stderr, stdin=self.stdin)
         os.chdir(cwd)
         self.next += 1
@@ -615,7 +616,7 @@ class buildlist(object):
         cwd = os.getcwd()
         cmd = "tail -f *.stdout *.stderr"
         os.chdir(gitroot)
-        self.tail_proc = Popen(cmd, shell=True)
+        self.tail_proc = Popen(cmd, shell=True, close_fds=True)
         os.chdir(cwd)
 
 
-- 
2.17.1


From c3329fe2d8a31d770d79385f6aa34c46d8346fcb Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 27 Aug 2018 21:13:29 +1200
Subject: [PATCH 2/2] autobuild: Avoid subshell for tail -f invocation

This should mean one less process in the process tree, and less places to hold
FDs open.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13591

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
 script/autobuild.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/script/autobuild.py b/script/autobuild.py
index 6ad1941890b..fec64094c00 100755
--- a/script/autobuild.py
+++ b/script/autobuild.py
@@ -613,11 +613,11 @@ class buildlist(object):
             os.unlink(b.stderr_path)
 
     def start_tail(self):
-        cwd = os.getcwd()
-        cmd = "tail -f *.stdout *.stderr"
-        os.chdir(gitroot)
-        self.tail_proc = Popen(cmd, shell=True, close_fds=True)
-        os.chdir(cwd)
+        cmd = ["tail", "-f"]
+        for b in self.tlist:
+            cmd.append(b.stdout_path)
+            cmd.append(b.stderr_path)
+        self.tail_proc = Popen(cmd, close_fds=True)
 
 
 def cleanup():
-- 
2.17.1