[apparmor] [patch] update aa.py is_skippable_file() according to libapparmor

[Christian Boltz]

Hello,

[resent without linebreaks in the patch]

Am Dienstag, 3. Februar 2015 schrieb Christian Boltz:
> Am Donnerstag, 25. Dezember 2014 schrieb Kshitij Gupta:
> > On Thu, Dec 25, 2014 at 6:02 AM, Christian Boltz wrote:
> > > Am Mittwoch, 24. Dezember 2014 schrieb Kshitij Gupta:
> > > > On Sun, Dec 7, 2014 at 2:49 AM, Christian Boltz wrote:
> <big snip, scroll down for updated patch>
> 
> > > I'd also be interested in some numbers - how long do the following
> > > variants take?
> > > 
> > > a) the "old" code we currently have in bzr (hint: it does 4
> > > re.search)
> >  
> >  0.185 s for 1000 (x13 in each for all cases) calls to functions
> >  
> > > b) with my patch applied (only one re.search, so I'd expect it to
> > > be
> > > 
> > >    faster)
> >  
> >  0.160 s for 1000 (x13 in each for all cases) calls to functions
> 
> That difference isn't too big.
> 
> >> c) with my patch applied + using re.compile for skippable_files
> >> 
> > >    c1) with re.compile inside the function (= one re.compile per
> > >    file)
> > 
> > 0.144 s for 1000 (x13 in each for all cases) calls to functions
> 
> So re.compile already improves things for a regex used only once.
> 
> > >    c2) with re.compile outside the function (= only one re.compile
> > >    in total)
> > 
> > 0.065 s for 1000 (x13 in each for all cases) calls to functions
> 
> That's a clear sign that we should use re.compile() everywhere,
> ideally in the global area (outside of any functions) for often-used
> regexes so that each regex is compiled only once.
> 
> > d) with the code using os.path.* you proposed
> > 
> > Using os.path.* and in operator:
> > 0.082 s for 1000 (x13 in each for all cases) calls to functions
> > 
> > Using os.path.basename followed by your suggested endswith for
> > basename: 0.052 s for 1000 (x13 in each for all cases) calls to
> > functions
> 
> This variant is fast, easily readable, avoids the escaping we would
> need for a regex and avoids having something (the compiled regex)
> "polluting" the module namespace, so it's the way to go :-)
> 
> > I've attach the test scripts, separate files for all 6 cases.
> 
> Thanks, and thanks for all the testing!
> 
> Here's the updated patch, based on your "ext6.py". I only changed the
> aa.py part, the testcases didn't change since v1.
> 
> 
> 
> Update is_skippable_file() to match all extensions that are listed in
> libapparmor _aa_is_blacklisted() - some extensions were missing in the
> python code.
> 
> Also make the code more readable and add some testcases.
> 
> Notes:
> - the original code additionally ignored *.swp. I didn't include that
> - *.swp looks like vim swap files which are also dot files
> - the python code ignores README files, but the C code doesn't
>   (do we need to add README in the C code?)
> 
> 
[ is_skippable_file.diff ]

=== modified file 'utils/apparmor/aa.py'
--- utils/apparmor/aa.py        2015-01-30 20:08:17 +0000
+++ utils/apparmor/aa.py        2015-02-03 18:12:37 +0000
@@ -2539,14 +2539,22 @@
         else:
             return False
 
-# rpm backup files, dotfiles, emacs backup files should not be processed
-# The skippable files type needs be synced with apparmor initscript
+
 def is_skippable_file(path):
