[apparmor] [patch] introduce AppArmorBug exceptions

[Christian Boltz]

Hello,

Am Mittwoch, 12. November 2014 schrieb Steve Beattie:
> On Sun, Nov 09, 2014 at 11:05:38PM +0100, Christian Boltz wrote:
> > while developing the capability rule class, I added lots of error
> > handling, including several checks for "things that should never
> > happen"[tm], and raise an exception in those cases.

> > I'd like to have a separate exception class for these things to
> > a) clearly separate them from "expected" exceptions (for example
> > invalid> 
> >    lines in a profile is more or less "expected")
> > 
> > b) allow us to disable backtraces for the "expected"
> > AppArmorExceptions,> 
> >    while still having full debug output for unexpected issues
> > 
> > The naming of the new exception type is an interesting question -
> > I'd
> > just use "AppArmorBug", but Kshitij hopes we can use something like
> > AppArmorSledgeHammerException or SledgehammerBug ;-)
> 
> I'm not entirely sure I see the distinction; these sound more like
> assert()-like sanity checks. Which is fine, I don't mind having them
> broken out into a separate Exception class (AppArmorAssert?).

I'd prefer the name AppArmorBug because it makes clear something is 
really broken. AppArmorAssert sounds too harmless ;-)

For the distinction, let me give you an example:

a) AppArmorException: aa-logprof parses the profiles and finds a line 
   that doesn't match any of the RE_* patterns. That's probably a syntax
   error in the profile - something that can happen after manually 
   editing a profile.

b) AppArmorBug: parse_audit_allow() finds out that matches.group('allow') 
   contains "maybe" (hint: the regex explicitely checks for "allow|deny",
   so someone must have handed over a match object from a wrong regex -
   that's clearly a bug.

> That said, for broken policy type stuff, it'd be nice to suggest to
> the user what's gone wrong and suggestions on how to fix it (yes, the
> parser needs a lot of work on that, too; alas, lex and yacc are not
> the friendliest for accomplishing such things.

My TODO list is already long enough ;-)

> As for your short-term implmentation:

> All the behavior here is a duplicate of the AppArmorException.  So it
> would probably be better to inherit from it and override the behaviors
> you want to change; e.g. for the time being:
> 
> class
> AppArmorExceptionClassToBeNamedLaterAfterManyLongTediousArgumentsOnIR
> C(AppArmorException): 
>     '''This class represents AppArmor exceptions "that should never happen"''' 
>     pass

KMail thinks your class name is too long ;-)

Actually the goal is to use cgitb for all exceptions except 
AppArmorException (which should just print the summary, but not a 
backtrace) - but that's another patch (IIRC Kshitij has a draft for that 
already).

This also means that AppArmorBug should be based on "Exception" so that 
it doesn't inherit the (planned) changed behaviour of AppArmorException. 
However I don't see a problem with making AppArmorBug behaving like a 
normal Exception, so the updated patch is:

=== modified file 'utils/apparmor/common.py'
--- utils/apparmor/common.py    2014-10-14 10:54:39 +0000
+++ utils/apparmor/common.py    2014-11-13 17:38:31 +0000
@@ -35,6 +35,10 @@
     def __str__(self):
         return repr(self.value)
 
+class AppArmorBug(Exception):
+    '''This class represents AppArmor exceptions "that should never happen"'''
+    pass
+
 #
 # Utility functions
 #



Regards,

Christian Boltz
-- 
> Kann ich auf einen Bootloader (lilo oder grub) verzichten,
> falls auf der Festplatte nur 2 Partitionen sind
Klar kannst du. Vorausgesetzt du kannst auch darauf verzichten
das Betriebssystem zu booten.
[> Wolfgang Erlenkötter und Hartmut Meyer in suse-linux]