793856 - Remove code to restart as root in devicemanagerADB; replace with on-demand use of su

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

This code really doesn't work properly in many environments (for example, it causes further commands to fail on my Galaxy Nexus running Jelly Bean). Other people have had similar issues: 

    https://github.com/vvuk/mozbase/commit/ad8b807d9fcd80cb55f69541c2f0d902c9a288ee

I believe the going assumption should be that dmADB should rely on root for as few things as possible, so that contributors can run things like mochitest and reftest on unrooted devices. For those few cases where root is absolutely required (should be very rare), we should use the su command (call shell() with su). If the user requested a method that requires root but we don't have a working su executable, we should throw an exception.

Comment 1

[William Lachance (:wlach)]

Attachment: Remove code to restart as root in devicemanagerADB; replace with on-demand use of su if don't have root shell

Hey Geoff, thought I'd get your review on this since you're the one who's probably worked with this code the most as far as devicemanagerADB is concerned. You probably want to read the rest of the bug for context; hopefully my plan makes sense. I really think we should try to clean up this code as I've frequently found developers confused by the current behaviour (e.g. the strange messages about "NOT running as root", failures to work after running "adb root", etc.).

mdas: ahal: jgriffin: I'm trying to get the sense of how this will affect B2G. Does this make sense there too? Is the adb shell we run in B2G root by default? If not, is there an su command that can be made use of? If my approach won't work in that context, what would?

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 664240 [details] [diff] [review]
Remove code to restart as root in devicemanagerADB; replace with on-demand use of su if don't have root shell

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

LGTM. I agree that "adb root" is problematic and I think this approach is superior.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +95,5 @@
> +        if root and not self.haveRootShell and not self.haveSu:
> +            raise DMError("Shell command '%s' requested to run as root but su "
> +                          "command not available or working on this device. You "
> +                          "need to either correctly root your device or refactor "
> +                          "the test/harness to not require root." %

nit - consider "Shell command '%s' requested to run as root but root is not available on this device. Root your device or refactor the test/harness to not require root."

@@ +904,5 @@
> +            # if this returns true, we don't care about su
> +            return
> +
> +        # ok, if we don't have a root shell, let's check to see whether we can
> +        # use the "su" command to gain root privileges

nit - perhaps simply "if root shell is not available, check if 'su' can be used to gain root"

@@ +909,5 @@
> +        proc = self.runCmd(["shell", "su", "-c", "id"])
> +
> +        # 15 second timeout (should return very quickly unless we don't
> +        # have permission or the Android SuperUser app is asking for
> +        # permission, which we can't have)

nit - consider "wait for response for maximum of 15 seconds, in case su prompts for a password or triggers the Android SuperUser prompt"

Comment 3

[Jonathan Griffin (:jgriffin)]

Comment on attachment 664240 [details] [diff] [review]
Remove code to restart as root in devicemanagerADB; replace with on-demand use of su if don't have root shell

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

This looks like it should be compatible with B2G.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 664240 [details] [diff] [review]
Remove code to restart as root in devicemanagerADB; replace with on-demand use of su if don't have root shell

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

By default, we are root, and we also have a 'su' command available by default as well.

The patch looks good, this will work in b2g.

Comment 5

[William Lachance (:wlach)]

Pushed with suggested wording changes from gbrown, plus one extra fix to make sure we kill the su-checking subprocess if it hangs (verified by gbrown over irc):

https://github.com/mozilla/mozbase/commit/1aef34ff74874e99ee3b2bf38fa5d4887b82f592

Comment 6

[William Lachance (:wlach)]

Comment on attachment 664240 [details] [diff] [review]
Remove code to restart as root in devicemanagerADB; replace with on-demand use of su if don't have root shell

Cancelling feedback request from ahal, got enough comments from jgriffin and mdas.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Awesome, thanks guys; I'll update my other patches (and my app that uses it) to take advantage of the new stuff.  One comment -- if the device is rooted and SuperUser prompts, it'll often prompt for every distinct su command.  This is a little annoying, so might be worth thinking of a way around.. one thought I had, which is complicated, but doable, is to write the command to a script (like /data/local/tmp/run-as-root.sh) so that the command can always be 'su -c run-as-root.sh'.  More complicated, but might be worth it to avoid the annoyance.