Merge Narrow broad except statements.

As discussed in #241 and !906, there are some overly broad `except` statements that should be fixed. This MR does so. Some notes:
- `profile_dir` in `aa.py` may be `None`, hence the `TypeError` catches. The other globals are not caught in the code, however. E.g. there are possibilities of TypeErrors due to `extra_profile_dir` being `None`.
- I added a TODO in `common.py` that I'd like some eyes on. I do not think that `os.path.normpath` can raise an exception (the [Python docs](https://docs.python.org/3/library/os.path.html?#os.path.normpath) don't mention exceptions, and none are raised in the [CPython source code](https://github.com/python/cpython/blob/main/Lib/posixpath.py#L345)). [After discussing this in the MR, the `os.path.normpath` check was removed.]
- The `except Exception: raise` occurrences throughout `test-aa-easyprof.py` do nothing, so I removed them.
- In `valgrind_simple.py`, I fixed a possible `NameError` in the `finally` clause.

MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/912
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>

parser/tst/caching.py

@@ -544,9 +544,7 @@ def main():
     p.add_argument('-d', '--debug', action="store_true", dest="debug")
     config = p.parse_args()
 
-    verbosity = 1
+    verbosity = 2 if config.verbose else 1
-    if config.verbose:
-        verbosity = 2
 
     test_suite = unittest.TestSuite()
     test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAParserBasicCachingTests))
@@ -556,17 +554,14 @@ def main():
     test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAParserCreateCacheAltCacheTestsCacheNotExist))
     test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAParserCachingTests))
     test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAParserAltCacheTests))
-    rc = 0
     try:
         result = unittest.TextTestRunner(verbosity=verbosity).run(test_suite)
-        if not result.wasSuccessful():
+    except Exception:
         rc = 1
-    except:
+    else:
-        rc = 1
+        rc = 0 if result.wasSuccessful() else 1
-
     return rc
 
 
 if __name__ == "__main__":
-    rc = main()
+    exit(main())
-    exit(rc)

parser/tst/errors.py

@@ -91,8 +91,6 @@ class AAErrorTests(testlib.AATestTemplate):
 
 
 def main():
-    rc = 0
-
     global config
     p = ArgumentParser()
     p.add_argument('-p', '--parser', default=testlib.DEFAULT_PARSER, action="store", dest='parser',
@@ -100,18 +98,16 @@ def main():
     p.add_argument('-v', '--verbose', action="store_true", dest="verbose")
     config = p.parse_args()
 
-    verbosity = 1
+    verbosity = 2 if config.verbose else 1
-    if config.verbose:
-        verbosity = 2
 
     test_suite = unittest.TestSuite()
     test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAErrorTests))
     try:
         result = unittest.TextTestRunner(verbosity=verbosity).run(test_suite)
-        if not result.wasSuccessful():
+    except Exception:
-            rc = 1
-    except:
         rc = 1
+    else:
+        rc = 0 if result.wasSuccessful() else 1
 
     return rc
 

parser/tst/testlib.py

@@ -103,7 +103,7 @@ class AATestTemplate(unittest.TestCase, metaclass=AANoCleanupMetaClass):
         try:
             out, outerr = timeout_communicate(input)
             rc = sp.returncode
-        except TimeoutFunctionException as e:
+        except TimeoutFunctionException:
             sp.terminate()
             outerr = 'test timed out, killed'
             rc = TIMEOUT_ERROR_CODE
@@ -164,7 +164,7 @@ def filesystem_time_resolution():
 
             last_stamp = s.st_mtime
             time.sleep(default_diff)
-    except:
+    except Exception:
         pass
     finally:
         if os.path.exists(tmp_dir):

parser/tst/valgrind_simple.py

@@ -102,7 +102,9 @@ def main():
     if config.verbose:
         verbosity = 2
 
-    if not config.skip_suppressions:
+    if config.skip_suppressions:
+        suppression_file = None
+    else:
         suppression_file = create_suppressions()
         VALGRIND_ARGS.append('--suppressions=%s' % (suppression_file))
 
@@ -116,16 +118,17 @@ def main():
 
     try:
         result = unittest.TextTestRunner(verbosity=verbosity).run(test_suite)
+    except Exception:
+        rc = 1
+    else:
         if not result.wasSuccessful():
             rc = 1
