[apparmor] [patch] update aa.py is_skippable_file() according to libapparmor
Kshitij Gupta
kgupta8592 at gmail.com
Wed Dec 24 09:36:20 UTC 2014
Hello,
On Sun, Dec 7, 2014 at 2:49 AM, Christian Boltz <apparmor at cboltz.de> wrote:
> Hello,
>
> this patch updates 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 - even if it merges 4 re.search() into
> one, 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
>
I'm not sure do we want to keep this or not. As stated parser_misc doesn't
ignore this.
A abc.txt.swp (open by vim) wont be covered under the dot file?
Also there are other ways a .swp file maybe created.
We'll get a bug report if it is meant to be there.
A user says even mv created a .swp file:
http://unix.stackexchange.com/questions/27923/what-causes-swap-files-to-be-created
- the python code ignores README files, but the C code doesn't
> (do we need to add README in the C code?)
>
Somebody else can answer this?
>
>
> [ is_skippable_file.diff ]
>
> === modified file 'utils/apparmor/aa.py'
> --- utils/apparmor/aa.py 2014-11-29 12:40:10 +0000
> +++ utils/apparmor/aa.py 2014-12-06 21:12:49 +0000
> @@ -2546,13 +2546,16 @@
> 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'):
> + """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 (without directory)"""
> +
> + skippable_suffix =
> '(\.dpkg-new|\.dpkg-old|\.dpkg-dist|\.dpkg-bak|\.rpmnew|\.rpmsave|\.orig|\.rej|~)$'
> + skippable_files = '^(.*/)?(README|\.[^/]*|)$' # README, dot files
> and empty filename
>
I suppose the number of calls to this function will be many as a large
number of files are parsed/ignored.
So, might I suggest that we (over?)optimise this?
I can think of two ways:
1. Use the regex and use re.compile for all the three regex's outside the
function call (It shows considerable speedup as tested by cProfile in
Python2.7)
2. Use simple string methods and leverage os.path submodule. This gives
probably performance better than uncompiled regex and only a bit better
than compiled versions.
An alternative version string based:
def is_skippable_file(path):
basename = os.path.basename(path)
if not basename or basename[-1] == '~' or basename[0] == '.' or
basename == 'README':
return True
skippable_extension = ['.dpkg-new', '.dpkg-old', '.dpkg-dist',
'.dpkg-bak', '.rpmnew', '.rpmsave', '.orig', '.rej']
extension = os.path.splitext(basename)[1]
if extension in skippable_extension:
return True
return False
Letme know if you want to have a look at the script I used to compare the
performance.
+ skippable_all = '%s|%s' % (skippable_suffix, skippable_files)
> +
> + if re.search(skippable_all, path):
> return True
>
> Also maybe add a "return False" at the end ;-)
> def is_skippable_dir(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 2014-12-06 21:06:15 +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)
>
>
> The tests look good. I'll hold-back the ACK for the modified version.
Please have a look at the suggestions.
Merry Christmas. (Apparently you were expecting an ACK under the christmas
tree ;-) )
Regards,
Kshitij Gupta
Regards,
>
> Christian Boltz
> --
> Wir brauchen ein "postfixbuchconf"-Kommando, damit wir Autor und Version
> bestimmen können... ;) [Patrick Ben Koetter in postfixbuch-users]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141224/e2d9396e/attachment.html>
More information about the AppArmor
mailing list