telemetry: android: use both methods to access protected files

build/android/pylib/android_commands.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides an interface to communicate with the device via the adb command.
 
 Assumes adb binary is currently on system path.
 """
 # pylint: disable-all
 
@@ skipping 279 matching lines @@
     self._potential_push_size = 0
     self._actual_push_size = 0
     self._external_storage = ''
     self._util_wrapper = ''
     self._system_properties = system_properties.SystemProperties(self.Adb())
     self._push_if_needed_cache = {}
     self._control_usb_charging_command = {
         'command': None,
         'cached': False,
     }
+    self._protected_file_access_method = None
 
   @property
   def system_properties(self):
     return self._system_properties
 
   def _LogShell(self, cmd):
     """Logs the adb shell command."""
     if self._device:
       device_repr = self._device[-4:]
     else:
@@ skipping 786 matching lines @@
   def _GetDeviceTempFileName(self, base_name):
     i = 0
     while self.FileExistsOnDevice(
         self.GetExternalStorage() + '/' + base_name % i):
       i += 1
     return self.GetExternalStorage() + '/' + base_name % i
 
   def RunShellCommandWithSU(self, command, timeout_time=20, log_result=False):
     return self.RunShellCommand('su -c %s' % command, timeout_time, log_result)
 
+  ACCESS_METHOD_NO_ACCESS = "no access"

[Sami, 2014/04/28 12:55:49]

nit: Start with underscore since these probably don't need to be public. Also
these could just be numeric constants since it doesn't look like the strings are
used for anything.

[jbudorick, 2014/04/28 14:22:52]

+1 for using numeric constants.

[pasko, 2014/04/29 09:42:32]

Renaming: Done. Most votes are for numeric constants, changed to numeric.

[pasko, 2014/04/29 09:42:32]

Done.

+  ACCESS_METHOD_AS_ROOT = "as root"
+  ACCESS_METHOD_WITH_SU = "with su"

[bulach, 2014/04/28 12:21:48]

nit: use single quotes

[pasko, 2014/04/29 09:42:32]

thanks, changed to numeric constants

+
   def CanAccessProtectedFileContents(self):
-    """Returns True if Get/SetProtectedFileContents would work via "su".
+    """Returns True if Get/SetProtectedFileContents would work via "su" or adb
+    shell running as root.
 
     Devices running user builds don't have adb root, but may provide "su" which
     can be used for accessing protected files.
     """
-    r = self.RunShellCommandWithSU('cat /dev/null')
-    return r == [] or r[0].strip() == ''
+    return (self._ProtectedFileAccessMethod() !=
+        AndroidCommands.ACCESS_METHOD_NO_ACCESS)
+
+  def _ProtectedFileAccessMethod(self):
+    """Tests for the best method to access protected files on the device.
+
+    Returns:
+      ACCESS_METHOD_NO_ACCESS: if protected files cannot be accessed
+      ACCESS_METHOD_AS_ROOT:   if the adb shell has root privileges
+      ACCESS_METHOD_WITH_SU:   if the 'su' command properly works on the device
+    """
+    if self._protected_file_access_method is not None:

[bulach, 2014/04/28 12:21:48]

nit: s/is not None//

[pasko, 2014/04/29 09:42:32]

I changed this to numeric constants, so to distinguish between 0 and None we
would need this 'is not None'.

+      return self._protected_file_access_method
+
+    def AuxContentsLookReal(contents):

[bulach, 2014/04/28 12:21:48]

nit: how about "_IsValidContents" ?

[jbudorick, 2014/04/28 14:22:52]

Why check the output rather than the return code?

[pasko, 2014/04/29 09:42:32]

I changed this to IsValidAuxContents(). Since the definition is function-local,
we don't need the leading underscore, right?

[pasko, 2014/04/29 09:42:32]

This is mainly because RunShellCommand() and RunShellCommandWithSU() both return
contents and swallow the return code. I did not want to risk potential future
inconsistency between how I run "su" and how RunShellCommandWithSU() does it.
However, the risk is relatively small and I could just do cmd_helper.RunCmd('su
-c cat /proc/1/auxv') and check for the return code. I'm OK to use this variant
if it's your preference.

+      # The leading 4 or 8-bytes of auxv vector is a_type. There are not many
+      # reserved a_type values, hence byte 2 must always be '\0' for a realistic
+      # auxv. See /usr/include/elf.h.
+      return len(contents) > 0 and (contents[0][2] == '\0')
+
+    self._protected_file_access_method = AndroidCommands.ACCESS_METHOD_NO_ACCESS
+
+    # Get contents of the auxv vector for the init(8) process from a small
+    # binary file that always exists on a linux and is always read-protected.
+    contents = self.RunShellCommand('cat /proc/1/auxv')

[Sami, 2014/04/28 12:55:49]

Neat trick :)

[pasko, 2014/04/29 09:42:32]

thnx)

+    if AuxContentsLookReal(contents):
+      # Protected data is available without SU, hence adb is running as root.
+      self._protected_file_access_method = AndroidCommands.ACCESS_METHOD_AS_ROOT