-    except:
-        rc = 1
     finally:
+        if suppression_file:
             os.remove(suppression_file)
 
     return rc
 
 
 if __name__ == "__main__":
-    rc = main()
+    exit(main())
-    exit(rc)

utils/apparmor/aa.py

@@ -315,7 +315,7 @@ def create_symlink(subdir, filename):
     if not os.path.exists(link):
         try:
             os.symlink(filename, link)
-        except:
+        except OSError:
             raise AppArmorException(
                 _('Could not create %(link)s symlink to %(file)s.')
                 % {'link': link, 'file': filename})
@@ -323,13 +323,12 @@ def create_symlink(subdir, filename):
 
 def head(file):
     """Returns the first/head line of the file"""
-    first = ''
     if os.path.isfile(file):
         with open_file_read(file) as f_in:
             try:
                 first = f_in.readline().rstrip()
             except UnicodeDecodeError:
-                pass
+                first = ''
             return first
     else:
         raise AppArmorException(_('Unable to read first line from %s: File Not Found') % file)
@@ -1747,7 +1746,7 @@ def read_profiles(ui_msg=False, skip_profiles=[]):
 
     try:
         os.listdir(profile_dir)
-    except:
+    except (OSError, TypeError):
         fatal_error(_("Can't read AppArmor profiles in %s") % profile_dir)
 
     for file in os.listdir(profile_dir):
@@ -1776,7 +1775,7 @@ def read_inactive_profiles(skip_profiles=[]):
         return
     try:
         os.listdir(profile_dir)
-    except:
+    except (OSError, TypeError):
         fatal_error(_("Can't read AppArmor profiles in %s") % extra_profile_dir)
 
     for file in os.listdir(extra_profile_dir):

utils/apparmor/common.py

@@ -144,9 +144,8 @@ def cmd_pipe(command1, command2):
 
 def valid_path(path):
     """Valid path"""
-    # No relative paths
     m = "Invalid path: %s" % (path)
-    if not path.startswith('/'):
+    if not path.startswith('/'):  # No relative paths
         debug("%s (relative)" % (m))
         return False
 
@@ -154,11 +153,6 @@ def valid_path(path):
         debug("%s (contains quote)" % (m))
         return False
 
-    try:
-        os.path.normpath(path)
-    except Exception:
-        debug("%s (could not normalize)" % (m))
-        return False
     return True
 
 
@@ -167,12 +161,7 @@ def get_directory_contents(path):
     if not valid_path(path):
         return None
 
-    files = []
+    return sorted(glob.glob(path + "/*"))
-    for f in glob.glob(path + "/*"):
-        files.append(f)
-
-    files.sort()
-    return files
 
 
 def is_skippable_file(path):
@@ -314,7 +303,7 @@ class DebugLogger:
             self.debugging = os.getenv('LOGPROF_DEBUG')
             try:
                 self.debugging = int(self.debugging)
