Avoid leakage of file descriptors in LLDB and LLGS

[labath]

Summary

Both LLDB and LLGS are leaking file descriptors into the debugged process. This plugs the leak by
closing the unneeded descriptors. In one case I use O_CLOEXEC, which I hope is supported on
relevant platforms. I also added a regression test and plugged a fd leak in dosep.py.

[labath]

Update main.c: remove tabs

[zturner]

This doesn't seem like the best way for the test to do this. I don't know
much about linux, but windows at least we can just have the debugger
directly figure out how many handles are open in the target process. And
this is a pretty useful thing to be able to do in a debugger, for example
if you're debugging a handle leak.

Is this not possible on linux? If it is, it would be much better to just
have the debugger enumerate the target's handles and verify there are only 3

[labath]

On linux, you can just enumerate the files in /proc/PID/fd/. However, this approach does not work on mac (afaik there is no proc filesystem there). This was the most general solution I could think of, and it should work on most posix implementations (those that we care about at least). I am aware that it will likely not work on windows. If you have a proposal how to test it, I will be happy to add it.

I agree that file handle/descriptor inspection using the debugger is a very useful feature, but I think implementing it is out of scope of what I am trying to achieve here.

[zturner]

I had hoped there was already a command in lldb that did this, but i guess
not. Can you just mark the test xfail for windows?

[labath]

Mark the test as XFAIL on windows.

[vharron]

Thanks for adding a test to confirm/protect.

LGTM If this builds/runs on OSX and Linux and doesn't cause any test regressions. (other than maybe this test on OSX)

[clayborg]

Looks good.

[ovyalov]

The CL is causing TestProcessLaunch.py regression.

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

1551 ↗