[jbudorick, 2014/04/28 14:22:52]

FWIW, I've been toying with doing the as_root check in the refactored
implementation by checking the 'service.adb.root' property on the device. That
method doesn't have a great way to check for su access, though.

[pasko, 2014/04/29 09:42:32]

I did not know about this property :)

+    else:
+      contents = self.RunShellCommandWithSU('cat /proc/1/auxv')
+      if AuxContentsLookReal(contents):
+        # Protected data is available when asked with SU.
+        self._protected_file_access_method = (
+            AndroidCommands.ACCESS_METHOD_WITH_SU)
+    return self._protected_file_access_method

[Sami, 2014/04/28 12:55:49]

Set access method to ACCESS_METHOD_NO_ACCESS here?

[pasko, 2014/04/29 09:42:32]

I've set it above. If I wanted to move it here from the above, I'd have to add
two more return statements. Is that what you want?

 
   def GetProtectedFileContents(self, filename):
     """Gets contents from the protected file specified by |filename|.
 
-    This is less efficient than GetFileContents, but will work for protected
-    files and device files.
+    This is potentially less efficient than GetFileContents.
     """
-    # Run the script as root
-    return self.RunShellCommandWithSU('cat "%s" 2> /dev/null' % filename)
+    command = 'cat "%s" 2> /dev/null' % filename
+    access_method = self._ProtectedFileAccessMethod()
+    if access_method == AndroidCommands.ACCESS_METHOD_WITH_SU:
+      return self.RunShellCommandWithSU(command)
+    elif access_method == AndroidCommands.ACCESS_METHOD_AS_ROOT:
+      return self.RunShellCommand(command)
+    else:
+      logging.warning('Could not access protected file: %s' % filename)
+      return []

[bulach, 2014/04/28 12:21:48]

now, if we're going to fix this once and for all :)
frankf / craigdh had this idea of never failing gracefully at this level and
instead throw hard exceptions, so that caller code can explicitly handle it.
I'm not sure how much would it break right now, but I think we should at least
give it a try(job), wdyt?

[jbudorick, 2014/04/28 14:22:52]

I'm ok with this idea in general. I think implementing it will break a lot,
though, so I'm not sure that this CL is the place for it.

[pasko, 2014/04/29 09:42:32]

I'm +1 to jbudorick@ here: it will likely break in many places with a hard
exception. Let's start with a mere warning and then address this in next CL(s).

 
   def SetProtectedFileContents(self, filename, contents):
     """Writes |contents| to the protected file specified by |filename|.
 
-    This is less efficient than SetFileContents, but will work for protected
-    files and device files.
+    This is less efficient than SetFileContents.
     """
     temp_file = self._GetDeviceTempFileName(AndroidCommands._TEMP_FILE_BASE_FMT)
     temp_script = self._GetDeviceTempFileName(
         AndroidCommands._TEMP_SCRIPT_FILE_BASE_FMT)
 
     # Put the contents in a temporary file
     self.SetFileContents(temp_file, contents)
     # Create a script to copy the file contents to its final destination
     self.SetFileContents(temp_script, 'cat %s > %s' % (temp_file, filename))
-    # Run the script as root
-    self.RunShellCommandWithSU('sh %s' % temp_script)
+
+    command = 'sh %s' % temp_script
+    access_method = self._ProtectedFileAccessMethod()

[Sami, 2014/04/28 12:55:49]

This could be a little cleaner if access_method was just a reference to
self.RunShellCommand or self.RunShellCommandWithSU depending on the detection
result. Then this could just check that it has valid access method and fail
otherwise.

[pasko, 2014/04/29 09:42:32]

Do you suggest to check for None and call otherwise? If i'd be checking for a
method among RunShellCommand and RunShellCommandWithSU, then it's probably a
little cleaner, but similar level of verbosity.

I don't usually compare function pointers in python, but when I do, I ask if an
average reader in our project would know what this magic means :)

+    if access_method == AndroidCommands.ACCESS_METHOD_WITH_SU:
+      return self.RunShellCommandWithSU(command)
+    elif access_method == AndroidCommands.ACCESS_METHOD_AS_ROOT:
+      return self.RunShellCommand(command)
+    else:
+      logging.warning('Could not set contents of protected file: %s' % filename)

[bulach, 2014/04/28 12:21:48]

ditto.

+
     # And remove the temporary files
     self.RunShellCommand('rm ' + temp_file)
     self.RunShellCommand('rm ' + temp_script)
 
   def RemovePushedFiles(self):
     """Removes all files pushed with PushIfNeeded() from the device."""
     for p in self._pushed_files:
       self.RunShellCommand('rm -r %s' % p, timeout_time=2 * 60)
 
   def ListPathContents(self, path):
@@ skipping 774 matching lines @@
   """
   def __init__(self, output):
     self._output = output
 
   def write(self, data):
     data = data.replace('\r\r\n', '\n')
     self._output.write(data)
 
   def flush(self):
     self._output.flush()