[apparmor] [patch] Fix aa-mergeprof crash with files containing multiple profiles

[Christian Boltz]

Hello,

Am Freitag, 12. Februar 2016, 03:26:36 CET schrieb Kshitij Gupta:
> On Sat, Dec 26, 2015, Christian Boltz <apparmor at cboltz.de> wrote:
> > if a profile file contains multiple profiles, aa-mergeprof crashes
> > on
> > saving in write_profile() because the second profile in the file is
> > not listed in 'changed'.
> > 
> > This patch first checks if 'changed' contains the profile before
> > pop()ing it.
> 
> Assuming, this is because the second profile is actually not changing

Exactly. (I should have mentioned this in the patch description.)

> )looking at the in and out files, only comment was removed)/
> I guess ideally we should probably have two lists of profiles for a
> file(changed and unchanged), to maintain that the profile is always
> processed and we may show this info elsewhere.

Not really - the list of changed profiles should stay exactly what it is, 
and we already make sure that we keep all profiles in a file.

One day, we'll need to support nested child profiles, and when we 
implement that, we'll probably also have to adjust several of our 
internal storage methods (aa[profile][hat] won't work anymore if you 
allow deeper nesting, and several other things that use a similar 
pattern  will also explode. Oh, and external hats already explode with 
the current code.)

While adjusting that, we can also address the file <-> profile 
relationship storage.

> > Reproducer: copy utils/test/cleanprof_test.in to your profile
> > directory and run   aa-mergeprof utils/test/cleanprof_test.out.
> > Then just press 's' to save the profile.
> > 
> > 
> > I can reproduce this with trunk, 2.10 and 2.9 and therefore propose
> > this patch for all these branches.
> > 
> > 
> > [ 47-fix-multi-profile-mergeprof-crash.diff ]
> > 
> > --- utils/apparmor/aa.py        2015-12-26 16:47:30.614839586 +0100
> > +++ utils/apparmor/aa.py        2015-12-26 17:27:36.376228122 +0100
> > @@ -4039,7 +4039,11 @@
> > 
> >      os.rename(newprof.name, prof_filename)
> > 
> > -    changed.pop(profile)
> > +    if profile in changed:
> > +        changed.pop(profile)
> > +    else:
> > +        debug_logger.error("%s written, but not listed in 'changed'
> > list" % profile)
> > +
> 
> We may rephrase it to something like: "Unchanged/unlisted profile %s
> written to file" with a log level of warn?

It will only be visible in the debug log (so it's unlikely someone sees 
it in practise), but I'll change the message to
    Unchanged profile written: %s

I won't switch to the warn loglevel because DebugLogger doesn't support 
that ;-) - thinking about it, info is probably the better choice.

> With above nit-pick considered:
> 
> Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>
> 
> for trunk, 2.10 and 2.9

Thanks!


Regards,

Christian Boltz
-- 
Wieviele Leute braucht es, um Windows zu installieren? - Sieben!
Einen, der sich die CD leisten kann, drei die ausdiskutieren, wie man
das wohl macht, zwei Priester, die beten, dass es funktioniert und einen
Psychiater, der die Nervenzusammenbrüche behandelt!!!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160211/301c31e0/attachment.pgp>