[lldb-vscode] Fix two issues with runInTerminal test.

[jgorbe]

Summary

With ptrace_scope = 1 the kernel only allows tracing descendants of a process.
When using runInTerminal, the target process is not launched by the debugger,
so we need to modify LaunchRunInTerminal to explicitly allow tracing.
This should fix a problem reported in https://reviews.llvm.org/D84974#3903716

Also, remove a special case from the launch method in the lldbvscode_testcase
test harness. The existing test was using stopOnEntry, so the first stop didn't
happen at the expected breakpoint unless the harness did configurationDone
first.

[rupprecht]

lldb/tools/lldb-vscode/lldb-vscode.cpp

3163

https://manpages.debian.org/bullseye/manpages-dev/prctl.2.en.html#PR_SET_PTRACER

It would be nice if we could avoid PR_SET_PTRACER_ANY and set the pid directly. (Maybe you will need an additional flag, along with --comm-file and --launch-target?). Using PR_SET_PTRACER_ANY effectively circumvents the system's ptrace policy, IIUC.

[wallace]

wow, nice improvement. I don't have anything else to add besides what @rupprecht said

[jgorbe]

Added a --debugger-pid flag that needs to be passed with --launch-target so
that we can restrict PR_SET_PTRACER to the main lldb-vscode instance instead of
using PR_SET_PTRACER_ANY.

[rupprecht]

The pid plumbing looks fine for the happy case, but I think we could be more lenient if (for whatever reason) the pid flag isn't being set on non-Linux systems that won't actually be using it. Even on a Linux system, this is only needed if ptrace_scope != 0, so if the pid isn't being provided there, we could just skip the prctl call and hope the user's system has a value of 0 for that.

(Also, 0 is a weird value, we should probably just reject that)

lldb/tools/lldb-vscode/JSONUtils.cpp

1119

Only pass this if != 0

(or get fancy and use a std::optional or whatever)

lldb/tools/lldb-vscode/lldb-vscode.cpp

1569

unsigned long -> lldb::pid_t

3163

Only invoke this if debugger_pid != 0

3262

StringRef is usually used for parsing strings as numbers, something like:

unsigned long pid = 0;
  llvm::StringRef debugger_pid_value = debugger_pid->getValue())
  if (debugger_pid_value.getAsInteger(10, pid)) {
    llvm::errs() << ...
  }
}

[jgorbe]

Addressed review comments.

• Use lldb::pid_t instead of unsigned long, and LLDB_INVALID_PROCESS_ID instead of 0 in the cases where the PID is missing.
• Only pass the --debugger-pid flag if we're on Linux and we have a debugger PID. I don't think we need to be particularly lenient with flag parsing, because IIUC the command lines for the launcher should only ever be built by lldb-vscode itself and not by hand, but it can be confusing for users in other platforms to see --debugger-pid 0 in the command line on the terminal.
• Changed parsing of the --debugger-pid numeric value to use llvm::StringRef.

lldb/tools/lldb-vscode/JSONUtils.cpp

1119

Done. Given that I've changed the debugger_pid argument to an lldb::pid_t I'm using LLDB_INVALID_PROCESS_ID as a sentinel value.

lldb/tools/lldb-vscode/lldb-vscode.cpp

3163

Done.

3262

Thanks! Once this patch lands we should consider changing how the port flag is parsed (just a few lines after this block) to match this style.

[rupprecht]

Thanks!

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py

def launch(self, program=None, args=None, cwd=None, env=None,
             postRunCommands=postRunCommands)
 
         if expectFailure:
             return response
 
         if not (response and response['success']):
             self.assertTrue(response['success'],
                             'launch failed (%s)' % (response['message']))
-        # We need to trigger a request_configurationDone after we've successfully
-        # attached a runInTerminal process to finish initialization.
-        if runInTerminal:
-            self.vscode.request_configurationDone()
         return response
 
 
     def build_and_launch(self, program, args=None, cwd=None, env=None,
                          stopOnEntry=False, disableASLR=True,
                          disableSTDIO=False, shellExpandArguments=False,
                          trace=False, initCommands=None, preRunCommands=None,
                          stopCommands=None, exitCommands=None,

lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py

def test_runInTerminal(self):
             return
         '''
             Tests the "runInTerminal" reverse request. It makes sure that the IDE can
             launch the inferior with the correct environment variables and arguments.
         '''
         program = self.getBuildArtifact("a.out")
         source = 'main.c'
         self.build_and_launch(
-            program, stopOnEntry=True, runInTerminal=True, args=["foobar"],
-            env=["FOO=bar"])
+            program, runInTerminal=True, args=["foobar"], env=["FOO=bar"])
 
         breakpoint_line = line_number(source, '// breakpoint')
 
         self.set_source_breakpoints(source, [breakpoint_line])
         self.continue_to_next_stop()
 
         # We verify we actually stopped inside the loop
         counter = int(self.vscode.get_local_variable_value('counter'))
