[apparmor] [patch] delete_duplicates(): don't modify self.rules while looping over it

[Seth Arnold]

On Sun, Aug 07, 2016 at 04:57:44PM +0200, Christian Boltz wrote:
> by calling self.delete() inside the delete_duplicates() loop, the
> self.rules list was modified. This resulted in some rules not being
> checked and therefore (some, not all) superfluous rules not being
> removed.
> 
> This patch switches to a temporary variable to loop over, and rebuilds
> self.rules with the rules that are not superfluous.
> 
> This also fixes some strange issues already marked with a "Huh?" comment
> in the tests.
> 
> 
> I propose this patch for trunk and 2.10.
> Note that in 2.10 cleanprof_test.* doesn't contain a ptrace rule,
> therefore the cleanprof_test.out change doesn't make sense for 2.10.

Nice find.

Acked-by: Seth Arnold <seth.arnold at canonical.com>

Acked for both branches.

Thanks

> 
> 
> [ 01-delete_duplicates-dont-modify-list-in-loop.diff ]
> 
> --- utils/apparmor/rule/__init__.py     2016-07-31 19:12:31.537453276 +0200
> +++ utils/apparmor/rule/__init__.py     2016-08-07 16:32:19.435814124 +0200
> @@ -431,10 +431,13 @@
>  
>          # delete rules that are covered by include files
>          if include_rules:
> -            for rule in self.rules:
> -                if include_rules.is_covered(rule, True, True):
> -                    self.delete(rule)
> +            oldrules = self.rules
> +            self.rules = []
> +            for rule in oldrules:
> +                if include_rules.is_covered(rule, True, False):
>                      deleted += 1
> +                else:
> +                    self.rules.append(rule)
>  
>          # de-duplicate rules inside the profile
>          deleted += self.delete_in_profile_duplicates()
> --- utils/test/cleanprof_test.out       2016-07-31 19:12:31.517453376 +0200
> +++ utils/test/cleanprof_test.out       2016-08-07 16:37:22.402324691 +0200
> @@ -16,8 +16,6 @@
>  
>    signal set=(abrt alrm bus chld fpe hup ill int kill pipe quit segv stkflt term trap usr1 usr2),
>  
> -  ptrace tracedby,
> -
>    unix (receive) type=dgram,
>  
>    allow /home/*/** r,
> --- utils/test/test-capability.py       2015-11-19 17:42:26.325879118 +0100
> +++ utils/test/test-capability.py       2016-08-07 16:40:33.097385534 +0200
> @@ -817,7 +817,6 @@
>              inc.add(CapabilityRule.parse(rule))
>  
>          expected_raw = [
> -            '  allow capability sys_admin,',  # XXX huh? should be deleted!
>              '  deny capability chgrp, # example comment',
>              '',
>          ]
> @@ -825,11 +824,9 @@
>          expected_clean = [
>              '  deny capability chgrp, # example comment',
>              '',
> -            '  allow capability sys_admin,',  # XXX huh? should be deleted!
> -            '',
>          ]
>  
> -        self.assertEqual(self.ruleset.delete_duplicates(inc), 1)
> +        self.assertEqual(self.ruleset.delete_duplicates(inc), 2)
>          self.assertEqual(expected_raw, self.ruleset.get_raw(1))
>          self.assertEqual(expected_clean, self.ruleset.get_clean(1))
>  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160808/96ac18a4/attachment.pgp>