On Wed, Apr 08, 2015 at 12:25:05AM +0200, Christian Boltz wrote:
> Hello,
>
> the LibreOffice profile uncovered that handling of @{var} += is broken:
>
> File ".../utils/apparmor/aa.py", line 3272, in store_list_var
> var[list_var] = set(var[list_var] + vlist)
> TypeError: unsupported operand type(s) for +: 'set' and 'list'
>
> This patch fixes it:
> - change separate_vars() to use and return a set instead of a list
> (FYI: separate_vars() is only called by store_list_var())
> - adoptstore_list_var() to expect a set
> - remove some old comments in these functions
> - explain the less-intuitive parameters of store_list_var()
>
> Also add some tests for separate_vars() and store_list_var().
> The tests were developed based on the old code, but not all of them
> succeed with the old code.
>
> As usual, the tests uncovered some interesting[tm] behaviour in
> separate_vars() (see the XXX comments and tell me what the really
> expected behaviour is ;-)
>
> Since this is broken in trunk and the 2.9 branch, I propose this patch
> for trunk and 2.9.
>
> Oh, and I noticed that serialize_profile_from_old_profile() also crashes
> when it hits "@{var} +=", but I didn't debug that yet.
>
>
> [ 33-fix-add-to-variable-and-add-tests.diff ]Acked-by: Steve Beattie <[email protected]> for trunk and 2.9 Some comments on the questioned test cases inlined: > === modified file utils/apparmor/aa.py > --- utils/apparmor/aa.py 2015-04-08 00:09:02.931423160 +0200 > +++ utils/apparmor/aa.py 2015-04-08 00:13:59.273822091 +0200 > @@ -3241,13 +3241,12 @@ > > def separate_vars(vs): > """Returns a list of all the values for a variable""" > - data = [] > + data = set() > > - #data = [i.strip('"') for i in vs.split()] > RE_VARS = re.compile('\s*((\".+?\")|([^\"]\S+))\s*(.*)$') > while RE_VARS.search(vs): > matches = RE_VARS.search(vs).groups() > - data.append(strip_quotes(matches[0])) > + data.add(strip_quotes(matches[0])) > vs = matches[3] > > return data > @@ -3259,17 +3258,19 @@ > return False > > def store_list_var(var, list_var, value, var_operation, filename): > - """Store(add new variable or add values to variable) the variables > encountered in the given list_var""" > + """Store(add new variable or add values to variable) the variables > encountered in the given list_var > + - the 'var' parameter will be modified > + - 'list_var' is the variable name, for example '@{foo}' > + """ > vlist = separate_vars(value) > if var_operation == '=': > if not var.get(list_var, False): > var[list_var] = set(vlist) > else: > - #print('Ignored: New definition for variable for:',list_var,'=', > value, 'operation was:',var_operation,'old value=', var[list_var]) > raise AppArmorException(_('Redefining existing variable > %(variable)s: %(value)s in %(file)s') % { 'variable': list_var, 'value': > value, 'file': filename }) > elif var_operation == '+=': > if var.get(list_var, False): > - var[list_var] = set(var[list_var] + vlist) > + var[list_var] |= vlist > else: > raise AppArmorException(_('Values added to a non-existing > variable %(variable)s: %(value)s in %(file)s') % { 'variable': list_var, > 'value': value, 'file': filename }) > else: > === modified file utils/test/test-aa.py > --- utils/test/test-aa.py 2015-04-08 00:09:02.934422982 +0200 > +++ utils/test/test-aa.py 2015-04-07 23:14:27.316826914 +0200 > @@ -16,7 +16,7 @@ > import tempfile > from common_test import read_file, write_file > > -from apparmor.aa import check_for_apparmor, get_profile_flags, > set_profile_flags, is_skippable_file, is_skippable_dir, parse_profile_start, > write_header, serialize_parse_profile_start > +from apparmor.aa import check_for_apparmor, get_profile_flags, > set_profile_flags, is_skippable_file, is_skippable_dir, parse_profile_start, > separate_vars, store_list_var, write_header, serialize_parse_profile_start > from apparmor.common import AppArmorException, AppArmorBug > > class AaTestWithTempdir(AATest): > @@ -349,6 +349,66 @@ > with self.assertRaises(AppArmorBug): > self._parse('xy', '/bar', '/bar') # not a profile start > > +class AaTest_separate_vars(AATest): > + tests = [ > + ('' , set() ), > + (' ' , set() ), > + (' foo bar' , {'foo', 'bar' }), > + ('foo " ' , {'foo' }), # > XXX " is ignored If this is the raw variable assignment string included in the profile, then this is invalid syntax (the parser will want a close quote): $ echo -e '@{FOO}=foo " \n profile /t { /@{FOO} r, }' @{FOO}=foo " profile /t { /@{FOO} r, } $ echo -e '@{FOO}=foo " \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables AppArmor parser error, in stdin line 1: Found unexpected character: '"' > + (' " foo ' , {' "', 'foo' }), # > XXX really? Same. Note however, that a backslash escaped " is valid: $ echo -e '@{FOO}= \" foo \n profile /t { /@{FOO} r, }' @{FOO}= \" foo profile /t { /@{FOO} r, } $ echo -e '@{FOO}= \" foo \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables @FOO = "foo" """ > + (' foo bar ' , {'foo', 'bar' }), > + (' foo bar # comment' , {'foo', 'bar', 'comment' }), # > XXX should comments be stripped? Hrm, I think the parser has a bug here: $ echo -e '@{FOO}= foo # comment \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables @FOO = "comment" "#" "foo" > + ('foo' , {'foo' }), > + ('"foo" "bar baz"' , {'foo', 'bar baz' }), > + ('foo "bar baz" xy' , {'foo', 'bar baz', 'xy' }), > + ('foo "bar baz ' , {'foo', 'bar', 'baz' }), # > half-quoted Again, invalid syntax: $ echo -e '@{FOO}= foo "bar baz \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables AppArmor parser error, in stdin line 1: Found unexpected character: '"' > + (' " foo" bar' , {' foo', 'bar' }), > + ] > + > + def _run_test(self, params, expected): > + result = separate_vars(params) > + self.assertEqual(result, expected) > + > + > +class AaTest_store_list_var(AATest): > + tests = [ > + # old var value operation expected > (False for exception) > + ([ {} , 'foo' , '=' ], {'foo'} > ), # set > + ([ {} , 'foo bar' , '=' ], {'foo', > 'bar'} ), # set multi > + ([ {'@{var}': {'foo'}} , 'bar' , '=' ], False > ), # redefine var > + ([ {} , 'bar' , '+=' ], False > ), # add to undefined var > + ([ {'@{var}': {'foo'}} , 'bar' , '+=' ], {'foo', > 'bar'} ), # add > + ([ {'@{var}': {'foo'}} , 'bar baz' , '+=' ], {'foo', > 'bar', 'baz'} ), # add multi > + ([ {'@{var}': {'foo', 'xy'}} , 'bar baz' , '+=' ], {'foo', > 'xy', 'bar', 'baz'} ), # add multi to multi > + ([ {} , 'foo' , '-=' ], False > ), # unknown operation > + ] > + > + def _run_test(self, params, expected): > + var = params[0] > + value = params[1] > + operation = params[2] > + > + if not expected: > + with self.assertRaises(AppArmorException): > + store_list_var(var, '@{var}', value, operation, 'somefile') > + return > + > + # dumy value that must not be changed > + var['@{foo}'] = {'one', 'two'} > + > + exp_var = { > + '@{foo}': {'one', 'two'}, > + '@{var}': expected, > + } > + > + store_list_var(var, '@{var}', value, operation, 'somefile') > + > + self.assertEqual(var.keys(), exp_var.keys()) > + > + for key in exp_var: > + self.assertEqual(var[key], exp_var[key]) > + > + > class AaTest_write_header(AATest): > tests = [ > # name embedded_hat write_flags depth flags > attachment prof.keyw. comment expected -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