class TestVSCode_runInTerminal(lldbvscode_testcase.VSCodeTestCaseBase):
     @skipIfWindows
     @skipIfRemote
     @skipIf(archs=no_match(['x86_64']))
     def test_runInTerminalInvalidTarget(self):
         if not self.isTestSupported():
             return
         self.build_and_create_debug_adaptor()
         response = self.launch(
-            "INVALIDPROGRAM", stopOnEntry=True, runInTerminal=True, args=["foobar"], env=["FOO=bar"], expectFailure=True)
+            "INVALIDPROGRAM", runInTerminal=True, args=["foobar"], env=["FOO=bar"], expectFailure=True)
         self.assertFalse(response['success'])
         self.assertIn("Could not create a target for a program 'INVALIDPROGRAM': unable to find executable",
             response['message'])
 
     @skipIfWindows
     @skipIfRemote
     @skipIf(archs=no_match(['x86_64']))
     def test_missingArgInRunInTerminalLauncher(self):

lldb/tools/lldb-vscode/JSONUtils.h

 ///
 /// \param[in] debug_adaptor_path
 ///     Path to the current debug adaptor. It will be used to delegate the
 ///     launch of the target.
 ///
 /// \param[in] comm_file
 ///     The fifo file used to communicate the with the target launcher.
 ///
+/// \param[in] debugger_pid
+///     The PID of the lldb-vscode instance that will attach to the target. The
+///     launcher uses it on Linux tell the kernel that it should allow the
+///     debugger process to attach.
+///
 /// \return
 ///     A "runInTerminal" JSON object that follows the specification outlined by
 ///     Microsoft.
 llvm::json::Object
 CreateRunInTerminalReverseRequest(const llvm::json::Object &launch_request,
                                   llvm::StringRef debug_adaptor_path,
-                                  llvm::StringRef comm_file);
+                                  llvm::StringRef comm_file,
+                                  lldb::pid_t debugger_pid);
 
 /// Create a "Terminated" JSON object that contains statistics
 ///
 /// \return
 ///     A body JSON object with debug info and breakpoint info
 llvm::json::Object CreateTerminatedEventObject();
 
 /// Convert a given JSON object to a string.
 std::string JSONToString(const llvm::json::Value &json);
 
 } // namespace lldb_vscode
 
 #endif

lldb/tools/lldb-vscode/JSONUtils.cpp

llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit unit) {
   return llvm::json::Value(std::move(object));
 }
 
 /// See
 /// https://microsoft.github.io/debug-adapter-protocol/specification#Reverse_Requests_RunInTerminal
 llvm::json::Object
 CreateRunInTerminalReverseRequest(const llvm::json::Object &launch_request,
                                   llvm::StringRef debug_adaptor_path,
-                                  llvm::StringRef comm_file) {
+                                  llvm::StringRef comm_file,
+                                  lldb::pid_t debugger_pid) {
   llvm::json::Object reverse_request;
   reverse_request.try_emplace("type", "request");
   reverse_request.try_emplace("command", "runInTerminal");
 
   llvm::json::Object run_in_terminal_args;
   // This indicates the IDE to open an embedded terminal, instead of opening the
   // terminal in a new window.
   run_in_terminal_args.try_emplace("kind", "integrated");
 
   auto launch_request_arguments = launch_request.getObject("arguments");
   // The program path must be the first entry in the "args" field
   std::vector<std::string> args = {
-      debug_adaptor_path.str(), "--comm-file", comm_file.str(),
-      "--launch-target", GetString(launch_request_arguments, "program").str()};
+      debug_adaptor_path.str(), "--comm-file", comm_file.str()};
+  if (debugger_pid != LLDB_INVALID_PROCESS_ID) {
+    args.push_back("--debugger-pid");
+    args.push_back(std::to_string(debugger_pid));
+  }
+  args.push_back("--launch-target");
+  args.push_back(GetString(launch_request_arguments, "program").str());
   std::vector<std::string> target_args =
       GetStrings(launch_request_arguments, "args");
   args.insert(args.end(), target_args.begin(), target_args.end());
   run_in_terminal_args.try_emplace("args", args);
 
   const auto cwd = GetString(launch_request_arguments, "cwd");
   if (!cwd.empty())
     run_in_terminal_args.try_emplace("cwd", cwd);

lldb/tools/lldb-vscode/Options.td

 def: Separate<["-"], "p">,
   Alias<port>,
   HelpText<"Alias for --port">;
 
 def launch_target: Separate<["--", "-"], "launch-target">,
   MetaVarName<"<target>">,
   HelpText<"Launch a target for the launchInTerminal request. Any argument "
     "provided after this one will be passed to the target. The parameter "
-    "--comm-files-prefix must also be specified.">;
+    "--comm-file must also be specified.">;
 
 def comm_file: Separate<["--", "-"], "comm-file">,
   MetaVarName<"<file>">,
