[lldb-server/android] Show more processes by relaxing some checks

[wallace]

Summary

By default platform process list only shows the processes of the current user that lldb-server can parse.
There are several problems:

• apk programs don't have an executable file. They instead use a package name as identifier. We should show them instead.
• each apk also runs under a different user. That's how android works
• because of the user permission, some files like /proc/<pid>/{environ,exe} can't be read.

This results in a very small process list.

This is a local run on my machine

(lldb) platform process list
2 matching processes were found on "remote-android"
PID    PARENT USER       TRIPLE                   NAME
====== ====== ========== ======================== ============================
23291  3177              aarch64-unknown-linux-android sh
23301  23291            aarch64-unknown-linux-android lldb-server

However, I have 700 processes running at this time.

By implementing a few fallbacks for android, I've expanded this list to 202, filtering out kernel processes, which would presumably appear in this list if the device was rooted.

(lldb) platform process list
202 matching processes were found on "remote-android"
PID    PARENT USER       TRIPLE                   NAME
====== ====== ========== ======================== ============================
...
12647  3208              aarch64-unknown-linux-android sh
12649  12647             aarch64-unknown-linux-android lldb-server
12653  982                                        com.samsung.faceservice
13185  982                                        com.samsung.vvm
15899  982                                        com.samsung.android.spay
16220  982                                        com.sec.spp.push
17126  982                                        com.sec.spp.push:RemoteDlcProcess
19772  983                                        com.android.chrome
20209  982                                        com.samsung.cmh:CMH
20380  982                                        com.google.android.inputmethod.latin
20879  982                                        com.samsung.android.oneconnect:Receiver
21212  983                                        com.tencent.mm
24459  1                 aarch64-unknown-linux-android wpa_supplicant
25974  982                                        com.samsung.android.contacts
26293  982                                        com.samsung.android.messaging
28714  982                                        com.samsung.android.dialer
31605  982                                        com.samsung.android.MtpApplication
32256  982                                        com.bezobidny

Something to notice is that the architecture is unkonwn for all apks. And that's fine, because run-as would be required to gather this information and that would make this entire functionality massively slow.

There are still several improvements to make here, like displaying actual user names, which I'll try to do in a following diff.

Note: Regarding overall apk debugging support from lldb. I'm planning on having lldb spawn lldb-server by itself with the correct user, so that everything works well. The initial lldb-server used for connecting to the remote platform can be reused for such purpose. Furthermore, eventually lldb could also launch that initial lldb-server on its own.

[wallace]

remove file accidentally included

[wallace]

• [process list] make the TRIPLE column wider

[wallace]

.

[wallace]

.

[labath]

It seems to me that the Host class is too low level to be encoding some of the android specifics into it. I think it would be better to handle these things higher up. Please see inline comments for details.

lldb/source/Host/linux/Host.cpp

175–178

Why does this fail on android exactly? Is it some seccomp thingy? I'm surprised that we're allowed to read /proc/cmdline, but not the /proc/exe link..

Anyway, I don't think that pretending that "com.my.app" is an executable file name is a good idea. Maybe it would be better to implement something on the other end. I.e., we print out argv[0] if the file name is not present?

203–208

I think we can just make this non-android specific and say that we're ok with returning the results if we were unable to fetch the environment. For instance, you could make the caller not fail hard (but just log or something) if this returns false.

234–240

Just from the perspective of the FindProcesses API, I don't think it's reasonable for it to unilaterally ignore the explicit request to limit the set of processes to return. Ideally, this decision would be made higher up (somewhere near the code that actually enables us to attach to processes of other users), and communicated here via the GetMatchAllUsers flag. However, that code doesn't exist yet. Maybe we could drop this bit for now? Or implement a flag to the "platform process list" which would allow one to see all processes? Disregarding android, it may still be interesting to see all processes of some remote system, even if one cannot attach to them. And later, once we're actually able to attach to processes of other "users" we can set it up so that this flag becomes the default for android targets?

[wallace]

lldb/source/Host/linux/Host.cpp

175–178

From what I've been seeing, /proc/cmdline is always available, even for root processes. On the other hand, /proc/exe is only available for the user that is seeing it. In fact, if you use run-as, you can see the contents of /proc/exe, but that would involve making too many slow calls (run-as is slow).

I do think that having "com.my.app" as executable name is okay, because android doesn't let you interact with an apk by any other means, unless you root your device. When using android commands, "com.my.app" is just like any other process.

I don't have any strong opinions though, so I'm okay with following your suggestion if you prefer so.

Handling the android case from the host by doing exePath = argv[0] would look like this:

• the Arg0 fallback is done inside the client when the exe path is empty
• a special additional handling would be required for cases when lldb is running on Android. With the approach of this diff, that case is covered.
• the current function, in the case of android, returns a possible empty exe path. For anything not android, empty exe paths are not allowed

what do you think?

203–208

agree!

234–240

this is a good plan. I'll implement that flag for now

[labath]

lldb/source/Host/linux/Host.cpp

175–178

Having "com.my.app" as an executable (more like, process) name is probably fine. However, having that as an executable _path_ (of type FileSpec!) is streching it too far, I believe. If this function was returning some higher-level concept of a process, which had a "name" as a separate property, then putting argv[0] there would be fine. However, for something of type FileSpec, I think it's better to have nothing rather than something that is definitely _not_ a file.

the Arg0 fallback is done inside the client when the exe path is empty

agreed, and this doesn't need to be android-specific

a special additional handling would be required for cases when lldb is running on Android. With the approach of this diff, that case is covered.

I am not sure what you mean by that. Can you elaborate?

the current function, in the case of android, returns a possible empty exe path. For anything not android, empty exe paths are not allowed

yes, and I think we should, like for the case of environment, just permit a ProcessInstanceInfo result with no executable path, regardless of the OS.

[wallace]

simplify this diff

[wallace]

i'm learning arc...

[wallace]

I removed all android-specific logic from this diff, and relaxed most of the checks inside GetProcessAndStatInfo so that more processes are displayed.

[wallace]

.

[labath]

Looks good. I don't think there's a reasonable way to test this, as these will fail only when running stuff under a different user, or on a system with a paranoid security module.

lldb/source/Host/linux/Host.cpp

193–194

We usually don't put braces around short statements like this.

[wallace]

last nit

[wallace]

format

[jankratochvil]

It has regressed on Linux Fedora 30 x86_64:

lldb-Suite :: commands/process/attach/TestProcessAttach.py
lldb-Suite :: tools/lldb-vscode/attach/TestVSCode_attach.py

122 KBDownload

[labath]

In D68289#1696654, @jankratochvil wrote:

It has regressed on Linux Fedora 30 x86_64:

lldb-Suite :: commands/process/attach/TestProcessAttach.py
lldb-Suite :: tools/lldb-vscode/attach/TestVSCode_attach.py

122 KBDownload

Should be fixed by r373925.

[labath]

Hi Walter,

unfortunately my quick-fix ended up causing problems on mac os. I believe I have a solution to that, but it's going to be slightly more involved so, I'd like to get the patch reviewed first. I've reverted your patch for the time being to get the linux build green.

[wallace]

thanks, @labath !

[labath]

I've committed my ProcessInstanceInfoMatch fix now. Feel free to recommit when you're able to check the bots for failure.

[wallace]

rebase

lldb/source/Host/linux/Host.cpp

static ArchSpec GetELFProcessCPUType(llvm::StringRef exe_path) {
   case llvm::ELF::ELFCLASS64:
     return HostInfo::GetArchitecture(HostInfo::eArchKind64);
   default:
     LLDB_LOG(log, "Unknown elf class ({0}) in file {1}", exe_class, exe_path);
     return ArchSpec();
   }
 }
 
