[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

Fri Oct 18 01:34:20 PDT 2019

labath added a comment.

Thank you for doing the investigation, and the detailed summary. I'm going to respond inline.

In D68968#1713788 <https://reviews.llvm.org/D68968#1713788>, @wallace wrote:

> Thanks for your feedback and to @clayborg  for an offline discussion we had.
>
> I'm going to add three new attributes to the ProcessInfo class, with corresponding parameters in the gdb-remote packet
>
> - display_name: This is a custom display name that the server can set for a given process. This will supersede any other other attribute in the GetProcessName call. There are interesting cases that are worth mentioning for Android:
>   - Kernel threads and system processes are displayed like [kworker/u:0] [khubd] by ps on Android. At least on modern devices ps.c identify those processes as the ones with unreadable /proc/<pid>/cmdline and unreadable /proc/<pid>/exe. The name is gotten from /proc/<pid>/stat. There's some generic information about this here https://unix.stackexchange.com/questions/22121/what-do-the-brackets-around-processes-mean. In this case both the actual executable and args are not known, so it's better to leave those fields empty and store the display name in this new field.
>   - Custom apk process names: apks can launch processes and subprocesses with custom names. The custom name has the form com.example.app:custom_name. It's always that way and it's documented here https://developer.android.com/guide/topics/manifest/application-element#proc (go to the android:process section). A given apk can have several subprocesses, each one with a different custom name, e.g. com.samsung.services:Core, com.samsung.services:Monitor, etc. In this case, the package name is known and a display_name field would be useful to store the entire custom name.

I think this is an excellent idea. It makes it clear that this is a separate concept, and that one should not rely on this being identical to any other concept (exe path, bundle it, etc.)

> 
> 
> - bundle_id: This will be exactly the apk package name or the iOS bundle name inferred by the server. I plan to make a best effort guess for modern android devices, leaving old devices as cases to implement as needed. I plan to use `pm list packages` for this, given that this is the only reliable way to get all the apk names. Besides, I'll check /proc<pid>/stat for the process name and not arg0. It seems that /stat is not modifiable the same way arg0 is.

Having a bundle_id field in this struct seems fine. I'm much less enthusiastic about lldb-server "inferring" its contents. In fact, after reading the android docs you linked above, I'm starting to become opposed to the idea. I'll copy the interesting bits of that document:

  android:process
      The name of a process where all components of the application should run. Each component can override this default by setting its own process attribute.

      By default, Android creates a process for an application when the first of its components needs to run.

The way I read this is that this gives an application a fully documented way of changing it's process name to _anything_ it wants, including the name of another package (or at least, something that looks very similar to it). The app doesn't even have to go native to scribble into some random bytes in memory (which could be considered unsupported). I also think that going for /proc/<pid>/stat is a downgrade, as the name there comes from /proc/<pid>/comm, which is a file that can be written to by anyone with appropriate permissions (as opposed to argv[0], which requires permissions to write to the memory of that process -- a harder thing to come by). It is also limited to 16 characters, so it likely will not contain the full package name.

What's the end goal here? My impression was that you're trying to to make "platform process list" output useful on android, but it seems to me like this is already achieved by having the "display name" field. Can we postpone this part until we actually need it? I'm hoping that at that point it will become clearer whether these hac^H^Heuristics are really needed, or if there's another way to achieve the same goal. For instance when running doing a "process attach PID", we can (reliably) get the user id that PID runs under, and then use "pm list" to tie that user id to a package name that can be given to the "run-as" command -- no need to guess something in the implementation of GetProcessInfo.

> 
> 
> - bundle_path: On android this is the path the apk path. This will also be useful for iOS, because app bundles are also stored somewhere. Fortunately, the command `pm list packages -f`, shows the list of apks along with their corresponding paths on disk. This command doesn't work on old devices, but still, I wouldn't prioritize them now.

Likewise, the bundle_path concept seems fine, but I have doubts about the methods used to retrieve it, particularly as having two different apps share the same process is a legitimate and recommended way of reducing the memory footprint ("By setting this attribute to a process name that's shared with another application, you can arrange for components of both applications to run in the same process").

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68968/new/

https://reviews.llvm.org/D68968