1485852 - [mozdevice] adb*.py should use require_root parameter to control root detection

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Bug 1482898 introduced a property require_root for the ADBCommand class which allowed the skipping of making root=True calls when checking the test root if the user determined root was not necessary. This was incomplete and was prone to error in its use.

We should make this available in the other relevant classes as well and use it to guard against checking for su when we don't need it.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: add-require-root-initializer-everywhere.patch

I think this will be helpful in a future effort to eliminate root altogether.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 9003665 [details] [diff] [review]
add-require-root-initializer-everywhere.patch

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

just 1 suggestion

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +616,5 @@
>          uid = 'uid=0'
>          # Do we have a 'Superuser' sh like su?
>          try:
> +            if (self._require_root and
> +                self.shell_output("su -c id", timeout=timeout).find(uid) != -1):

do we want to check for _require_root here as we are just setting a flag that su is available- possibly _have_su is not a flag, but a blocking gate.

::: testing/mozbase/mozdevice/mozdevice/adb_android.py
@@ +265,5 @@
>                      failure = "Device state: %s" % state
>                      success = False
>                  else:
> +                    if (self._require_root and
> +                        self.selinux and

selinux can only be True if _require_root is set above, so this is redundant, but I believe keeping it in here helps prevent bad copy/paste of code in the future.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #2)
> Comment on attachment 9003665 [details] [diff] [review]
> add-require-root-initializer-everywhere.patch
> 
> Review of attachment 9003665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just 1 suggestion
> 
> ::: testing/mozbase/mozdevice/mozdevice/adb.py
> @@ +616,5 @@
> >          uid = 'uid=0'
> >          # Do we have a 'Superuser' sh like su?
> >          try:
> > +            if (self._require_root and
> > +                self.shell_output("su -c id", timeout=timeout).find(uid) != -1):
> 
> do we want to check for _require_root here as we are just setting a flag
> that su is available- possibly _have_su is not a flag, but a blocking gate.

I'm not sure I understand. _have_su means we will invoke su as su -c ...  while _have_android_su means we will invoke it using su 0 ...

For one thing, I didn't want to try to execute su unless I wanted to do so later. That would prevent the kind of situation where I found a hang on my local pixel2/magisk running su 0 unnecessarily. The other is I think this will help find locations where root is or is not required since it prevents detection of the su command. If adbd is not running as root and su is not detected and an attempt is made to call a shell command with root=True we get ADBRootError raised. That should flag locations that need to have a non-root alternative.

adbd is automatically restarted as root but this only works on engineering/debug AOSP builds and not for most rooted commercial devices.

Perhaps, I'm just missing the idea though.

> 
> ::: testing/mozbase/mozdevice/mozdevice/adb_android.py
> @@ +265,5 @@
> >                      failure = "Device state: %s" % state
> >                      success = False
> >                  else:
> > +                    if (self._require_root and
> > +                        self.selinux and
> 
> selinux can only be True if _require_root is set above, so this is
> redundant, but I believe keeping it in here helps prevent bad copy/paste of
> code in the future.

actually it is set via

            enforce = self.shell_output('getenforce', timeout=timeout)
            self.selinux = (enforce == 'enforcing' or enforce == '1')

in ADBAndroid's constructor which could be the case regardless of _require_root. If we _require_root and we initially detected that we were Enforcing we'll execute the code to set it to Permissive. This is actually a bit confusing but the self.selinux is used by reboot to tell us that we need to call setenforce again after rebooting. This will allow us to detect situations where Enforcing causes problems if we aren't rooted. At least that was the idea, but I've managed to confuse myself now. ;-)

Comment 4

[Bob Clary [:bc] (inactive)]

I'm not happy with the getenforce/setenforce usage.

getenforce/setenforce is supposedly available on Android 4.1+ which is sdk 16 which is our minimum level of support.
https://android.googlesource.com/platform/system/core/+/master/shell_and_utilities/
https://en.wikipedia.org/wiki/Android_version_history

Checking, I find that getenforce/setenforce is available on our standard emulators except the 'x86' which is a 4.2 tablet which should have it but doesn't.

getenforce returns Permissive on the emulators that have it but selinux is set to True which violates one of my earlier statements.

I'm going to rework that a bit. New patch coming up.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: rework-selinux-support.patch

This is a follow up to the previous patch. We can fold them together if you like or keep them separate. Works well locally and on try. Thanks making me rethink this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a85cfa2d7573c6f6e8cca3c86a91bb9c080d7b54&filter-tier=1&filter-tier=2&filter-tier=3

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/240acefd371b
[mozdevice]  adb*.py should use require_root parameter to control root detection, r=jmaher.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed22b84466f
[mozdevice] rework SELinux support in adb_android.py, r=jmaher.

Comment 7

[William Lachance (:wlach)]

Great stuff :bc! This will be super helpful for mozregression.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/240acefd371b
https://hg.mozilla.org/mozilla-central/rev/2ed22b84466f