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

[Andrew Bartlett]

On Mon, 2018-08-27 at 22:12 +1200, Andrew Bartlett via samba-technical
wrote:
> On Mon, 2018-08-27 at 12:08 +0200, Stefan Metzmacher wrote:
> > Hi Andrew,
> > 
> > doesn't this mean we'll loose the debug output of those processes?
> > 
> > metze
> 
> I don't think so.  This is about what is passed in, because the python
> FDs are (sadly) not close-on-exec.  
> 
> The correct files (or /dev/null for stdin) are attached to the correct
> child via the stdin/stdout/stderr parameters, but the un-referenced
> files that just happen to be in an FD slot are closed.
> 
> The problem is, and I'm happy to clarify the message, that every
> *other* .stderr and .stdout file previously opened in the script was
> also passed in, on a random high FD, by virtue of the fork()
> 
> 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.

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 c3d5be64446a9324a55bf63d3e1b991d2910314d 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 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.

(These 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 b142b9e59e5fa1a2ca006e7e391e9b828e75f9b9 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