Hello, On Mon, Feb 22, 2016 at 1:28 AM, Christian Boltz <appar...@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 <kgupta8...@gmail.com> output.pop() > + > return (ret, output) > > > > > > Regards, > > Christian Boltz > -- > Der geistige Horizont ist der Abstand zwischen Brett und Kopf. > > -- > AppArmor mailing list > AppArmor@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor > > -- Regards, Kshitij Gupta
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor