Add test for denied process attach by pid and fix found bugs in Process/ProcessPOSIX.cpp

[ovyalov]

Summary

New test test/functionalities/process_attach/attach_denied covers the case when attach is denied by potential inferior.
This test uncovered next issues that I fixed along as well:

• lldbtest calls SBProcess::Kill for every inferior within test's tearDown. If attach fails ProcessPOSIX::DoDestroy calls m_monitor->Kill() which sends SIGKILL to pid 0, i.e. killing all processes in the current process group. Added check for invalid pid in ProcessPOSIX::DoDestroy
• Modified ProcessPOSIX::DoDestroy made test to fail since Kill returns an error. Changed Target/Process.cpp in order to transition process to exited state in case of attach failure - so, DoDestroy exits properly since HasExited() returns true.

[ovyalov]

Removed unused imports from TestAttachDenied.py.

[ovyalov]

Hi Ed,

since you monitor POSIX code changes let me add you as reviewer.

[ovyalov]

• Added synchronization between test and inferior via named pipe.
• Restricted test run conditions - either Darwin or Linux.

Please take a look.

[emaste]

For future would you provide full context, i.e. diff -U999999 if manually uploading diffs?

This looks OK to me, but as I think we lack an appropriate interface in FreeBSD right now I can't test, so hope one of the Linux or Darwin developers can comment.

[ovyalov]

Thanks for suggestion - re-did patch with full context.

Yes, I tried to run the test on FreeBSD but prctl isn't available - so, for now BSD will be skipped.

[emaste]

Some of my FreeBSD colleagues recommended trigger the failure by trying to attach to a process that already has a debugger attached, as that's the common cause of the failure we're handling here anyway.

Perhaps attach_denied/main.c can fork(), PT_ATTACH to the child, and then return the child's pid for LLDB to try to attach.

[ovyalov]

I reworked test according to Ed's suggestion with fork to make the test to cover OSX, Linux and FreeBSD.
Along the way FreeBSD/ProcessMonitor bug was discovered which led LLDB to hang when Attach fails - fixed this.
Checked against OSX 10.9.5, Ubuntu 14.0.4 and FreeBSD 10 - no regressions found.

Please take another look.

[ovyalov]

Removed unneeded file from patch.

[emaste]

overall lgtm

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

1143

we probably ought to clean this up to just return; now - the goto is just extra obfuscation

test/functionalities/process_attach/attach_denied/main.cpp

61

just 1 probably. -ve return values from main end up with implementation-specific results.

[ovyalov]

Fixed according to your suggestions.

Thank you for the review!

[emaste]

LGTM.

[clayborg]

lgtm

[emaste]

ping?

[emaste]

rL222372

source/Plugins/Process/FreeBSD/ProcessMonitor.h

private:
     };
 
     void
     StartAttachOpThread(AttachArgs *args, lldb_private::Error &error);
 
     static void *
     AttachOpThread(void *args);
 
-    static bool
+    static void
     Attach(AttachArgs *args);
 
     static void
     ServeOperation(OperationArgs *args);
 
     static bool
     DupDescriptor(const char *path, int fd, int flags);
 

source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

ProcessMonitor::StartAttachOpThread(AttachArgs *args, lldb_private::Error &error)
     m_operation_thread = ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args, &error);
 }
 
 void *
 ProcessMonitor::AttachOpThread(void *arg)
 {
     AttachArgs *args = static_cast<AttachArgs*>(arg);
 
-    if (!Attach(args))
-        return NULL;
+    Attach(args);
 
     ServeOperation(args);
     return NULL;
 }
 
-bool
+void
 ProcessMonitor::Attach(AttachArgs *args)
 {
     lldb::pid_t pid = args->m_pid;
 
     ProcessMonitor *monitor = args->m_monitor;
     ProcessFreeBSD &process = monitor->GetProcess();
 
     if (pid <= 1)
     {
         args->m_error.SetErrorToGenericError();
         args->m_error.SetErrorString("Attaching to process 1 is not allowed.");
-        goto FINISH;
+        return;
     }
 
     // Attach to the requested process.
     if (PTRACE(PT_ATTACH, pid, NULL, 0) < 0)
     {
         args->m_error.SetErrorToErrno();
-        goto FINISH;
+        return;
     }
 
     int status;
     if ((status = waitpid(pid, NULL, 0)) < 0)
     {
         args->m_error.SetErrorToErrno();
-        goto FINISH;
+        return;
     }
 
     process.SendMessage(ProcessMessage::Attach(pid));
-
-FINISH:
-    return args->m_error.Success();
 }
 
 size_t
 ProcessMonitor::GetCurrentThreadIDs(std::vector<lldb::tid_t>&thread_ids)
 {
     lwpid_t *tids;
     int tdcnt;
 

source/Plugins/Process/POSIX/ProcessPOSIX.cpp

 ProcessPOSIX::DoDestroy()
 {
     Error error;
 
     if (!HasExited())
     {
         assert(m_monitor);
         m_exit_now = true;
+        if (GetID() == LLDB_INVALID_PROCESS_ID)
+        {
+            error.SetErrorString("invalid process id");
+            return error;
+        }
         if (!m_monitor->Kill())
         {
             error.SetErrorToErrno();
             return error;
         }
 
         SetPrivateState(eStateExited);
     }

source/Target/Process.cpp

if (attach_pid != LLDB_INVALID_PROCESS_ID)
             {
                 
                 SetNextEventAction(new Process::AttachCompletionHandler(this, attach_info.GetResumeCount()));
                 StartPrivateStateThread();
             }
             else
             {
                 if (GetID() != LLDB_INVALID_PROCESS_ID)
-                {
                     SetID (LLDB_INVALID_PROCESS_ID);
-                    const char *error_string = error.AsCString();
-                    if (error_string == NULL)
-                        error_string = "attach failed";
+
 
-                    SetExitStatus(-1, error_string);
-                }
-            }
-        }
-    }
     return error;
 }
 
 void
 Process::CompleteAttach ()
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
     if (log)

