[apparmor] [patch] Move re_match_include() to regex.py
Kshitij Gupta
kgupta8592 at gmail.com
Fri Jun 19 12:53:18 UTC 2015
Hello,
On Fri, Jun 19, 2015 at 3:20 AM, Christian Boltz <apparmor at cboltz.de> wrote:
> Hello,
>
> Am Montag, 15. Juni 2015 schrieb Kshitij Gupta:
> > On Mon, Jun 15, 2015 at 12:44 AM, Christian Boltz wrote:
> > > this patch moves re_match_include() to regex.py. The function is
> > > basically a wrapper around a regex, so regex.py is a much better
> > > home.
> > >
>
> > While on it, rename the regex to RE_INCLUDE and compile it outside
> > > the function, which should result in a (small) performance
> > > improvement.
> > >
> > > Also adjust code calling it to the new location.
> > >
> > > Finally, add some tests for re_match_include()
> > >
> > >
> > >
> > > [ 49-move-re_match_include.diff ]
>
> > > === modified file utils/apparmor/regex.py
> > > --- utils/apparmor/regex.py 2015-06-14 20:33:26.205498791 +0200
> > > +++ utils/apparmor/regex.py 2015-06-14 20:14:39.423202789 +0200
> > > @@ -112,6 +112,19 @@
> > >
> > > return result
> > >
> > > +
> > > +RE_INCLUDE = re.compile('^\s*#?include\s*<(.*)>\s*(#.*)?$')
> >
> > How about replacing the end part using RE_EOL?
>
> Initially I just wanted to move the function, but... ;-)
>
> That said - good idea, and the tests added by this patch still pass
> after switching to RE_EOL.
>
> I also switched to named matches In case you wonder why I use the name
> 'magicpath' - that's what's used in man apparmor.d for <...> style
> includes. There's also the "include foo" variant which behaves
> differently, but the tools don't support that yet.
>
> > Also the .* for include name means we can have #include<>? I would
> > believe the check happens somewhere for valid paths this but why not
> > deal with the easy case in regex itself?
>
> That would effectively mean that #include <> will be handled as a
> comment (and silently ignored), not as syntax error. That doesn't sound
> like a good idea.
>
> good point!
> You have a good point that we should check for an empty filename -
> I added that in re_match_include() and also added some tests for it.
>
> nice!
> > Also, about should the regex be placed directly above the function
> > using it or along with all the other regexes?
>
> I prefer to have it next to the function - even if I didn't always do
> this ;-) (RE_PROFILE_CHANGE_PROFILE managed to get between
> RE_PROFILE_START and parse_profile_start_line() :-/ )
>
> > > +def re_match_include(path):
> > > + """Matches the path for include and returns the include path"""
> > > + match = RE_INCLUDE.search(path)
> > > + if match:
> > > + return match.groups()[0]
> >
> > hmm... for the matters of this function the second comment group in
> > regex is wasted.
>
> The whole comment group is optional, so we need to wrap it - it's hard
> or even impossible to rewrite (#.*)? to something without (...) ;-)
>
> > + else:
> > > + return None
> > > +
> > > +
> > > +
> >
> > Bikeshedding: 3 newlines or 2 newlines for separation?
>
> 2.6, and do the rounding depending on the moon phase ;-)
>
> ;-)
Seriously: I don't have a strict rule in my head besides "whatever looks
> good", but 2 are enough here.
>
> > > === modified file utils/test/test-regex_matches.py
> > >
> > > + (' #include <abstractions/base> ',
> > > 'abstractions/base'>
> > > ),
> >
> > could probably also use #include<abstractions/base> the no space
> > version?
>
> Good idea, added.
>
> > > + def _run_test(self, params, expected):
> > > + self.assertEqual(re_match_include(params), expected)
> > > +
> >
> > You could probably add just copy-paste (with little tweaking) the
> > above tests with the regex instead of the function and cover the
> > comment part.
>
> Nothing is expected to use the regex directly, so I don't see a need to
> explicitely test it ;-)
>
> > > class TestStripQuotes(AATest):
> > > def test_strip_quotes_01(self):
> > With suggestions for regex and tests addressed.
> >
> > Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.
>
> I did quite some changes (as described above) which are nearly a rewrite
> of re_match_include(), so please check the patch or at least the
> interdiff[1] again ;-)
>
>
>
> Here's the updated patch:
>
>
> Move re_match_include() to regex.py and improve it
>
> The function is basically a wrapper around a regex, so regex.py is a
> much better home.
>
> While on it, rename the regex to RE_INCLUDE, change it to named matches,
> use RE_EOL to handle comments and compile it outside the function, which
> should result in a (small) performance improvement.
>
> Also rewrite re_match_include() and let it check for empty include
> filenames ("#include <>") and let it raise AppArmorException in that case.
>
> Finally, adjust code calling it to the new location, and add some tests
> for re_match_include()
>
>
>
> [ 49-move-re_match_include.diff ]
>
> === modified file utils/aa-mergeprof
> --- utils/aa-mergeprof 2015-06-18 23:00:46.789404817 +0200
> +++ utils/aa-mergeprof 2015-06-14 20:13:24.204740154 +0200
> @@ -17,9 +17,10 @@
> import os
>
> import apparmor.aa
> -from apparmor.aa import available_buttons, combine_name,
> delete_duplicates, is_known_rule, match_includes, re_match_include
> +from apparmor.aa import available_buttons, combine_name,
> delete_duplicates, is_known_rule, match_includes
> import apparmor.aamode
> from apparmor.common import AppArmorException
> +from apparmor.regex import re_match_include
> import apparmor.severity
> import apparmor.cleanprofile as cleanprofile
> import apparmor.ui as aaui
> @@ -267,7 +268,7 @@
> done = True
> elif ans == 'CMD_ALLOW':
> selection = options[selected]
> - inc = apparmor.aa.re_match_include(selection)
> + inc = re_match_include(selection)
> self.user.filelist[self.user.filename]['include'][inc] =
> True
> options.pop(selected)
> aaui.UI_Info(_('Adding %s to the file.') % selection)
> @@ -302,7 +303,7 @@
> done = True
> elif ans == 'CMD_ALLOW':
> selection = options[selected]
> - inc = apparmor.aa.re_match_include(selection)
> + inc = re_match_include(selection)
> deleted =
> apparmor.aa.delete_duplicates(aa[profile][hat], inc)
> aa[profile][hat]['include'][inc] = True
> options.pop(selected)
> @@ -524,7 +525,7 @@
> elif ans == 'CMD_ALLOW':
> path = options[selected]
> done = True
> - match = apparmor.aa.re_match_include(path)
> + match = re_match_include(path)
> if match:
> inc = match
> deleted =
> apparmor.aa.delete_duplicates(aa[profile][hat], inc)
> @@ -589,7 +590,7 @@
>
> elif ans == 'CMD_NEW':
> arg = options[selected]
> - if not apparmor.aa.re_match_include(arg):
> + if not re_match_include(arg):
> ans = aaui.UI_GetString(_('Enter new
> path: '), arg)
> # if ans:
> # if not matchliteral(ans,
> path):
> @@ -603,7 +604,7 @@
>
> elif ans == 'CMD_GLOB':
> newpath = options[selected].strip()
> - if not
> apparmor.aa.re_match_include(newpath):
> + if not re_match_include(newpath):
> newpath =
> apparmor.aa.glob_path(newpath)
>
> if newpath not in options:
> @@ -614,7 +615,7 @@
>
> elif ans == 'CMD_GLOBEXT':
> newpath = options[selected].strip()
> - if not
> apparmor.aa.re_match_include(newpath):
> + if not re_match_include(newpath):
> newpath =
> apparmor.aa.glob_path_withext(newpath)
>
> if newpath not in options:
> === modified file utils/apparmor/aa.py
> --- utils/apparmor/aa.py 2015-06-18 23:00:46.791404710 +0200
> +++ utils/apparmor/aa.py 2015-06-15 00:22:30.094916147 +0200
> @@ -49,7 +49,7 @@
> RE_PROFILE_HAT_DEF, RE_PROFILE_DBUS,
> RE_PROFILE_MOUNT,
> RE_PROFILE_SIGNAL, RE_PROFILE_PTRACE,
> RE_PROFILE_PIVOT_ROOT,
> RE_PROFILE_UNIX, RE_RULE_HAS_COMMA,
> RE_HAS_COMMENT_SPLIT,
> - strip_quotes, parse_profile_start_line )
> + strip_quotes, parse_profile_start_line,
> re_match_include )
>
> import apparmor.rules as aarules
>
> @@ -2110,15 +2110,6 @@
>
> return newincludes
>
> -def re_match_include(path):
> - """Matches the path for include and returns the include path"""
> - regex_include = re.compile('^\s*#?include\s*<(.*)>\s*(#.*)?$')
> - match = regex_include.search(path)
> - if match:
> - return match.groups()[0]
> - else:
> - return None
> -
> def valid_include(profile, incname):
> if profile and profile['include'].get(incname, False):
> return False
> === modified file utils/apparmor/cleanprofile.py
> --- utils/apparmor/cleanprofile.py 2015-06-18 23:00:46.808403808 +0200
> +++ utils/apparmor/cleanprofile.py 2015-06-14 20:29:20.957207783 +0200
> @@ -12,6 +12,7 @@
> #
> # ----------------------------------------------------------------------
> import apparmor.aa as apparmor
> +from apparmor.regex import re_match_include
>
> class Prof(object):
> def __init__(self, filename):
> @@ -90,7 +91,7 @@
> if not same_profile:
> deleted.append(entry)
> continue
> - if apparmor.re_match_include(rule) or
> apparmor.re_match_include(entry):
> + if re_match_include(rule) or re_match_include(entry):
> continue
> # Check if the rule implies entry
> if apparmor.matchliteral(rule, entry):
> === modified file utils/apparmor/regex.py
> --- utils/apparmor/regex.py 2015-06-18 23:00:46.808403808 +0200
> +++ utils/apparmor/regex.py 2015-06-18 23:28:06.825232112 +0200
> @@ -112,6 +112,21 @@
> return result
>
>
> +RE_INCLUDE = re.compile('^\s*#?include\s*<(?P<magicpath>.*)>' + RE_EOL)
> +
> +def re_match_include(line):
> + """Matches the path for include and returns the include path"""
> + matches = RE_INCLUDE.search(line)
> +
> + if not matches:
> + return None
> +
> + if not matches.group('magicpath').strip():
> + raise AppArmorException(_('Syntax error: #include rule with empty
> filename'))
> +
> + return matches.group('magicpath')
>
If we can have people using #include< abstractions/base > (ignoring the
more terrible uses such as "#include < abstractions / base >") then,
looking at the usage of the returned value I believe the caller expects a
proper string, so I think the return statement should be:
matches.group('magicpath').strip()
+
> +
> def strip_quotes(data):
> if data[0] + data[-1] == '""':
> return data[1:-1]
> === modified file utils/test/test-regex_matches.py
> --- utils/test/test-regex_matches.py 2015-06-18 23:00:46.808403808 +0200
> +++ utils/test/test-regex_matches.py 2015-06-18 23:22:09.054007926 +0200
> @@ -12,9 +12,9 @@
> import apparmor.aa as aa
> import unittest
> from common_test import AATest, setup_all_loops
> -from apparmor.common import AppArmorBug
> +from apparmor.common import AppArmorBug, AppArmorException
>
> -from apparmor.regex import strip_quotes, parse_profile_start_line,
> RE_PROFILE_START, RE_PROFILE_CAP
> +from apparmor.regex import strip_quotes, parse_profile_start_line,
> re_match_include, RE_PROFILE_START, RE_PROFILE_CAP
>
>
> class AARegexTest(AATest):
> @@ -465,6 +465,34 @@
> with self.assertRaises(AppArmorBug):
> parse_profile_start_line(line, 'somefile')
>
> +class Test_re_match_include(AATest):
> + tests = [
> + ('#include <abstractions/base>', 'abstractions/base'
> ),
> + ('#include <abstractions/base> # comment', 'abstractions/base'
> ),
> + ('#include<abstractions/base>#comment', 'abstractions/base'
> ),
> + (' #include <abstractions/base> ', 'abstractions/base'
> ),
> + ('include <abstractions/base>', 'abstractions/base'
> ), # not supported by parser
> + # ('include foo', 'foo'
> ), # XXX not supported in tools yet
> + # ('include /foo/bar', '/foo/bar'
> ), # XXX not supported in tools yet
> + # ('include "foo"', 'foo'
> ), # XXX not supported in tools yet
> + # ('include "/foo/bar"', '/foo/bar'
> ), # XXX not supported in tools yet
> + (' some #include <abstractions/base>', None,
> ),
> + (' /etc/fstab r,', None,
> ),
> + ]
> +
> + def _run_test(self, params, expected):
> + self.assertEqual(re_match_include(params), expected)
> +
> +class TestInvalid_re_match_include(AATest):
> + tests = [
> + ('#include <>', AppArmorException ),
> + ('#include < >', AppArmorException ),
> + ]
> +
> + def _run_test(self, params, expected):
> + with self.assertRaises(expected):
> + re_match_include(params)
> +
>
> class TestStripQuotes(AATest):
> def test_strip_quotes_01(self):
>
> Thanks for the updated patch.
With surrounding spaces in includes suggestion addressed.
Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.
Regards,
Kshitij Gupta
>
>
> Regards,
>
> Christian Boltz
>
>
> [1] interdiff 49-move-re_match_include.OLD 49-move-re_match_include.diff
> diff -u utils/apparmor/regex.py utils/apparmor/regex.py
> --- utils/apparmor/regex.py 2015-06-14 20:14:39.423202789 +0200
> +++ utils/apparmor/regex.py 2015-06-18 23:28:06.825232112 +0200
> @@ -112,17 +112,19 @@
> return result
>
>
> +RE_INCLUDE = re.compile('^\s*#?include\s*<(?P<magicpath>.*)>' + RE_EOL)
>
> -RE_INCLUDE = re.compile('^\s*#?include\s*<(.*)>\s*(#.*)?$')
> -
> -def re_match_include(path):
> +def re_match_include(line):
> """Matches the path for include and returns the include path"""
> - match = RE_INCLUDE.search(path)
> - if match:
> - return match.groups()[0]
> - else:
> + matches = RE_INCLUDE.search(line)
> +
> + if not matches:
> return None
>
> + if not matches.group('magicpath').strip():
> + raise AppArmorException(_('Syntax error: #include rule with empty
> filename'))
> +
> + return matches.group('magicpath')
>
>
> def strip_quotes(data):
> diff -u utils/test/test-regex_matches.py utils/test/test-regex_matches.py
> --- utils/test/test-regex_matches.py 2015-06-14 20:14:57.120135248 +0200
> +++ utils/test/test-regex_matches.py 2015-06-18 23:22:09.054007926 +0200
> @@ -12,7 +12,7 @@
> import apparmor.aa as aa
> import unittest
> from common_test import AATest, setup_all_loops
> -from apparmor.common import AppArmorBug
> +from apparmor.common import AppArmorBug, AppArmorException
>
> from apparmor.regex import strip_quotes, parse_profile_start_line,
> re_match_include, RE_PROFILE_START, RE_PROFILE_CAP
>
> @@ -469,6 +469,7 @@
> tests = [
> ('#include <abstractions/base>', 'abstractions/base'
> ),
> ('#include <abstractions/base> # comment', 'abstractions/base'
> ),
> + ('#include<abstractions/base>#comment', 'abstractions/base'
> ),
> (' #include <abstractions/base> ', 'abstractions/base'
> ),
> ('include <abstractions/base>', 'abstractions/base'
> ), # not supported by parser
> # ('include foo', 'foo'
> ), # XXX not supported in tools yet
> @@ -482,6 +483,16 @@
> def _run_test(self, params, expected):
> self.assertEqual(re_match_include(params), expected)
>
> +class TestInvalid_re_match_include(AATest):
> + tests = [
> + ('#include <>', AppArmorException ),
> + ('#include < >', AppArmorException ),
> + ]
> +
> + def _run_test(self, params, expected):
> + with self.assertRaises(expected):
> + re_match_include(params)
> +
>
> class TestStripQuotes(AATest):
> def test_strip_quotes_01(self):
>
> --
> Spätestens dabei handelt es sich um Filtereffekte, die ImageMagick
> bestimmt nicht beherrschen kann. Sollten sie _das_ nachprogrammiert
> haben, würde ich barfuß hinlaufen und ihnen ein halbes Schwein
> opfern ob ihrer Genialität. [Ratti in suse-linux]
>
>
> --
> 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/20150619/0e548ed1/attachment-0001.html>
More information about the AppArmor
mailing list