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.
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.
> 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 <kgupta8...@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')
+
+
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):
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@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
