On Fri, Sep 06, 2013 at 11:57:51AM -0700, Tyler Hicks wrote: > Nice catch! (and shame on me for not using valgrind)
Well, we don't really make it easy to run valgrind regularly. In the interest of fixing that, attached is a simple script for doing so, along with a separate patch for fixing a few leaks that it found; most occur in error paths, though there's a couple of legitimate ones it found in parser_yacc.y. There are also more reported leaks that I have not fully investigated yet. It's also finding what may be some false positives around invalid reads around profile serialization (e.g. on simple_tests/conditional/else_if_5.sd) and conditional jump or move depends on uninitialized value(s) (e.g. simple_tests/network/network_ok_1.sd) that need more investigation, to determine whether it's a legitimate issue or a valgrind check that hopefully can be disabled. Running the parser under valgrind on all 69000+ simple test cases takes *way* too long to include it on by default; I kicked off a run in a vm last night before going to bed and 10 hours later, it's still going over the generated dbus tests. Thus I didn't include it in the default 'make check' target. -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
Subject: parser - add simple valgrind wrapper tests This patch adds a test wrapper that runs valgrind on the parser over the simple_tests tree (or other directory tree if passed on the command line). An alternate parser location can also be passed on the command line. Like the libapparmor python bindings test, this test uses a bit of magic to generate tests that doesn't work with auto-detecting test utilities like nose. Running valgrind on the parser over all 69000+ testcases takes several hours, so while this patch includes a make target 'make valgrind', it does not add it to the set of tests run when 'make check' is called. Perhaps a 'make extra-tests' target is in order. Signed-off-by: Steve Beattie <[email protected]> --- parser/tst/Makefile | 3 parser/tst/valgrind_simple.py | 141 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) Index: b/parser/tst/valgrind_simple.py =================================================================== --- /dev/null +++ b/parser/tst/valgrind_simple.py @@ -0,0 +1,141 @@ +#!/usr/bin/env python +# ------------------------------------------------------------------ +# +# Copyright (C) 2013 Canonical Ltd. +# Author: Steve Beattie <[email protected]> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of version 2 of the GNU General Public +# License published by the Free Software Foundation. +# +# ------------------------------------------------------------------ + +from optparse import OptionParser # deprecated, should move to argparse eventually +import os +import signal +import subprocess +import unittest + +DEFAULT_TESTDIR = "./simple_tests/vars" +DEFAULT_PARSER = '../apparmor_parser' +VALGRIND_ERROR_CODE = 151 +TIMEOUT_ERROR_CODE = 152 +VALGRIND_ARGS = ['--leak-check=full', '--error-exitcode=%d' % (VALGRIND_ERROR_CODE)] + + +# http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html +# This is needed so that the subprocesses that produce endless output +# actually quit when the reader goes away. +def subprocess_setup(): + # Python installs a SIGPIPE handler by default. This is usually not + # what non-Python subprocesses expect. + signal.signal(signal.SIGPIPE, signal.SIG_DFL) + + +def run_cmd(command, input=None, stderr=subprocess.STDOUT, stdout=subprocess.PIPE, stdin=None, timeout=120): + '''Try to execute given command (array) and return its stdout, or + return a textual error if it failed.''' + + try: + sp = subprocess.Popen(command, stdin=stdin, stdout=stdout, stderr=stderr, close_fds=True, preexec_fn=subprocess_setup) + except OSError as e: + return [127, str(e)] + + timeout_communicate = TimeoutFunction(sp.communicate, timeout) + out, outerr = (None, None) + try: + out, outerr = timeout_communicate(input) + rc = sp.returncode + except TimeoutFunctionException as e: + sp.terminate() + outerr = b'test timed out, killed' + rc = TIMEOUT_ERROR_CODE + + # Handle redirection of stdout + if out is None: + out = b'' + # Handle redirection of stderr + if outerr is None: + outerr = b'' + return [rc, out.decode('utf-8') + outerr.decode('utf-8')] + + +# Timeout handler using alarm() from John P. Speno's Pythonic Avocado +class TimeoutFunctionException(Exception): + """Exception to raise on a timeout""" + pass + + +class TimeoutFunction: + def __init__(self, function, timeout): + self.timeout = timeout + self.function = function + + def handle_timeout(self, signum, frame): + raise TimeoutFunctionException() + + def __call__(self, *args, **kwargs): + old = signal.signal(signal.SIGALRM, self.handle_timeout) + signal.alarm(self.timeout) + try: + result = self.function(*args, **kwargs) + finally: + signal.signal(signal.SIGALRM, old) + signal.alarm(0) + return result + + +class AAParserValgrindTests(unittest.TestCase): + def setUp(self): + # REPORT ALL THE OUTPUT + self.maxDiff = None + + def _runtest(self, testname, config): + parser_args = ['-Q', '-I', config.testdir] + failure_rc = [VALGRIND_ERROR_CODE, TIMEOUT_ERROR_CODE] + command = ['valgrind'] + command.extend(VALGRIND_ARGS) + command.append(config.parser) + command.extend(parser_args) + command.append(testname) + rc, output = run_cmd(command, timeout=120) + self.assertNotIn(rc, failure_rc, + "valgrind returned error code %d, gave the following output\n%s" % (rc, output)) + + +def find_testcases(testdir): + '''dig testcases out of passed directory''' + + for (fdir, direntries, files) in os.walk(testdir): + for f in files: + if f.endswith(".sd"): + yield os.path.join(fdir, f) + + +def main(): + usage = "usage: %prog [options] [test_directory]" + p = OptionParser(usage=usage) + p.add_option('-p', '--parser', default=DEFAULT_PARSER, action="store", type="string", dest='parser') + p.add_option('-v', '--verbose', action="store_true", dest="verbose") + config, args = p.parse_args() + + verbosity = 1 + if config.verbose: + verbosity = 2 + + if len(args) == 1: + config.testdir = args[0] + else: + config.testdir = DEFAULT_TESTDIR + + for f in find_testcases(config.testdir): + def stub_test(self, testname=f): + self._runtest(testname, config) + stub_test.__doc__ = "test %s" % (f) + setattr(AAParserValgrindTests, 'test_%s' % (f), stub_test) + test_suite = unittest.TestSuite() + test_suite.addTest(unittest.TestLoader().loadTestsFromTestCase(AAParserValgrindTests)) + return unittest.TextTestRunner(verbosity=verbosity).run(test_suite) + +if __name__ == "__main__": + main() Index: b/parser/tst/Makefile =================================================================== --- a/parser/tst/Makefile +++ b/parser/tst/Makefile @@ -50,6 +50,9 @@ minimize: $(PARSER) equality: $(PARSER) LANG=C APPARMOR_PARSER="$(PARSER)" ./equality.sh +valgrind: $(PARSER) + LANG=C ./valgrind_simple.py -p "$(PARSER)" -v simple_tests + $(PARSER): make -C $(PARSER_DIR) $(PARSER_BIN)
Subject: parser - fix memory leaks identified by valgrind tests This patch fixes a few memory leaks found by valgrind. Most of these occur in error cases and as such, are not a big deal. The dbus TOK_MODE and flags TOK_CONDID leaks in parser_yacc.y are legitimate leaks, if of very small amounts of memory. Signed-off-by: Steve Beattie <[email protected]> --- parser/parser_merge.c | 1 + parser/parser_symtab.c | 3 +++ parser/parser_yacc.y | 2 ++ 3 files changed, 6 insertions(+) Index: b/parser/parser_symtab.c =================================================================== --- a/parser/parser_symtab.c +++ b/parser/parser_symtab.c @@ -399,6 +399,7 @@ static int __expand_variable(struct symt PERROR("Variable @{%s} is referenced recursively (by @{%s})\n", split->var, symbol->var_name); retval = 1; + free_values(this_value); goto out; } @@ -407,12 +408,14 @@ static int __expand_variable(struct symt PERROR("Variable @{%s} references undefined variable @{%s}\n", symbol->var_name, split->var); retval = 3; + free_values(this_value); goto out; } rc = __expand_variable(ref); if (rc != 0) { retval = rc; + free_values(this_value); goto out; } Index: b/parser/parser_merge.c =================================================================== --- a/parser/parser_merge.c +++ b/parser/parser_merge.c @@ -111,6 +111,7 @@ static int process_file_entries(struct c PERROR(_("profile %s: has merged rule %s with " "conflicting x modifiers\n"), cod->name, cur->name); + free(table); return 0; } //if (next->audit) Index: b/parser/parser_yacc.y =================================================================== --- a/parser/parser_yacc.y +++ b/parser/parser_yacc.y @@ -436,6 +436,7 @@ opt_flags: { /* nothing */ $$ = 0; } { if (strcmp($1, "flags") != 0) yyerror("expected flags= got %s=", $1); + free($1); $$ = 1; } @@ -1210,6 +1211,7 @@ dbus_perm: TOK_VALUE | TOK_MODE { parse_dbus_mode($1, &$$, 1); + free($1); } dbus_perms: { /* nothing */ $$ = 0; }
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