test/functionalities/process_attach/attach_denied/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+
+EXE := AttachDenied
+
+include $(LEVEL)/Makefile.rules

test/functionalities/process_attach/attach_denied/TestAttachDenied.py

+"""
+Test denied process attach.
+"""
+
+import os
+import shutil
+import tempfile
+import unittest2
+import lldb
+from lldbtest import *
+
+exe_name = 'AttachDenied'  # Must match Makefile
+
+class AttachDeniedTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIfWindows
+    def test_attach_to_process_by_id_denied(self):
+        """Test attach by process id denied"""
+
+        self.buildDefault()
+        exe = os.path.join(os.getcwd(), exe_name)
+
+        temp_dir = tempfile.mkdtemp()
+        self.addTearDownHook(lambda: shutil.rmtree(temp_dir))
+
+        # Use named pipe as a synchronization point between test and inferior.
+        pid_pipe_path = os.path.join(temp_dir, "pid_pipe")
+        os.mkfifo(pid_pipe_path)
+
+        # Spawn a new process
+        popen = self.spawnSubprocess(exe, [pid_pipe_path])
+        self.addTearDownHook(self.cleanupSubprocesses)
+
+        pid_pipe = open(pid_pipe_path, 'r')
+        self.addTearDownHook(lambda: pid_pipe.close())
+        pid = pid_pipe.read()
+
+        self.expect('process attach -p ' + pid,
+                    startstr = 'error: attach failed:',
+                    error = True)
+
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

test/functionalities/process_attach/attach_denied/main.cpp

+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/types.h>
+#include <sys/ptrace.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+#if defined(PTRACE_ATTACH)
+#define ATTACH_REQUEST PTRACE_ATTACH
+#define DETACH_REQUEST PTRACE_DETACH
+#elif defined(PT_ATTACH)
+#define ATTACH_REQUEST PT_ATTACH
+#define DETACH_REQUEST PT_DETACH
+#else
+#error "Unsupported platform"
+#endif
+
+bool writePid (const char* file_name, const pid_t pid)
+{
+    int fd = open (file_name, O_WRONLY);
+    if (fd == -1)
+    {
+        fprintf (stderr, "open(%s) failed: %s\n", file_name, strerror (errno));
+        return false;
+    }
+    char buffer[64];
+    snprintf (buffer, sizeof(buffer), "%ld", (long)pid);
+
+    bool res = true;
+    if (write (fd, buffer, strlen (buffer)) == -1)
+    {
+        fprintf (stderr, "write(%s) failed: %s\n", buffer, strerror (errno));
+        res = false;
+    }
+    close (fd);
+    return res;
+}
+
+void sigterm_handler (int)
+{
+}
+
+int main (int argc, char const *argv[])
+{
+    if (argc < 2)
+    {
+        fprintf (stderr, "invalid number of command line arguments\n");
+        return 1;
+    }
+
+    const pid_t pid = fork ();
+    if (pid == -1)
+    {
+        fprintf (stderr, "fork failed: %s\n", strerror (errno));
+        return 1;
+    }
+
+    if (pid > 0)
+    {
+        // Make pause call to return when SIGTERM is received.
+        signal (SIGTERM, sigterm_handler);
+        if (ptrace (ATTACH_REQUEST, pid, NULL, 0) == -1)
+        {
+            fprintf (stderr, "ptrace(ATTACH) failed: %s\n", strerror (errno));
+        }
+        else
+        {
+            if (writePid (argv[1], pid))
+                pause ();  // Waiting for the debugger trying attach to the child.
+
+            if (ptrace (DETACH_REQUEST, pid, NULL, 0) != 0)
+                fprintf (stderr, "ptrace(DETACH) failed: %s\n", strerror (errno));
+        }
+
+        kill (pid, SIGTERM);
+        int status = 0;
+        if (waitpid (pid, &status, 0) == -1)
+            fprintf (stderr, "waitpid failed: %s\n", strerror (errno));
+    }
+    else
+    {
+        // child inferior.
+        pause ();
+    }
+
+    printf ("Exiting now\n");
+    return 0;
+}