(On Diff #19307)

This statement makes TestProcesslaunch.test_io_with_dwarf to fail with SIGHUP:

runCmd: process launch -i /usr/local/google/home/ovyalov/projects/lldb/dev/lldb/test/functionalities/process_launch/input-file.txt -o /usr/local/google/home/ovyalov/projects/lldb/dev/lldb/test/functionalities/process_launch/output-test.out -e /usr/local/google/home/ovyalov/projects/lldb/dev/lldb/test/functionalities/process_launch/output-test.err
output: Process 25324 launching
Process 25324 stopped

• thread #1: tid = 25324, 0x00007ffff7ddb2d0, name = 'a.out', stop reason = signal SIGHUP frame #0: 0x00007ffff7ddb2d0

-> 0x7ffff7ddb2d0: movq %rsp, %rdi

0x7ffff7ddb2d3: callq  0x7ffff7ddea70
0x7ffff7ddb2d8: movq   %rax, %r12
0x7ffff7ddb2db: movl   0x221b17(%rip), %eax

[labath]

looking into it

[labath]

Proposed solution in D7440, please verify.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

if (pid == 0)
         // FIXME consider opening a pipe between parent/child and have this forked child
         // send log info to parent re: launch status, in place of the log lines removed here.
 
         // Start tracing this child that is about to exec.
         PTRACE(PTRACE_TRACEME, 0, nullptr, nullptr, 0, args->m_error);
         if (args->m_error.Fail())
             exit(ePtraceFailed);
 
+        // terminal has already dupped the tty descriptors to stdin/out/err.
+        // This closes original fd from which they were copied (and avoids
+        // leaking descriptors to the debugged process.
+        terminal.CloseSlaveFileDescriptor();
+
         // Do not inherit setgid powers.
         if (setgid(getgid()) != 0)
             exit(eSetGidFailed);
 
         // Attempt to have our own process group.
         if (setpgid(0, 0) != 0)
         {
             // FIXME log that this failed. This is common.

 bool
 NativeProcessLinux::DupDescriptor(const char *path, int fd, int flags)
 {
     int target_fd = open(path, flags, 0666);
 
     if (target_fd == -1)
         return false;
 
-    return (dup2(target_fd, fd) == -1) ? false : true;
+    if (dup2(target_fd, fd) == -1)
+        return false;
+
+    return (close(target_fd) == -1) ? false : true;
 }
 
 void
 NativeProcessLinux::StopMonitoringChildProcess()
 {
     if (m_monitor_thread.IsJoinable())
     {
         m_monitor_thread.Cancel();

source/Plugins/Process/Linux/ProcessMonitor.cpp

ProcessMonitor::Launch(LaunchArgs *args)
 
     // Child process.
     if (pid == 0)
     {
         // Trace this process.
         if (PTRACE(PTRACE_TRACEME, 0, NULL, NULL, 0) < 0)
             exit(ePtraceFailed);
 
+        // terminal has already dupped the tty descriptors to stdin/out/err.
+        // This closes original fd from which they were copied (and avoids
+        // leaking descriptors to the debugged process.
+        terminal.CloseSlaveFileDescriptor();
+
         // Do not inherit setgid powers.
         if (setgid(getgid()) != 0)
             exit(eSetGidFailed);
 
         // Let us have our own process group.
         setpgid(0, 0);
 
         // Dup file descriptors if needed.

 bool
 ProcessMonitor::DupDescriptor(const char *path, int fd, int flags)
 {
     int target_fd = open(path, flags, 0666);
 
     if (target_fd == -1)
         return false;
 
-    return (dup2(target_fd, fd) == -1) ? false : true;
+    if (dup2(target_fd, fd) == -1)
+        return false;
+
+    return (close(target_fd) == -1) ? false : true;
 }
 
 void
 ProcessMonitor::StopMonitoringChildProcess()
 {
     if (m_monitor_thread.IsJoinable())
     {
         m_monitor_thread.Cancel();

source/Target/ProcessLaunchInfo.cpp

if (GetFileActionForFD(STDIN_FILENO) == NULL ||
             }
 
             if (default_to_use_pty && (!in_path || !out_path || !err_path))
             {
                 if (log)
                     log->Printf ("ProcessLaunchInfo::%s default_to_use_pty is set, and at least one stdin/stderr/stdout is unset, so generating a pty to use for it",
                                  __FUNCTION__);
 
-                if (m_pty->OpenFirstAvailableMaster(O_RDWR| O_NOCTTY, NULL, 0))
+                if (m_pty->OpenFirstAvailableMaster(O_RDWR | O_NOCTTY | O_CLOEXEC, NULL, 0))
                 {
                     const char *slave_path = m_pty->GetSlaveName(NULL, 0);
 
                     // Only use the slave tty if we don't have anything specified for
                     // input and don't have an action for stdin
                     if (!in_path && GetFileActionForFD(STDIN_FILENO) == NULL)
                     {
                         AppendOpenFileAction(STDIN_FILENO, slave_path, true, false);

test/dosep.py

 
 # Status codes for running command with timeout.
 eTimedOut, ePassed, eFailed = 124, 0, 1
 
 def call_with_timeout(command, timeout):
     """Run command with a timeout if possible."""
     if timeout_command and timeout != "0":
         return subprocess.call([timeout_command, timeout] + command,
-                               stdin=subprocess.PIPE)
-    return (ePassed if subprocess.call(command, stdin=subprocess.PIPE) == 0
+                               stdin=subprocess.PIPE, close_fds=True)
+    return (ePassed if subprocess.call(command, stdin=subprocess.PIPE, close_fds=True) == 0
             else eFailed)
 
 def process_dir(root, files, test_root, dotest_options):
     """Examine a directory for tests, and invoke any found within it."""
     timed_out = []
     failed = []
     passed = []
     for name in files:

test/functionalities/avoids-fd-leak/Makefile

+LEVEL = ../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

test/functionalities/avoids-fd-leak/TestFdLeak.py

+"""
+Test whether a process started by lldb has no extra file descriptors open.
+"""
+
+import os
+import unittest2
+import lldb
+from lldbtest import *
+import lldbutil
+
+class AvoidsFdLeakTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @expectedFailureWindows("The check for descriptor leakage needs to be implemented differently")
+    def test_fd_leak (self):
+        self.buildDefault()
+        exe = os.path.join (os.getcwd(), "a.out")
+
+        target = self.dbg.CreateTarget(exe)
+
+        process = target.LaunchSimple (None, None, self.get_process_working_directory())
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        self.assertTrue(process.GetState() == lldb.eStateExited)
+        self.assertTrue(process.GetExitStatus() == 0)
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/functionalities/avoids-fd-leak/main.c

+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
+
+int
+main (int argc, char const **argv)
+{
+    struct stat buf;
+    int i;
+
+    // Make sure stdin/stdout/stderr exist.
+    for (i = 0; i <= 2; ++i) {
+        if (fstat(i, &buf) != 0)
+            return 1;
+    }
+
+    // Make sure no other file descriptors are open.
+    for (i = 3; i <= 256; ++i) {
+        if (fstat(i, &buf) == 0 || errno != EBADF) {
+            fprintf(stderr, "File descriptor %d is open.\n", i);
+            return 2;
+        }
+    }
+
+    return 0;
+}