Closed Bug 1485852 Opened Last year Closed Last year

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

Categories

(Testing :: Mozbase, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(2 files)

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.
I think this will be helpful in a future effort to eliminate root altogether.
Attachment #9003665 - Flags: review?(jmaher)
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.
Attachment #9003665 - Flags: review?(jmaher) → review+
(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. ;-)
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.
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
Attachment #9004083 - Flags: review?(jmaher)
Attachment #9004083 - Flags: review?(jmaher) → review+
Pushed by bclary@mozilla.com:
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.
Great stuff :bc! This will be super helpful for mozregression.
https://hg.mozilla.org/mozilla-central/rev/240acefd371b
https://hg.mozilla.org/mozilla-central/rev/2ed22b84466f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1487130
You need to log in before you can comment on or make changes to this bug.