[apparmor] [patch] add tests for aamode.py

[Christian Boltz]

Hello,

Am Donnerstag, 27. November 2014 schrieb Steve Beattie:
> On Thu, Nov 27, 2014 at 02:06:11PM +0100, Christian Boltz wrote:

> > >>> split_log_mode('r::w::r')
> > 
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> >   File "/home/cb/apparmor/HEAD-clean/utils/apparmor/aamode.py", line
> > 106, in split_log_mode
> > 
> >     user, other = mode.split("::")
> > 
> > ValueError: too many values to unpack (expected 2)
> > 
> > 
> > The only function that calls split_log_mode() is str_to_mode() in
> > aamode.py. str_to_mode() doesn't do any validation of the result, so
> > raising an exception sounds like the better way.
> 
> If we're going to raise an exception that nobody's going to check for,
> wuld it be better to raise AppArmorBug, as that seems more
> appropriate, that either there's a bug in our code or we got a dodgy
> log message with a weird permission mode. Plus we can attach
> information about why things went wrong. Patch follows:

So we replace an exception that nobody is going to check for with 
another exception (with some additional details) that nobody is going to 
check for? ;-)

> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---
>  utils/apparmor/aamode.py  |    9 ++++++++-
>  utils/test/test-aamode.py |    7 +++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> Index: b/utils/apparmor/aamode.py
> ===================================================================
> --- a/utils/apparmor/aamode.py
> +++ b/utils/apparmor/aamode.py
> @@ -12,6 +12,7 @@
>  #
>  #
> ---------------------------------------------------------------------
> - import re
> +from apparmor.common import AppArmorBug
> 
>  def AA_OTHER(mode):
>      other = set()
> @@ -103,11 +104,17 @@ def split_log_mode(mode):
>      other = ''
> 
>      if "::" in mode:
> -        user, other = mode.split("::")
> +        try:
> +            user, other = mode.split("::")
> +        except ValueError as e:
> +            raise AppArmorBug("Got ValueError '%s' when splitting %s"
> % (str(e), mode)) 

That gives us a better error message than implicitely raising a 
ValueError, and isn't measurable in the execution time. So even if I 
joked about it above, it's an improvement.

>      else:
>          user = mode
>          other = mode
>      #print ('split_logmode:', user, mode)

Unrelated: please delete the "#print" line before commiting.

> +    if ":" in user or ":" in other:
> +        raise AppArmorBug("After splitting %s, user (%s) or other
> (%s) contained ':' " % (mode, user, other)) 

This check makes aa-logprof slightly slower (~2% - 1:10 instead of 1:09 
runtime for 20 aa-logprof runs on a 4 MB audit.log).

Valid data might be worth that - OTOH, the function doesn't check if 
user or other contain unknown letters, numbers or even special chars 
(splitting "yes!!11!!::no§42" will work successfully, at least until you 
try to use the result ;-)

So I'm undecided if adding this check really makes sense - given that we 
only check for ":" and allow any other strange character, it feels like 
a drop in the ocean ;-)

>  def mode_contains(mode, subset):
> Index: b/utils/test/test-aamode.py
> ===================================================================
> --- a/utils/test/test-aamode.py
> +++ b/utils/test/test-aamode.py
> @@ -12,6 +12,7 @@
>  import unittest
> 
>  from apparmor.aamode import split_log_mode, sub_str_to_mode
> +from apparmor.common import AppArmorBug
> 
>  class AamodeTest_split_log_mode(unittest.TestCase):
>      def test_split_log_mode_1(self):
> @@ -26,6 +27,12 @@ class AamodeTest_split_log_mode(unittest
>          self.assertEqual(split_log_mode('r::w'), ('r', 'w'))
>      def test_split_log_mode_6(self):
>          self.assertEqual(split_log_mode('rw::rw'), ('rw', 'rw'))
> +    def test_split_log_mode_invalid_1(self):
> +        with self.assertRaises(AppArmorBug):
> +            split_log_mode('r::w::r')
> +    def test_split_log_mode_invalid_2(self):
> +        with self.assertRaises(AppArmorBug):
> +            split_log_mode('r:::r')

With or without the check for ":":
    Acked-by: Christian Boltz <apparmor at cboltz.de>

(If you don't include the check for ":", you'll also have to remove 
test_split_log_mode_invalid_2() test)


Regards,

Christian Boltz
-- 
Intel kauft McAfee. [...] Der Synergieeffekt ist offensichtlich. 
McAfee macht Computer so langsam, dass man eine neue Intel-CPU braucht.
Ähnliche Synergieeffekte gibt es zwischen [...] Computerspielen und
Grafikkartenbedarf und zwischen Webforen und Tischkantenbedarf.
[http://blog.fefe.de/?ts=b2935cd1]