-    """Returns True if filename matches something to be skipped"""
-    if (re.search('(^|/)\.[^/]*$', path) or re.search('\.rpm(save|new)$', path)
-            or re.search('\.dpkg-(old|new)$', path) or re.search('\.swp$', path)
-            or path[-1] == '~' or path == 'README'):
-        return True
+    """Returns True if filename matches something to be skipped (rpm or dpkg backup files, hidden files etc.)
+        The list of skippable files needs to be synced with apparmor initscript and libapparmor _aa_is_blacklisted()
+        path: filename (with or without directory)"""
+
+    basename = os.path.basename(path)
+
+    if not basename or basename[0] == '.' or basename == 'README':
+        return True
+
+    skippable_suffix = ('.dpkg-new', '.dpkg-old', '.dpkg-dist', '.dpkg-bak', '.rpmnew', '.rpmsave', '.orig', '.rej', '~')
+    if basename.endswith(skippable_suffix):
+        return True
+
+    return False
 
 def is_skippable_dir(path):
     if re.search('(disable|cache|force-complain|lxc)', path):

=== modified file 'utils/test/test-aa.py'
--- utils/test/test-aa.py       2014-11-27 22:20:26 +0000
+++ utils/test/test-aa.py       2015-02-03 17:57:25 +0000
@@ -15,7 +15,7 @@
 import tempfile
 from common_test import write_file
 
-from apparmor.aa import check_for_apparmor
+from apparmor.aa import check_for_apparmor, is_skippable_file
 
 class AaTest_check_for_apparmor(unittest.TestCase):
     FILESYSTEMS_WITH_SECURITYFS = 'nodev\tdevtmpfs\nnodev\tsecurityfs\nnodev\tsockfs\n\text3\n\text2\n\text4'
@@ -70,6 +70,48 @@
         mounts = write_file(self.tmpdir, 'mounts', self.MOUNTS_WITH_SECURITYFS % self.tmpdir)
         self.assertEqual('%s/security/apparmor' % self.tmpdir, check_for_apparmor(filesystems, mounts))
 
+class AaTest_is_skippable_file(unittest.TestCase):
+    def test_not_skippable_01(self):
+        self.assertFalse(is_skippable_file('bin.ping'))
+    def test_not_skippable_02(self):
+        self.assertFalse(is_skippable_file('usr.lib.dovecot.anvil'))
+    def test_not_skippable_03(self):
+        self.assertFalse(is_skippable_file('bin.~ping'))
+    def test_not_skippable_04(self):
+        self.assertFalse(is_skippable_file('bin.rpmsave.ping'))
+    def test_not_skippable_05(self):
+        # normally is_skippable_file should be called without directory, but it shouldn't hurt too much
+        self.assertFalse(is_skippable_file('/etc/apparmor.d/bin.ping'))
+    def test_not_skippable_06(self):
+        self.assertFalse(is_skippable_file('bin.pingrej'))
+
+    def test_skippable_01(self):
+        self.assertTrue(is_skippable_file('bin.ping.dpkg-new'))
+    def test_skippable_02(self):
+        self.assertTrue(is_skippable_file('bin.ping.dpkg-old'))
+    def test_skippable_03(self):
+        self.assertTrue(is_skippable_file('bin.ping..dpkg-dist'))
+    def test_skippable_04(self):
+        self.assertTrue(is_skippable_file('bin.ping..dpkg-bak'))
+    def test_skippable_05(self):
+        self.assertTrue(is_skippable_file('bin.ping.rpmnew'))
+    def test_skippable_06(self):
+        self.assertTrue(is_skippable_file('bin.ping.rpmsave'))
+    def test_skippable_07(self):
+        self.assertTrue(is_skippable_file('bin.ping.orig'))
+    def test_skippable_08(self):
+        self.assertTrue(is_skippable_file('bin.ping.rej'))
+    def test_skippable_09(self):
+        self.assertTrue(is_skippable_file('bin.ping~'))
+    def test_skippable_10(self):
+        self.assertTrue(is_skippable_file('.bin.ping'))
+    def test_skippable_11(self):
+        self.assertTrue(is_skippable_file(''))  # empty filename
+    def test_skippable_12(self):
+        self.assertTrue(is_skippable_file('/etc/apparmor.d/'))  # directory without filename
+    def test_skippable_13(self):
+        self.assertTrue(is_skippable_file('README'))
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)



Regards,

Christian Boltz
-- 
"Bei mir" läuft KDE gar nicht.
Völlig korrekt. Logisch. Aber sinnfrei.
[David Haller in opensuse-de]