Snap for 7357916 from 6a0019626a4e51628bc06f6390a81f510c88f8b2 to mainline-captiveportallogin-release

Change-Id: I6d45bd90a6f7639695e9d20a009fc3d0acf64bbe

3 files changed, 84 insertions, 19 deletions

diff --git a/rh/config.py b/rh/config.py
index 2c7b4af..1eb93a7 100644
--- a/rh/config.py
+++ b/rh/config.py
@@ -66,7 +66,7 @@ class RawConfigParser(configparser.RawConfigParser):
     def items(self, section=_UNSET, default=_UNSET):
         """Return a list of (key, value) tuples for the options in |section|."""
         if section is _UNSET:
-            return super(RawConfigParser, self).items()
+            return super().items()
 
         try:
             return configparser.RawConfigParser.items(self, section)
@@ -214,7 +214,7 @@ class PreUploadConfig(object):
                 self.custom_hook(hook)
             except ValueError as e:
                 raise ValidationError('%s: hook "%s" command line is invalid: '
-                                      '%s' % (self.source, hook, e))
+                                      '%s' % (self.source, hook, e)) from e
 
         # Verify hook options are valid shell strings.
         for hook in self.builtin_hooks:
@@ -222,7 +222,7 @@ class PreUploadConfig(object):
                 self.builtin_hook_option(hook)
             except ValueError as e:
                 raise ValidationError('%s: hook options "%s" are invalid: %s' %
-                                      (self.source, hook, e))
+                                      (self.source, hook, e)) from e
 
         # Reject unknown tools.
         valid_tools = set(rh.hooks.TOOL_PATHS.keys())
@@ -259,13 +259,13 @@ class PreUploadFile(PreUploadConfig):
         Args:
           path: The config file to load.
         """
-        super(PreUploadFile, self).__init__(source=path)
+        super().__init__(source=path)
 
         self.path = path
         try:
             self.config.read(path)
         except configparser.ParsingError as e:
-            raise ValidationError('%s: %s' % (path, e))
+            raise ValidationError('%s: %s' % (path, e)) from e
 
         self._validate()
 
@@ -290,7 +290,7 @@ class LocalPreUploadFile(PreUploadFile):
     FILENAME = 'PREUPLOAD.cfg'
 
     def _validate(self):
-        super(LocalPreUploadFile, self)._validate()
+        super()._validate()
 
         # Reject Exclude Paths section for local config.
         if self.config.has_section(self.BUILTIN_HOOKS_EXCLUDE_SECTION):
@@ -320,7 +320,7 @@ class PreUploadSettings(PreUploadConfig):
           paths: The directories to look for config files.
           global_paths: The directories to look for global config files.
         """
-        super(PreUploadSettings, self).__init__()
+        super().__init__()
 
         self.paths = []
         for config in itertools.chain(
diff --git a/rh/utils.py b/rh/utils.py
index 52a6828..aeab52f 100644
--- a/rh/utils.py
+++ b/rh/utils.py
@@ -66,7 +66,7 @@ class CompletedProcess(getattr(subprocess, 'CompletedProcess', object)):
             self.stderr = stderr
             self.returncode = returncode
         else:
-            super(CompletedProcess, self).__init__(
+            super().__init__(
                 args=args, returncode=returncode, stdout=stdout, stderr=stderr)
 
     @property
@@ -99,7 +99,7 @@ class CalledProcessError(subprocess.CalledProcessError):
             raise TypeError('exception must be an exception instance; got %r'
                             % (exception,))
 
-        super(CalledProcessError, self).__init__(returncode, cmd, stdout)
+        super().__init__(returncode, cmd, stdout)
         # The parent class will set |output|, so delete it.
         del self.output
         # TODO(vapier): When we're Python 3-only, delete this assignment as the
@@ -361,6 +361,12 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
     env = env.copy() if env is not None else os.environ.copy()
     env.update(extra_env if extra_env else {})
 
+    def ensure_text(s):
+        """Make sure |s| is a string if it's bytes."""
+        if isinstance(s, bytes):
+            s = s.decode('utf-8', 'replace')
+        return s
+
     result.args = cmd
 
     proc = None
@@ -406,19 +412,26 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
             if extra_env:
                 msg += ', extra env=%s' % extra_env
             raise CalledProcessError(
-                result.returncode, result.cmd, stdout=result.stdout,
-                stderr=result.stderr, msg=msg)
+                result.returncode, result.cmd, msg=msg,
+                stdout=ensure_text(result.stdout),
+                stderr=ensure_text(result.stderr))
     except OSError as e:
+        # Avoid leaking tempfiles.
+        if popen_stdout is not None and not isinstance(popen_stdout, int):
+            popen_stdout.close()
+        if popen_stderr is not None and not isinstance(popen_stderr, int):
+            popen_stderr.close()
+
         estr = str(e)
         if e.errno == errno.EACCES:
             estr += '; does the program need `chmod a+x`?'
         if not check:
-            result = CompletedProcess(
-                args=cmd, stderr=estr.encode('utf-8'), returncode=255)
+            result = CompletedProcess(args=cmd, stderr=estr, returncode=255)
         else:
             raise CalledProcessError(
-                result.returncode, result.cmd, stdout=result.stdout,
-                stderr=result.stderr, msg=estr, exception=e)
+                result.returncode, result.cmd, msg=estr, exception=e,
+                stdout=ensure_text(result.stdout),
+                stderr=ensure_text(result.stderr)) from e
     finally:
         if proc is not None:
             # Ensure the process is dead.
