[apparmor] [patch] aa.py get_output(): raise exception on non-executable or non-existing programs

[Kshitij Gupta]

Hello,

On Mon, Feb 22, 2016 at 1:28 AM, Christian Boltz <apparmor at cboltz.de> wrote:

> Hello,
>
> if the program specified as get_output param isn't executable or doesn't
> exist at all, get_output() returns with ret = -1.
>
> Raising an exception looks like a better option, especially because
> other possible exec failures already raise an exception ("Unable to
> fork").
>
> Note: get_output is only used by get_reqs() which also does the
> os.access() check for x permissions (and raises an exception), so in
> practise raising an exception in get_output() doesn't change anything.
>
>
> This change also allows to rewrite and simplify get_output() quite a bit.
>
>
> Another minor change (and fix) is in the removal of the last line. The
> old code removed the last line if output contained at least two items.
> This had two not-so-nice effects:
> - an empty output resulted in [''] instead of []
> - if a command didn't add a \n on the last line, this line was deleted
>   nevertheless
>
> The patch changes that to always remove the last line if it is empty,
> which fixes both issues mentioned above.
>
>
> Also add a test to ensure the exception is really raised, and adjust the
> test that expects an empty stdout.
>
>
>
>
> [ 67-get_output-dont-ignore-non-executable.diff ]
>
> --- utils/test/test-aa.py       2016-02-01 20:06:26.082212471 +0100
> +++ utils/test/test-aa.py       2016-02-01 20:12:46.228094146 +0100
> @@ -77,12 +77,16 @@
>      tests = [
>          (['./fake_ldd', '/AATest/lib64/libc-2.22.so'],  (0, ['
> /AATest/lib64/ld-linux-x86-64.so.2 (0x0000556858473000)', '
> linux-vdso.so.1 (0x00007ffe98912000)']     )),
>          (['./fake_ldd', '/tmp/aa-test-foo'],            (0, ['        not
> a dynamic executable']
>                               )),
> -        (['./fake_ldd', 'invalid'],                     (1, ['']
>
>                               )),  # stderr is not part of output
> +        (['./fake_ldd', 'invalid'],                     (1, []
>
>                               )),  # stderr is not part of output
>      ]
>      def _run_test(self, params, expected):
>          self.assertEqual(get_output(params), expected)
>
> +    def test_get_output_nonexisting(self):
> +        with self.assertRaises(AppArmorException):
> +            ret, output = get_output(['./_file_/_not_/_found_'])
> +
>  class AATest_get_reqs(AATest):
>      tests = [
>          ('/AATest/bin/bash',    ['/AATest/lib64/libreadline.so.6',
> '/AATest/lib64/libtinfo.so.6', '/AATest/lib64/libdl.so.2',
> '/AATest/lib64/libc.so.6', '/AATest/lib64/ld-linux-x86-64.so.2']),
>
> --- utils/apparmor/aa.py        2016-02-01 20:06:26.086212449 +0100
> +++ utils/apparmor/aa.py        2016-02-01 20:12:27.192200872 +0100
> @@ -334,27 +334,21 @@
>
>  def get_output(params):
> -    """Returns the return code output by running the program with the
> args given in the list"""
> +    '''Runs the program with the given args and returns the return code
> and stdout (as list of lines)'''
> -    program = params[0]
> -    # args = params[1:]
> -    ret = -1
> -    output = []
> -    # program is executable
> -    if os.access(program, os.X_OK):
> -        try:
> -            # Get the output of the program
> -            output = subprocess.check_output(params)
> -        except OSError as e:
> -            raise AppArmorException(_("Unable to fork:
> %(program)s\n\t%(error)s") % { 'program': program, 'error': str(e) })
> -            # If exit-codes besides 0
> -        except subprocess.CalledProcessError as e:
> -            output = e.output
> -            output = output.decode('utf-8').split('\n')
> -            ret = e.returncode
> -        else:
> -            ret = 0
> -            output = output.decode('utf-8').split('\n')
> +    try:
> +        # Get the output of the program
> +        output = subprocess.check_output(params)
> +        ret = 0
> +    except OSError as e:
> +        raise AppArmorException(_("Unable to fork:
> %(program)s\n\t%(error)s") % { 'program': params[0], 'error': str(e) })
> +    except subprocess.CalledProcessError as e:  # If exit-codes besides 0
>
That comment looks off wrt "exit-codes", better written as: # exit code != 0
,or something along that line.


> +        output = e.output
> +        ret = e.returncode
> +
> +    output = output.decode('utf-8').split('\n')
> +
>      # Remove the extra empty string caused due to \n if present
> -    if len(output) > 1:
> +    if output[len(output) - 1] == '':
>
maybe worthwhile to add a .strip() just in case.

Nicely cleaned up!

Thanks for the patch.

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>

         output.pop()
> +
>      return (ret, output)
>
>
>
>
>
> Regards,
>
> Christian Boltz
> --
> Der geistige Horizont ist der Abstand zwischen Brett und Kopf.
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>


-- 
Regards,

Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160222/54b10ed7/attachment.html>