-            except Exception:
+            except (TypeError, ValueError):
                 self.debugging = False
             if self.debugging not in range(0, 4):
                 sys.stdout.write('Environment Variable: LOGPROF_DEBUG contains invalid value: %s'

utils/apparmor/easyprof.py

@@ -105,7 +105,7 @@ def valid_variable(v):
     debug("Checking '%s'" % v)
     try:
         (key, value) = v.split('=')
-    except Exception:
+    except ValueError:
         return False
 
     if not re.search(r'^@\{[a-zA-Z0-9_]+\}$', key):
@@ -169,12 +169,10 @@ def valid_policy_vendor(s):
 def valid_policy_version(v):
     """Verify the policy version"""
     try:
-        float(v)
+        f = float(v)
     except ValueError:
         return False
-    if float(v) < 0:
+    return f >= 0
-        return False
-    return True
 
 
 def valid_template_name(s, strict=False):

utils/apparmor/logparser.py

@@ -283,7 +283,6 @@ class ReadLog:
                 if event:
                     try:
                         self.parse_event_for_tree(event)
-
                     except AppArmorException as e:
                         ex_msg = ('%(msg)s\n\nThis error was caused by the log line:\n%(logline)s'
                                   % {'msg': e.value, 'logline': line})

utils/apparmor/sandbox.py

@@ -715,9 +715,7 @@ def run_xsandbox(command, opt):
     # aa-exec
     try:
         rc, report = aa_exec(command, opt, x.new_environ, required_rules)
-    except Exception:
+    finally:
-        x.cleanup()
-        raise
         x.cleanup()
 
     return rc, report

utils/apparmor/ui.py

@@ -537,5 +537,5 @@ def confirm_and_abort():
 def is_number(number):
     try:
         return int(number)
-    except:
+    except (TypeError, ValueError):
         return False

utils/test/test-aa-easyprof.py

@@ -229,8 +229,6 @@ TEMPLATES_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -247,8 +245,6 @@ TEMPLATES_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -265,8 +261,6 @@ TEMPLATES_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -358,8 +352,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -376,8 +368,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -394,8 +384,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(self.binary, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
 
         raise Exception("File should have been invalid")
 
@@ -504,8 +492,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.AppArmorEasyProfile('./foo', self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("Binary should have been invalid")
 
     def test_binary_symlink(self):
@@ -519,8 +505,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.AppArmorEasyProfile(symlink, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("Binary should have been invalid")
 
 #
@@ -875,8 +859,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_policy(template=os.path.join(self.tmpdir, "/nonexistent"))
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("template should be invalid")
 
     def test_genpolicy_name(self):
@@ -938,8 +920,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=['--abstractions=%s' % s])
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("abstraction '%s' should be invalid" % s)
 
     def _create_tmp_base_dir(self, prefix='', abstractions=[], tunables=[]):
@@ -999,8 +979,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=args)
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("abstraction '%s' should be invalid" % abstraction)
 
     def test_genpolicy_abstractions_custom_include(self):
@@ -1028,8 +1006,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=args)
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("abstraction '%s' should be invalid" % abstraction)
 
     def test_genpolicy_profile_name_bad(self):
@@ -1044,8 +1020,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=['--profile-name=%s' % s])
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("profile_name '%s' should be invalid" % s)
 
     def test_genpolicy_policy_group_bad(self):
@@ -1060,8 +1034,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=['--policy-groups=%s' % s])
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("policy group '%s' should be invalid" % s)
 
     def test_genpolicy_policygroups(self):
@@ -1102,8 +1074,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_policy(extra_args=['--policy-groups=nonexistent'])
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("policygroup should be invalid")
 
     def test_genpolicy_readpath_file(self):
@@ -1213,8 +1183,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_policy(extra_args=['--read-path=%s' % s])
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("read-path should be invalid")
 
     def test_genpolicy_writepath_file(self):
@@ -1324,8 +1292,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_policy(extra_args=['--write-path=%s' % s])
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("write-path should be invalid")
 
     def test_genpolicy_templatevar(self):
@@ -1371,8 +1337,6 @@ POLICYGROUPS_DIR="%s/templates"
                 self._gen_policy(extra_args=['--template-var=%s' % s])
             except AppArmorException:
                 continue
-            except Exception:
-                raise
             raise Exception("template-var should be invalid")
 
     def test_genpolicy_invalid_template_policy(self):
@@ -1396,8 +1360,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_policy(template=template)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("policy should be invalid")
 
     def test_genpolicy_no_binary_without_profile_name(self):
@@ -1406,8 +1368,6 @@ POLICYGROUPS_DIR="%s/templates"
             easyprof.gen_policy_params(None, self.options)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("No binary or profile name should have been invalid")
 
     def test_genpolicy_with_binary_with_profile_name(self):
@@ -1474,8 +1434,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_manifest_policy(m)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("abs path template name should be invalid")
 
     def test_gen_manifest_escape_path_templates(self):
@@ -1486,8 +1444,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_manifest_policy(m)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("../ template name should be invalid")
 
     def test_gen_manifest_policy_templates_nonexistent(self):
@@ -1498,8 +1454,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_manifest_policy(m)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("template should be invalid")
 
     def test_gen_manifest_policy_comment(self):
@@ -1577,8 +1531,6 @@ POLICYGROUPS_DIR="%s/templates"
             self._gen_manifest_policy(m)
         except AppArmorException:
             return
-        except Exception:
-            raise
         raise Exception("policygroup should be invalid")
 
     def test_gen_manifest_policy_templatevar(self):