-  HelpText<"The fifo file used to communicate the with the debug adaptor"
+  HelpText<"The fifo file used to communicate the with the debug adaptor "
     "when using --launch-target.">;
+
+def debugger_pid: Separate<["--", "-"], "debugger-pid">,
+  MetaVarName<"<pid>">,
+  HelpText<"The PID of the lldb-vscode instance that sent the launchInTerminal "
+    "request when using --launch-target.">;

lldb/tools/lldb-vscode/lldb-vscode.cpp

 #undef GetObject
 #include <io.h>
 #else
 #include <netinet/in.h>
 #include <sys/socket.h>
 #include <unistd.h>
 #endif
 
+#if defined(__linux__)
+#include <sys/prctl.h>
+#endif
+
 #include <algorithm>
 #include <chrono>
 #include <fstream>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <set>
 #include <sstream>
llvm::Error request_runInTerminal(const llvm::json::Object &launch_request) {
   llvm::Expected<std::shared_ptr<FifoFile>> comm_file_or_err =
       CreateRunInTerminalCommFile();
   if (!comm_file_or_err)
     return comm_file_or_err.takeError();
   FifoFile &comm_file = *comm_file_or_err.get();
 
   RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path);
 
+  lldb::pid_t debugger_pid = LLDB_INVALID_PROCESS_ID;
+#if !defined(_WIN32)
+  debugger_pid = getpid();
+#endif
   llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
-      launch_request, g_vsc.debug_adaptor_path, comm_file.m_path);
+      launch_request, g_vsc.debug_adaptor_path, comm_file.m_path, debugger_pid);
   llvm::json::Object reverse_response;
   lldb_vscode::PacketStatus status =
       g_vsc.SendReverseRequest(reverse_request, reverse_response);
   if (status != lldb_vscode::PacketStatus::Success)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Process cannot be launched by the IDE. %s",
                                    comm_channel.GetLauncherError().c_str());
 

 //
 // Once attached and resumed, the launcher will exec and become the program
 // specified by --launch-target, which is the original target the
 // user wanted to run.
 //
 // In case of errors launching the target, a suitable error message will be
 // emitted to the debug adaptor.
 void LaunchRunInTerminalTarget(llvm::opt::Arg &target_arg,
-                               llvm::StringRef comm_file, char *argv[]) {
+                               llvm::StringRef comm_file,
+                               lldb::pid_t debugger_pid, char *argv[]) {
 #if defined(_WIN32)
   llvm::errs() << "runInTerminal is only supported on POSIX systems\n";
   exit(EXIT_FAILURE);
 #else
+
+  // On Linux with the Yama security module enabled, a process can only attach
+  // to its descendants by default. In the runInTerminal case the target
+  // process is launched by the client so we need to allow tracing explicitly.
+#if defined(__linux__)
+  if (debugger_pid != LLDB_INVALID_PROCESS_ID)
+    (void)prctl(PR_SET_PTRACER, debugger_pid, 0, 0, 0);
+#endif
+
   RunInTerminalLauncherCommChannel comm_channel(comm_file);
   if (llvm::Error err = comm_channel.NotifyPid()) {
     llvm::errs() << llvm::toString(std::move(err)) << "\n";
     exit(EXIT_FAILURE);
   }
 
   // We will wait to be attached with a timeout. We don't wait indefinitely
   // using a signal to prevent being paused forever.
int main(int argc, char *argv[]) {
 
   if (input_args.hasArg(OPT_help)) {
     printHelp(T, llvm::sys::path::filename(argv[0]));
     return EXIT_SUCCESS;
   }
 
   if (llvm::opt::Arg *target_arg = input_args.getLastArg(OPT_launch_target)) {
     if (llvm::opt::Arg *comm_file = input_args.getLastArg(OPT_comm_file)) {
+      lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
+      llvm::opt::Arg *debugger_pid = input_args.getLastArg(OPT_debugger_pid);
+      if (debugger_pid) {
+        llvm::StringRef debugger_pid_value = debugger_pid->getValue();
+        if (debugger_pid_value.getAsInteger(10, pid)) {
+          llvm::errs() << "'" << debugger_pid_value << "' is not a valid "
+                          "PID\n";
+          return EXIT_FAILURE;
+        }
+      }
       int target_args_pos = argc;
       for (int i = 0; i < argc; i++)
         if (strcmp(argv[i], "--launch-target") == 0) {
           target_args_pos = i + 1;
           break;
         }
-      LaunchRunInTerminalTarget(*target_arg, comm_file->getValue(),
+      LaunchRunInTerminalTarget(*target_arg, comm_file->getValue(), pid,
                                 argv + target_args_pos);
     } else {
       llvm::errs() << "\"--launch-target\" requires \"--comm-file\" to be "
                       "specified\n";
       return EXIT_FAILURE;
     }
   }