1525401 - Magisk su 0 id will hang on Magisk higher than 17.3

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement, P3)

Description

[Florin Strugariu [:Bebe]]

If Magisk version is higher than 17.3 adb.py will hang when executing su 0 id.

This blocks any unit testing locally

Comment 1

[Florin Strugariu [:Bebe]]

:bc I know you fixed a similar issue do you have time to take a look?

running su -c id returns:

mozdevice.adb.ADBProcessError: args: adb wait-for-device shell su -c id; echo adb_returncode=$?, exitcode: 1, stdout: uid=0(root) gid=0(root) groups=0(root) context=u:r:magisk:s0

annd su 0 id hangs

Comment 2

[Florin Strugariu [:Bebe]]

this is what I get in console:

$ adb shell su -c id
uid=0(root) gid=0(root) groups=0(root) context=u:r:magisk:s0

Comment 3

[Florin Strugariu [:Bebe]]

and:

$ adb shell su -c id; echo adb_returncode=$?
uid=0(root) gid=0(root) groups=0(root) context=u:r:magisk:s0
adb_returncode=0

Not sure why we get exitcode 1 in python

Comment 4

[Bob Clary [:bc] (inactive)]

The fact that is has a non-zero exit code for su -c id causes it to fall back to checking su 0 id which hits the hang.

I would suggest either using an older version, I have https://github.com/topjohnwu/Magisk/releases/tag/v16.0 installed, or opening an issue with https://github.com/topjohnwu/Magisk/issues. Failing that, a different rooting method could be used. Is there a need for a version of 17.3 or later?

What type of device and version of android are you using?

(In reply to Florin Strugariu [:Bebe] from comment #3)

and:

adb shell su -c id;echo $?
uid=0(root) gid=0(root) groups=0(root) context=u:r:magisk:s0
0

Not sure why we get exitcode 1 in python

That isn't doing what you think it is doing. The echo $? is being executed on your machine, not the device. It is effectively doing

adb shell su -c id
echo $?

Comment 5

[Bob Clary [:bc] (inactive)]

chartjes and I went through a pdb session on this with android 9 and the latest magisk. I have some clues but I'll need to upgrade my magisk and possible android to 9 to reproduce. I'll look asap as soon as I can afford to hork my device later today or tomorrow.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-1525401.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57470f82cbc42509a29bf23e2aa96a1e29aa0825

It turns out that the first time we call ADBDevice.shell_output("su -c id") it will succeed in returning the appropriate output but the exitcode will be 1 since selinux is still enforcing. We can work around it by using ADBProcess directly to attempt to set selinux to permissive.

I've also changed the failure messages to include the exception though it wouldn't have been that much more informative in this case. Without setting selinux permissive prior to the check, the error messages would have read

Check for su -c failed: args: adb wait-for-device shell su -c id; echo adb_returncode=$?, exitcode: 1, stdout: uid=0(root) gid=0(root) groups=0(root) context=u:r:magisk:s0

Which would have told us more clearly that the command succeeded even though the exitcode was non-zero which might have helped debug this sooner.

With this patch Pixel2 Android 9 with Magisk v19 works for me locally. No idea on overall stability with regard to hangs or whatever. I'll investigate that later.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3a890fca43125cbc5a091ac8dd5d600e102a06

gbrown: I haven't yet done the phab dance. Would you mind putting this into phabricator for me?

Comment 7

[Geoff Brown [:gbrown]]

Attachment: Bug 1525401 - [mozdevice 3.0.2] work around need to invoke su -c setenforce 0 prior to check for su support when using Magisk later than 17.3, r=gbrown.

Comment 8

[Geoff Brown [:gbrown]]

Comment on attachment 9054456 [details] [diff] [review]
bug-1525401.patch

Review of attachment 9054456 [details] [diff] [review]:
-----------------------------------------------------------------

I imported your patch and preserved the author as you locally, but when I uploaded to phab, the author was changed to me. I don't know how to get around that.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e99a57e21d6
[mozdevice 3.0.2] work around need to invoke su -c setenforce 0 prior to check for su support when using Magisk later than 17.3, r=gbrown.

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0e99a57e21d6