-static bool GetProcessAndStatInfo(::pid_t pid,
-                                  ProcessInstanceInfo &process_info,
-                                  ProcessState &State, ::pid_t &tracerpid) {
-  tracerpid = 0;
-  process_info.Clear();
+static void GetProcessArgs(::pid_t pid, ProcessInstanceInfo &process_info) {
+  auto BufferOrError = getProcFile(pid, "cmdline");
+  if (!BufferOrError)
+    return;
+  std::unique_ptr<llvm::MemoryBuffer> Cmdline = std::move(*BufferOrError);
+
+  llvm::StringRef Arg0, Rest;
+  std::tie(Arg0, Rest) = Cmdline->getBuffer().split('\0');
+  process_info.SetArg0(Arg0);
+  while (!Rest.empty()) {
+    llvm::StringRef Arg;
+    std::tie(Arg, Rest) = Rest.split('\0');
+    process_info.GetArguments().AppendArgument(Arg);
+  }
+}
 
+static void GetExePathAndArch(::pid_t pid, ProcessInstanceInfo &process_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+  std::string ExePath(PATH_MAX, '\0');
 
   // We can't use getProcFile here because proc/[pid]/exe is a symbolic link.
   llvm::SmallString<64> ProcExe;
   (llvm::Twine("/proc/") + llvm::Twine(pid) + "/exe").toVector(ProcExe);
-  std::string ExePath(PATH_MAX, '\0');
 
   ssize_t len = readlink(ProcExe.c_str(), &ExePath[0], PATH_MAX);
-  if (len <= 0) {
+  if (len > 0) {
+    ExePath.resize(len);
+  } else {
     LLDB_LOG(log, "failed to read link exe link for {0}: {1}", pid,
              Status(errno, eErrorTypePOSIX));
-    return false;
+    ExePath.resize(0);
   }
-  ExePath.resize(len);
-
   // If the binary has been deleted, the link name has " (deleted)" appended.
   // Remove if there.
   llvm::StringRef PathRef = ExePath;
   PathRef.consume_back(" (deleted)");
 
+  if (!PathRef.empty()) {
+    process_info.GetExecutableFile().SetFile(PathRef, FileSpec::Style::native);
     process_info.SetArchitecture(GetELFProcessCPUType(PathRef));
+  }
+}
 
+static void GetProcessEnviron(::pid_t pid, ProcessInstanceInfo &process_info) {
   // Get the process environment.
   auto BufferOrError = getProcFile(pid, "environ");
   if (!BufferOrError)
-    return false;
-  std::unique_ptr<llvm::MemoryBuffer> Environ = std::move(*BufferOrError);
-
-  // Get the command line used to start the process.
-  BufferOrError = getProcFile(pid, "cmdline");
-  if (!BufferOrError)
-    return false;
-  std::unique_ptr<llvm::MemoryBuffer> Cmdline = std::move(*BufferOrError);
-
-  // Get User and Group IDs and get tracer pid.
-  if (!GetStatusInfo(pid, process_info, State, tracerpid))
-    return false;
-
-  process_info.SetProcessID(pid);
-  process_info.GetExecutableFile().SetFile(PathRef, FileSpec::Style::native);
+    return;
 
+  std::unique_ptr<llvm::MemoryBuffer> Environ = std::move(*BufferOrError);
   llvm::StringRef Rest = Environ->getBuffer();
   while (!Rest.empty()) {
     llvm::StringRef Var;
     std::tie(Var, Rest) = Rest.split('\0');
     process_info.GetEnvironment().insert(Var);
   }
-
-  llvm::StringRef Arg0;
-  std::tie(Arg0, Rest) = Cmdline->getBuffer().split('\0');
-  process_info.SetArg0(Arg0);
-  while (!Rest.empty()) {
-    llvm::StringRef Arg;
-    std::tie(Arg, Rest) = Rest.split('\0');
-    process_info.GetArguments().AppendArgument(Arg);
-  }
 
+static bool GetProcessAndStatInfo(::pid_t pid,
+                                  ProcessInstanceInfo &process_info,
+                                  ProcessState &State, ::pid_t &tracerpid) {
+  tracerpid = 0;
+  process_info.Clear();
+
+  process_info.SetProcessID(pid);
+
+  GetExePathAndArch(pid, process_info);
+  GetProcessArgs(pid, process_info);
+  GetProcessEnviron(pid, process_info);
+
+  // Get User and Group IDs and get tracer pid.
+  if (!GetStatusInfo(pid, process_info, State, tracerpid))
+    return false;
+
   return true;
 }
 
 uint32_t Host::FindProcesses(const ProcessInstanceInfoMatch &match_info,
                              ProcessInstanceInfoList &process_infos) {
   static const char procdir[] = "/proc/";
 
   DIR *dirproc = opendir(procdir);
   if (dirproc) {
     struct dirent *direntry = nullptr;
     const uid_t our_uid = getuid();
     const lldb::pid_t our_pid = getpid();
     bool all_users = match_info.GetMatchAllUsers();
 
     while ((direntry = readdir(dirproc)) != nullptr) {
       if (direntry->d_type != DT_DIR || !IsDirNumeric(direntry->d_name))
         continue;
 
       lldb::pid_t pid = atoi(direntry->d_name);
 
       // Skip this process.
       if (pid == our_pid)
         continue;
 
       ::pid_t tracerpid;
       ProcessState State;
       ProcessInstanceInfo process_info;