This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wallace on Oct 1 2019, 12:17 PM.

Details

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.

Diff Detail

Event Timeline

wallace created this revision.Oct 1 2019, 12:17 PM
wallace updated this revision to Diff 222671.Oct 1 2019, 12:19 PM

remove file accidentally included

wallace updated this revision to Diff 222673.Oct 1 2019, 12:37 PM
  • [process list] make the TRIPLE column wider
wallace edited the summary of this revision. (Show Details)Oct 1 2019, 12:38 PM
labath added a comment.Oct 2 2019, 1:59 AM

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 marked 3 inline comments as done.Oct 2 2019, 10:36 AM
wallace added inline comments.
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 added inline comments.Oct 2 2019, 11:08 AM
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 updated this revision to Diff 222903.Oct 2 2019, 1:40 PM

simplify this diff

wallace updated this revision to Diff 222905.Oct 2 2019, 1:43 PM

i'm learning arc...

wallace retitled this revision from [lldb-server/android] Show more processes and package name when necessary to [lldb-server/android] Show more processes by relaxing some checks.Oct 2 2019, 1:52 PM

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

labath accepted this revision.Oct 3 2019, 4:28 AM

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.

This revision is now accepted and ready to land.Oct 3 2019, 4:28 AM
wallace updated this revision to Diff 223235.Oct 4 2019, 9:30 AM

last nit

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 9:34 AM

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

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

Should be fixed by r373925.

labath added a comment.Oct 8 2019, 2:07 AM

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.

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

wallace reopened this revision.Oct 11 2019, 12:41 PM
This revision is now accepted and ready to land.Oct 11 2019, 12:41 PM