@@ -428,10 +441,8 @@ def run(cmd, redirect_stdout=False, redirect_stderr=False, cwd=None, input=None,
                                 None, None)
 
     # Make sure output is returned as a string rather than bytes.
-    if result.stdout is not None:
-        result.stdout = result.stdout.decode('utf-8', 'replace')
-    if result.stderr is not None:
-        result.stderr = result.stderr.decode('utf-8', 'replace')
+    result.stdout = ensure_text(result.stdout)
+    result.stderr = ensure_text(result.stderr)
 
     return result
 # pylint: enable=redefined-builtin,input-builtin
diff --git a/rh/utils_unittest.py b/rh/utils_unittest.py
index f3098a9..ea2ddaa 100755
--- a/rh/utils_unittest.py
+++ b/rh/utils_unittest.py
@@ -161,6 +161,60 @@ class RunCommandTests(unittest.TestCase):
         self.assertEqual(u'ß', ret.stdout)
         self.assertIsNone(ret.stderr)
 
+    def test_check_false(self):
+        """Verify handling of check=False."""
+        ret = rh.utils.run(['false'], check=False)
+        self.assertNotEqual(0, ret.returncode)
+        self.assertIn('false', str(ret))
+
+        ret = rh.utils.run(['true'], check=False)
+        self.assertEqual(0, ret.returncode)
+        self.assertIn('true', str(ret))
+
+    def test_check_true(self):
+        """Verify handling of check=True."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['false'], check=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('false', str(err))
+
+        ret = rh.utils.run(['true'], check=True)
+        self.assertEqual(0, ret.returncode)
+        self.assertIn('true', str(ret))
+
+    def test_check_false_output(self):
+        """Verify handling of output capturing w/check=False."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['sh', '-c', 'echo out; echo err >&2; false'],
+                         check=True, capture_output=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('false', str(err))
+
+    def test_check_true_missing_prog_output(self):
+        """Verify handling of output capturing w/missing progs."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['./!~a/b/c/d/'], check=True, capture_output=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('a/b/c/d', str(err))
+
+    def test_check_false_missing_prog_output(self):
+        """Verify handling of output capturing w/missing progs."""
+        ret = rh.utils.run(['./!~a/b/c/d/'], check=False, capture_output=True)
+        self.assertNotEqual(0, ret.returncode)
+        self.assertIn('a/b/c/d', str(ret))
+
+    def test_check_false_missing_prog_combined_output(self):
+        """Verify handling of combined output capturing w/missing progs."""
+        with self.assertRaises(rh.utils.CalledProcessError) as e:
+            rh.utils.run(['./!~a/b/c/d/'], check=True,
+                         combine_stdout_stderr=True)
+        err = e.exception
+        self.assertNotEqual(0, err.returncode)
+        self.assertIn('a/b/c/d', str(err))
+
 
 if __name__ == '__main__':
     unittest.main()