[apparmor] [patch] fix save_profile() by fixing some other code

[Steve Beattie]

On Thu, May 29, 2014 at 08:47:22PM +0200, Christian Boltz wrote:
> Hello,
> 
> when creating a child profile while using genprof, I get a backtrace:
> 
> Traceback (most recent call last):
>   File "aa-genprof", line 160, in <module>
>     lp_ret = apparmor.do_logprof_pass(logmark, passno)
>   File "/home/cb/apparmor/HEAD-CLEAN/utils/apparmor/aa.py", line 2291, in do_logprof_pass
>     save_profiles()
>   File "/home/cb/apparmor/HEAD-CLEAN/utils/apparmor/aa.py", line 2309, in save_profiles
>     for prof_name in changed.keys():
> RuntimeError: dictionary changed size during iteration
> 
> (See https://bugs.launchpad.net/apparmor/+bug/1014304 for more details.)
> 
> 
> After digging into the code, it seems for some reason the child profile 
> is added to "changed" - I doubt this is correct (guess why it's removed 
> later... ;-)
> 
> After digging a bit more, I found out that create_new_profile() is 
> (ab)used to create a new stub profile to be used as child profile. 
> create_new_profile then adds the new child (which looks like a normal 
> profile to it) to "changed".
> 
> This patch most probably makes the cleanup round in save_profile() 
> superfluous by adding a is_stub parameter to create_new_profile(). If
> this parameter is set, the new (child) profile is not added to "created" 
> and "changed".

Alright, after finally being able to reproduce this (need to use python
3, apparently), I can Acked-by: Steve Beattie <steve at nxnw.org> this
patch with a couple of formatting nits (no need to resubmit):

> === modified file 'utils/apparmor/aa.py'
> --- utils/apparmor/aa.py        2014-05-29 14:53:02 +0000
> +++ utils/apparmor/aa.py        2014-05-29 18:22:37 +0000
> @@ -386,7 +386,7 @@
>          return {local_profile: extras[local_profile]}
>      return dict()
>  
> -def create_new_profile(localfile):
> +def create_new_profile(localfile, is_stub = False):

For parameterized arguments like is_stub, the preferred pep8 style is to
not have spaces around the assigning equal sign, i.e. 'is_stub=False'.
This is pep8 error E251.

>      local_profile = hasher()
>      local_profile[localfile]['flags'] = 'complain'
>      local_profile[localfile]['include']['abstractions/base'] = 1
> @@ -428,8 +428,9 @@
>              for hat in sorted(cfg['required_hats'][hatglob].split()):
>                  local_profile[hat]['flags'] = 'complain'
>  
> -    created.append(localfile)
> -    changed[localfile] = True
> +    if not is_stub:
> +        created.append(localfile)
> +        changed[localfile] = True
>  
>      debug_logger.debug("Profile for %s:\n\t%s" % (localfile, local_profile.__str__()))
>      return {localfile: local_profile}
> @@ -1411,7 +1421,7 @@
>                                  if profile != hat:
>                                      aa[profile][hat]['flags'] = aa[profile][profile]['flags']
>  
> -                                stub_profile = create_new_profile(hat)
> +                                stub_profile = create_new_profile(hat, True)
>  
>                                  aa[profile][hat]['flags'] = 'complain'
>  
> @@ -2299,6 +2309,8 @@
>      # Ensure the changed profiles are actual active profiles
>      for prof_name in changed.keys():
>          if not is_active_profile(prof_name):
> +            print("*** save_profiles(): removing %s"%prof_name)

And here, pythonic style is have spaces around the '%' variable
substitution indicator, e.g. '[...] removing %s" % prof_name)'.
This is pep8 error E228.

> +            print('*** This should not happen. Please open a bugreport!')
>              changed.pop(prof_name)
>  
>      changed_list = sorted(changed.keys())
> 
> 
> BTW: I intentionally added the two print() lines in safe_profile because
> a) I think they will never be displayed
> b) I want to know if a is wrong ;-)
> c) it's always nice to have a "nice" error message before displaying
>    a backtrace ;-)
> 
> A final note: I hereby officially blame Steve for introducing this bug 
> in AppArmor.pm r2019 without checking the side effects of this function.
> (Kshitij is innocent because he just translated this well-hidden bug to
> python ;-)

Sure, I'll happily fall on my sword, here. That said, long term,
I want the deeply nested hashes/dictionaries to be replaced with
actual python classes.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140609/7f37eb83/attachment-0001.pgp>