<div dir="ltr">Hello, <div class="gmail_extra"> <div class="gmail_quote">On Mon, Feb 22, 2016 at 1:28 AM, Christian Boltz <<a href="mailto:apparmor@cboltz.de" target="_blank">apparmor@cboltz.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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/<a href="http://libc-2.22.so" rel="noreferrer" target="_blank">libc-2.22.so</a>'], (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 </blockquote><div>That comment looks off wrt "exit-codes", better written as: # exit code != 0 ,or something along that line. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + 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] == '': </blockquote><div>maybe worthwhile to add a .strip() just in case. Nicely cleaned up! Thanks for the patch. Acked-by: Kshitij Gupta <<a href="mailto:kgupta8592@gmail.com" target="_blank">kgupta8592@gmail.com</a>> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> output.pop() + return (ret, output) Regards, Christian Boltz -- Der geistige Horizont ist der Abstand zwischen Brett und Kopf. -- AppArmor mailing list <a href="mailto:AppArmor@lists.ubuntu.com">AppArmor@lists.ubuntu.com</a> Modify settings or unsubscribe at: <a href="https://lists.ubuntu.com/mailman/listinfo/apparmor" rel="noreferrer" target="_blank">https://lists.ubuntu.com/mailman/listinfo/apparmor</a> </blockquote></div> -- <div class="gmail_signature"><div dir="ltr"><div>Regards, </div>Kshitij Gupta </div></div> </div></div>