Skip to content

Android tagets: Piped su call vs --command option #386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ptosi opened this issue May 13, 2019 · 5 comments
Closed

Android tagets: Piped su call vs --command option #386

ptosi opened this issue May 13, 2019 · 5 comments

Comments

@ptosi
Copy link
Contributor

ptosi commented May 13, 2019

This is with (Google Pixel 2):

walleye:/ # su --version
15.3:MAGISKSU (topjohnwu)

and (host):

$ adb --version
Android Debug Bridge version 1.0.40
Version 28.0.2-5303910

The way devlib.AndroidTarget.execute works relies on su and there seems to be a difference in behaviour between a call to that method and actually running a command inside of su inside of an adb shell.

I am unsure about the reason but, as an example (the device is connected through adb and there exists a /sdcard/devlib-target/perf.data file on the device):

Python 3.6.7 (default, Oct 22 2018, 11:32:17)
>>> import devlib
>>> t = devlib.AndroidTarget()
>>> print(t.execute('cd /sdcard/devlib-target && /data/local/tmp/bin/perf report', as_root=True))
incompatible file format (rerun with -v to learn more)Warning:
Kernel address maps (/proc/{kallsyms,modules}) were restricted.

Check /proc/sys/kernel/kptr_restrict before running 'perf record'.

As no suitable kallsyms nor vmlinux was found, kernel samples
can't be resolved.

Samples in kernel modules can't be resolved as well.

Error:
The - file has no samples!
# ========
# captured on: Mon May 13 18:35:09 2019
# ========
#

while

$ adb shell
walleye:/ $ su
walleye:/ # cd /sdcard/devlib-target
walleye:/sdcard/devlib-target # /data/local/tmp/bin/perf report

gives the expected result.

In fact, going through adb shell with oneliners:

$ adb shell "echo 'cd /sdcard/devlib-target && /data/local/tmp/bin/perf report' | su"
incompatible file format (rerun with -v to learn more)Warning:
Kernel address maps (/proc/{kallsyms,modules}) were restricted.

Check /proc/sys/kernel/kptr_restrict before running 'perf record'.

As no suitable kallsyms nor vmlinux was found, kernel samples
can't be resolved.

Samples in kernel modules can't be resolved as well.

Error:
The - file has no samples!
# ========
# captured on: Mon May 13 18:22:58 2019
# ========
#

while the following works:

$ adb shell "su -c 'cd /sdcard/devlib-target && /data/local/tmp/bin/perf report'"

I am puzzled by this and believe that it is partially due to how perf is doing something (notice that it's supposed to read $(pwd)/perf.data by default, but the error message refers to "the - file"). However, the fact that the behaviour of perf depends on how su is called implies that su itself behaves differently.

Now, the solution might be to use -c (which seems to be somewhat standard) but I have no idea of the consequences this might have; in particular, when using other distributions of su. Was there a reason for using echo CMD | su?

@marcbonnici
Copy link
Collaborator

This is interesting behaviour which I see with my device (nexus 5) as well.
Using: 2.82:SUPERSU and

Android Debug Bridge version 1.0.40
Version 4986621

I'm not really sure what would be causing this, the only difference I've managed to find between the environments (printing using set) is that the using su -c that USER is reported as "shell" whereas piping into su reports as "root" although both versions share the correct USER_ID=0.

I think the reason we were using the piping method was that it used to be more reliable for older versions of su however I'm not sure if that is still the case. @setrofim do you know any more information on this?

As a side note if you you are trying to use perf report in this way, (for my environment at least) supplying the -i parameter, still with a relative path, fixes the issue using either method of using su. E.g.:
adb shell "echo 'cd /sdcard/devlib-target && /data/local/tmp/bin/perf report -i perf.data' | su"

@setrofim
Copy link
Collaborator

IIRC (and it has been a while), the reason we pipe into su is that -c option could not be relied on to always be available at the time. I have no idea whether that's something we need to worry about today.

I can't think of a reason other than that for the pipe, and would be happy to make the switching to using '-c' . Though it would be have to be thoroughly tested with WA/LISA before merging.

Worst case, we can make this a configurable option on target creation.

@JaviMerino
Copy link

The -c option is not present in the su used for aosp userdebug builds:

$ adb shell su -c ls
su: invalid uid/gid '-c'

devlib 1.1.1 works fine in this setup so I have reverted to it. I guess we will need it to be have configurable option. Another option is to test whether you can connect as root from the start. In aosp builds, adb root restarts adbd as root, and every command executed afterwards is run as root and you don't need to play with su.

@marcbonnici
Copy link
Collaborator

marcbonnici commented Sep 20, 2019

Regarding using su -c vs the old echo method this should have been addressed in commit 37eef46 allowing it to fallback to the old implementation if unsupported. Is this functionality not working for you?

As for running adbd as root, if necessary we can add in a check to see if we are connected_as_root and ignore the value of as_root as we have done for the SshConnection af70581?

@JaviMerino
Copy link

You are right 37eef46 fixes it. I had only tried released versions of devlib and I missed it. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants