[apparmor] [PATCH v2] parser_common.c and unit test cleanups

Kees Cook kees at ubuntu.com
Wed May 11 10:52:07 UTC 2011


[v2: added clean-ups, backed off on some of the build silencing]

This is a rather large rearrangement of how a subset of the parser global
variables are defined. Right now, there are unit tests built without
linking against parser_main.c. As a result, none of the globals defined in
parser_main.c could be used in the code that is built for unit tests
(misc, regex, symtab, variable). To get a clean build, either stubs needed
to be added to "#ifdef UNIT_TEST" blocks in each .c file, or we had to
depend on link-time optimizations that would throw out the unused routines.

First, this is a problem because all the compile-time warnings had to be
explicitly silenced, so reviewing the build logs becomes difficult on
failures, and we can potentially (in really unlucky situations) test
something that isn't actually part of the "real" parser.

Second, not all compilers will allow this kind of linking (e.g. mips gcc),
and the missing symbols at link time will fail the entire build even though
they're technically not needed.

To solve all of this, I've moved all of the global variables used in lex,
yacc, and main to parser_common.c, and adjusted the .h files. On top of
this, I made sure to fully link the tst builds so all symbols are resolved
(including aare lib) and removedonly  tst build-log silencing (for now,
deferring to another future patchset to consolidate the build silencing).

=== modified file 'parser/Makefile'
--- parser/Makefile	2011-03-17 17:50:53 +0000
+++ parser/Makefile	2011-05-11 10:51:13 +0000
@@ -73,29 +73,31 @@
 # Compile-time configuration of the location of the config file
 EXTRA_CFLAGS+=-DSUBDOMAIN_CONFDIR=\"${CONFDIR}\"
 
-SRCS = parser_include.c parser_interface.c parser_lex.c parser_main.c \
-       parser_misc.c parser_merge.c parser_symtab.c parser_yacc.c \
-       parser_regex.c parser_variable.c parser_policy.c parser_alias.c
+SRCS = parser_common.c parser_include.c parser_interface.c parser_lex.c \
+       parser_main.c parser_misc.c parser_merge.c parser_symtab.c \
+       parser_yacc.c parser_regex.c parser_variable.c parser_policy.c \
+       parser_alias.c
 HDRS = parser.h parser_include.h immunix.h
 TOOLS = apparmor_parser
 
-OBJECTS = parser_lex.o parser_yacc.o parser_main.o parser_interface.o \
-	  parser_include.o parser_merge.o parser_symtab.o parser_misc.o \
-	  parser_regex.o parser_variable.o parser_policy.o parser_alias.o
+OBJECTS = $(SRCS:.c=.o)
 
 AAREDIR= libapparmor_re
-AAREOBJECTS = ${AAREDIR}/libapparmor_re.a
+AAREOBJECT = ${AAREDIR}/libapparmor_re.a
+AAREOBJECTS = $(AAREOBJECT) libstdc++.a
+AARE_LDFLAGS=-static-libgcc -L.
 
 LEX_C_FILES	= parser_lex.c
 YACC_C_FILES	= parser_yacc.c parser_yacc.h
 
 TESTS = tst_regex tst_misc tst_symtab tst_variable
-TEST_FLAGS = -Wl,--warn-unresolved-symbols
-DISABLED_TESTS =
-
-TEST_OBJECTS = $(filter-out parser_lex.o, \
-	       $(filter-out parser_yacc.o, \
-	       $(filter-out parser_main.o, ${OBJECTS})))
+TEST_CFLAGS = $(EXTRA_CFLAGS) -DUNIT_TEST -Wno-unused-result
+TEST_OBJECTS = $(filter-out \
+			parser_lex.o \
+			parser_yacc.o \
+			parser_main.o, ${OBJECTS}) \
+               $(AAREOBJECTS)
+TEST_LDFLAGS = $(AARE_LDFLAGS)
 
 ifdef V
   VERBOSE = 1
@@ -141,11 +143,14 @@
 
 all:	main docs tests
 
-apparmor_parser: $(OBJECTS) $(AAREOBJECTS)
+.PHONY: libstdc++.a
+libstdc++.a:
 	rm -f ./libstdc++.a
 	ln -s `g++ -print-file-name=libstdc++.a`
+
+apparmor_parser: $(OBJECTS) $(AAREOBJECTS)
 	g++ $(EXTRA_CFLAGS) -o $@ $(OBJECTS) $(LIBS) \
-	      ${LEXLIB}  $(AAREOBJECTS) -static-libgcc -L.
+	      ${LEXLIB}  $(AAREOBJECTS) $(AARE_LDFLAGS)
 
 parser_yacc.c parser_yacc.h: parser_yacc.y parser.h
 	$(YACC) $(YFLAGS) -o parser_yacc.c parser_yacc.y
@@ -209,17 +214,8 @@
 cap_names.h: /usr/include/linux/capability.h
 	LC_ALL=C sed -n -e "/CAP_EMPTY_SET/d" -e "s/^\#define[ \\t]\\+CAP_\\([A-Z0-9_]\\+\\)[ \\t]\\+\\([0-9xa-f]\\+\\)\\(.*\\)\$$/\{\"\\L\\1\", \\UCAP_\\1\},/p" $< > $@
 
-tst_symtab: parser_symtab.c parser.h parser_variable.o
-	$(Q)$(CC) -DUNIT_TEST $(EXTRA_CFLAGS) $(TEST_FLAGS) -o $@ $< parser_variable.o $(BUILD_OUTPUT)
-
-tst_variable: parser_variable.c parser.h $(filter-out parser_variable.o, ${TEST_OBJECTS})
-	$(Q)$(CC) -DUNIT_TEST $(EXTRA_CFLAGS) $(TEST_FLAGS) -o $@ $< $(filter-out parser_variable.o, ${TEST_OBJECTS}) $(BUILD_OUTPUT)
-
-tst_misc: parser_misc.c parser.h parser_yacc.h af_names.h cap_names.h
-	$(Q)$(CC) -DUNIT_TEST $(EXTRA_CFLAGS) $(TEST_FLAGS) -o $@ $< $(BUILD_OUTPUT)
-
-tst_regex: parser_regex.c parser.h parser_yacc.h
-	$(Q)$(CC) -DUNIT_TEST $(EXTRA_CFLAGS) $(TEST_FLAGS) -o $@ $< $(BUILD_OUTPUT)
+tst_%: parser_%.c parser.h $(filter-out parser_%.o, ${TEST_OBJECTS})
+	$(CC) $(TEST_CFLAGS) -o $@ $< $(filter-out $(<:.c=.o), ${TEST_OBJECTS}) $(TEST_LDFLAGS)
 
 .SILENT: check
 .PHONY: check
@@ -227,16 +223,16 @@
 
 .SILENT: tests
 tests: ${TESTS}
-	sh -e -c 'for test in ${TESTS} ; do echo "*** running $${test}" && ./$${test} $(BUILD_OUTPUT) ; done'
+	sh -e -c 'for test in ${TESTS} ; do echo "*** running $${test}" && ./$${test}; done'
 	$(Q)make -s -C tst tests
 
 .SILENT: check
 check: tests
 
 # always need to rebuild.
-.SILENT: $(AAREOBJECTS)
-.PHONY: $(AAREOBJECTS)
-$(AAREOBJECTS):
+.SILENT: $(AAREOBJECT)
+.PHONY: $(AAREOBJECT)
+$(AAREOBJECT):
 	make -C $(AAREDIR) CFLAGS="$(EXTRA_CXXFLAGS)"
 
 .PHONY: install-rhel4
@@ -297,7 +293,6 @@
 	rm -f $(YACC_C_FILES)
 	rm -f parser_version.h
 	rm -f $(NAME)*.tar.gz $(NAME)*.tgz
-	rm -f libstdc++.a
 	rm -f af_names.h
 	rm -f cap_names.h
 	rm -rf techdoc.aux techdoc.log techdoc.pdf techdoc.toc techdor.txt techdoc/

=== modified file 'parser/parser.h'
--- parser/parser.h	2011-03-13 12:49:15 +0000
+++ parser/parser.h	2011-05-02 22:42:00 +0000
@@ -179,13 +179,8 @@
 
 #define FLAG_CHANGEHAT_1_4  2
 #define FLAG_CHANGEHAT_1_5  3
-extern int kernel_supports_network;
-extern int net_af_max_override;
-extern int flag_changehat_version;
-extern int read_implies_exec;
-extern dfaflags_t dfaflags;
+
 extern int preprocess_only;
-extern FILE *ofile;
 
 #define PATH_CHROOT_REL 0x1
 #define PATH_NS_REL 0x2
@@ -228,23 +223,34 @@
 #define list_last_entry(LIST, ENTRY) \
 	for ((ENTRY) = (LIST); (ENTRY) && (ENTRY)->next; (ENTRY) = (ENTRY)->next)
 
-/* Some external definitions to make b0rken programs happy */
+/* from parser_common.c */
+extern int regex_type;
+extern int perms_create;
+extern int net_af_max_override;
+extern int kernel_load;
+extern int kernel_supports_network;
+extern int flag_changehat_version;
+extern int conf_verbose;
+extern int conf_quiet;
+extern int names_only;
+extern int option;
+extern int current_lineno;
+extern dfaflags_t dfaflags;
 extern char *progname;
 extern char *subdomainbase;
 extern char *profilename;
 extern char *profile_namespace;
+extern char *current_filename;
+extern FILE *ofile;
+extern int read_implies_exec;
+extern void pwarn(char *fmt, ...) __attribute__((__format__(__printf__, 1, 2)));
 
-/* from parser_main */
+/* from parser_main (cannot be used in tst builds) */
 extern int force_complain;
-extern int conf_quiet;
-extern int conf_verbose;
-extern int kernel_load;
-extern int regex_type;
-extern int perms_create;
 extern struct timespec mru_tstamp;
 extern void update_mru_tstamp(FILE *file);
-extern void pwarn(char *fmt, ...) __attribute__((__format__(__printf__, 1, 2)));
 
+/* provided by parser_lex.l (cannot be used in tst builds) */
 extern FILE *yyin;
 extern void yyrestart(FILE *fp);
 extern int yyparse(void);
@@ -340,3 +346,32 @@
 extern void dump_policy_names(void);
 extern int die_if_any_regex(void);
 void free_policies(void);
+
+#ifdef UNIT_TEST
+/* For the unit-test builds, we must include function stubs for stuff that
+ * only exists in the excluded object files; everything else should live
+ * in parser_common.c.
+ */
+
+/* parser_yacc.y */
+void yyerror(char *msg, ...)
+{
+        va_list arg;
+        char buf[PATH_MAX];
+
+        va_start(arg, msg);
+        vsnprintf(buf, sizeof(buf), msg, arg);
+        va_end(arg);
+
+        PERROR(_("AppArmor parser error: %s\n"), buf);
+
+        exit(1);
+}
+
+#define MY_TEST(statement, error)               \
+        if (!(statement)) {                     \
+                PERROR("FAIL: %s\n", error);    \
+                rc = 1;                         \
+        }
+
+#endif

=== added file 'parser/parser_common.c'
--- parser/parser_common.c	1970-01-01 00:00:00 +0000
+++ parser/parser_common.c	2011-05-02 22:14:53 +0000
@@ -0,0 +1,76 @@
+/*
+ *   Copyright (c) 2010, 2011
+ *   Canonical, Ltd. (All rights reserved)
+ *
+ *   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.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, contact Novell, Inc. or Canonical,
+ *   Ltd.
+ */
+#include <stdlib.h>
+#include <stdarg.h>
+#include <libintl.h>
+#include <locale.h>
+#define _(s) gettext(s)
+#include "parser.h"
+
+int regex_type = AARE_DFA;
+int perms_create = 0;                   /* perms contain create flag */
+int net_af_max_override = -1;           /* use kernel to determine af_max */
+int kernel_load = 1;
+int kernel_supports_network = 1;        /* kernel supports network rules */
+int flag_changehat_version = FLAG_CHANGEHAT_1_5;
+int conf_verbose = 0;
+int conf_quiet = 0;
+int names_only = 0;
+int current_lineno = 1;
+int option = OPTION_ADD;
+
+dfaflags_t dfaflags = DFA_CONTROL_TREE_NORMAL | DFA_CONTROL_TREE_SIMPLE | DFA_CONTROL_MINIMIZE | DFA_CONTROL_MINIMIZE_HASH_TRANS | DFA_CONTROL_MINIMIZE_HASH_PERMS;
+
+char *subdomainbase = NULL;
+char *progname = __FILE__;
+char *profile_namespace = NULL;
+char *profilename = NULL;
+char *current_filename = NULL;
+
+FILE *ofile = NULL;
+
+#ifdef FORCE_READ_IMPLIES_EXEC
+int read_implies_exec = 1;
+#else
+int read_implies_exec = 0;
+#endif
+
+void pwarn(char *fmt, ...)
+{
+        va_list arg;
+        char *newfmt;
+        int rc;
+
+        if (conf_quiet || names_only || option == OPTION_REMOVE)
+                return;
+
+        rc = asprintf(&newfmt, _("Warning from %s (%s%sline %d): %s"),
+                      profilename ? profilename : "stdin",
+                      current_filename ? current_filename : "",
+                      current_filename ? " " : "",
+                      current_lineno,
+                      fmt);
+        if (!newfmt)
+                return;
+
+        va_start(arg, fmt);
+        vfprintf(stderr, newfmt, arg);
+        va_end(arg);
+
+        free(newfmt);
+}

=== modified file 'parser/parser_include.h'
--- parser/parser_include.h	2010-06-05 01:57:01 +0000
+++ parser/parser_include.h	2011-05-02 22:39:47 +0000
@@ -21,8 +21,6 @@
 #define PARSER_INCLUDE_H
 
 extern int preprocess_only;
-extern int current_lineno;
-extern char *current_filename;
 
 extern int add_search_dir(char *dir);
 extern void init_base_dir(void);

=== modified file 'parser/parser_lex.l'
--- parser/parser_lex.l	2010-11-19 10:27:33 +0000
+++ parser/parser_lex.l	2011-05-02 21:22:36 +0000
@@ -54,9 +54,6 @@
 
 #define YY_NO_INPUT
 
-int current_lineno     = 1;
-char *current_filename = NULL;
-
 struct ignored_suffix_t {
 	char * text;
 	int len;

=== modified file 'parser/parser_main.c'
--- parser/parser_main.c	2011-05-02 20:34:58 +0000
+++ parser/parser_main.c	2011-05-02 22:14:52 +0000
@@ -2,7 +2,7 @@
  *   Copyright (c) 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
  *   NOVELL (All rights reserved)
  *
- *   Copyright (c) 2010
+ *   Copyright (c) 2010, 2011
  *   Canonical, Ltd. (All rights reserved)
  *
  *   This program is free software; you can redistribute it and/or
@@ -58,46 +58,26 @@
 #define UNPRIVILEGED_OPS (!(PRIVILEGED_OPS))
 
 const char *parser_title	= "AppArmor parser";
-const char *parser_copyright	= "Copyright (C) 1999-2008 Novell Inc.\nCopyright 2009-2010 Canonical Ltd.";
+const char *parser_copyright	= "Copyright (C) 1999-2008 Novell Inc.\nCopyright 2009-2011 Canonical Ltd.";
 
 char *progname;
-int option = OPTION_ADD;
 int opt_force_complain = 0;
 int binary_input = 0;
-int names_only = 0;
 int dump_vars = 0;
 int dump_expanded_vars = 0;
-dfaflags_t dfaflags = DFA_CONTROL_TREE_NORMAL | DFA_CONTROL_TREE_SIMPLE | DFA_CONTROL_MINIMIZE | DFA_CONTROL_MINIMIZE_HASH_TRANS | DFA_CONTROL_MINIMIZE_HASH_PERMS;
-int conf_verbose = 0;
-int conf_quiet = 0;
-int kernel_load = 1;
 int show_cache = 0;
 int skip_cache = 0;
 int skip_read_cache = 0;
 int write_cache = 0;
-#ifdef FORCE_READ_IMPLIES_EXEC
-int read_implies_exec = 1;
-#else
-int read_implies_exec = 0;
-#endif
 int preprocess_only = 0;
 int skip_mode_force = 0;
 struct timespec mru_tstamp;
 
-char *subdomainbase = NULL;
 char *match_string = NULL;
 char *flags_string = NULL;
-int regex_type = AARE_DFA;
-int perms_create = 0;		/* perms contain create flag */
-int kernel_supports_network = 1;	/* kernel supports network rules */
-int net_af_max_override = -1;		/* use kernel to determine af_max */
-char *profile_namespace = NULL;
-int flag_changehat_version = FLAG_CHANGEHAT_1_5;
-FILE *ofile = NULL;
 
 /* per-profile settings */
 int force_complain = 0;
-char *profilename = NULL;
 
 struct option long_options[] = {
 	{"add", 		0, 0, 'a'},
@@ -322,31 +302,6 @@
 	print_flag_table(optflag_table);
 }
 
-void pwarn(char *fmt, ...)
-{
-	va_list arg;
-	char *newfmt;
-	int rc;
-
-	if (conf_quiet || names_only || option == OPTION_REMOVE)
-		return;
-
-	rc = asprintf(&newfmt, _("Warning from %s (%s%sline %d): %s"),
-		      profilename ? profilename : "stdin",
-		      current_filename ? current_filename : "",
-		      current_filename ? " " : "",
-		      current_lineno,
-		      fmt);
-	if (!newfmt)
-		return;
-
-	va_start(arg, fmt);
-	vfprintf(stderr, newfmt, arg);
-	va_end(arg);
-
-	free(newfmt);
-}
-
 static int process_args(int argc, char *argv[])
 {
 	int c, o;

=== modified file 'parser/parser_misc.c'
--- parser/parser_misc.c	2011-04-06 03:55:19 +0000
+++ parser/parser_misc.c	2011-05-02 22:23:14 +0000
@@ -929,27 +929,6 @@
 }
 
 #ifdef UNIT_TEST
-#define MY_TEST(statement, error)		\
-	if (!(statement)) {			\
-		PERROR("FAIL: %s\n", error);	\
-		rc = 1;				\
-	}
-
-/* Guh, fake routine */
-void yyerror(char *msg, ...)
-{
-	va_list arg;
-	char buf[PATH_MAX];
-
-	va_start(arg, msg);
-	vsnprintf(buf, sizeof(buf), msg, arg);
-	va_end(arg);
-
-	PERROR(_("AppArmor parser error: %s\n"), buf);
-
-	exit(1);
-}
-
 int test_str_to_boolean(void)
 {
 	int rc = 0;
@@ -973,7 +952,7 @@
 int test_processunquoted(void)
 {
 	int rc = 0;
-	const char *teststring, *processedstring;
+	char *teststring, *processedstring;
 
 	teststring = "";
 	MY_TEST(strcmp(teststring, processunquoted(teststring, strlen(teststring))) == 0,
@@ -1001,7 +980,7 @@
 int test_processquoted(void)
 {
 	int rc = 0;
-	const char *teststring, *processedstring;
+	char *teststring, *processedstring;
 	char *out;
 
 	teststring = "";

=== modified file 'parser/parser_regex.c'
--- parser/parser_regex.c	2011-05-02 20:34:58 +0000
+++ parser/parser_regex.c	2011-05-02 21:18:53 +0000
@@ -617,29 +617,6 @@
 }
 
 #ifdef UNIT_TEST
-#define MY_TEST(statement, error)		\
-	if (!(statement)) {			\
-		PERROR("FAIL: %s\n", error);	\
-		rc = 1;				\
-	}
-
-/* Guh, fake routine */
-void yyerror(char *msg, ...)
-{
-	va_list arg;
-	char buf[PATH_MAX];
-
-	va_start(arg, msg);
-	vsnprintf(buf, sizeof(buf), msg, arg);
-	va_end(arg);
-
-	PERROR(_("AppArmor parser error: %s\n"), buf);
-
-	exit(1);
-}
-/* Guh, fake symbol */
-char *progname;
-
 static int test_filter_slashes(void)
 {
 	int rc = 0;

=== modified file 'parser/parser_symtab.c'
--- parser/parser_symtab.c	2010-12-20 20:29:10 +0000
+++ parser/parser_symtab.c	2011-05-02 22:25:36 +0000
@@ -539,30 +539,6 @@
 }
 
 #ifdef UNIT_TEST
-#define MY_TEST(statement, error)		\
-	if (!(statement)) {			\
-		PERROR("FAIL: %s\n", error);	\
-		rc = 1;				\
-	}
-
-/* Guh, fake symbol */
-char *progname;
-
-/* Guh, fake routine */
-void yyerror(char *msg, ...)
-{
-	va_list arg;
-	char buf[PATH_MAX];
-
-	va_start(arg, msg);
-	vsnprintf(buf, sizeof(buf), msg, arg);
-	va_end(arg);
-
-	PERROR(_("AppArmor parser error: %s\n"), buf);
-
-	exit(1);
-}
-
 int main(void)
 {
 	int rc = 0;
@@ -589,7 +565,7 @@
 	retval = new_set_var("test", "different value");
 	MY_TEST(retval != 0, "new set variable 2");
 
-	retval = new_set_var("testes", "testes");
+	retval = new_set_var("testing", "testing");
 	MY_TEST(retval == 0, "new set variable 3");
 
 	retval = new_set_var("monopuff", "Mockingbird");

=== modified file 'parser/parser_variable.c'
--- parser/parser_variable.c	2011-03-28 17:52:02 +0000
+++ parser/parser_variable.c	2011-05-02 22:18:34 +0000
@@ -216,29 +216,6 @@
 }
 
 #ifdef UNIT_TEST
-#define MY_TEST(statement, error)		\
-	if (!(statement)) {			\
-		PERROR("FAIL: %s\n", error);	\
-		rc = 1;				\
-	}
-
-/* Guh, fake routine */
-void yyerror(char *msg, ...)
-{
-	va_list arg;
-	char buf[PATH_MAX];
-
-	va_start(arg, msg);
-	vsnprintf(buf, sizeof(buf), msg, arg);
-	va_end(arg);
-
-	PERROR(_("AppArmor parser error: %s\n"), buf);
-
-	exit(1);
-}
-/* Guh, fake symbol */
-char *progname;
-
 int test_get_var_end(void)
 {
 	int rc = 0;
@@ -277,7 +254,7 @@
 	char *var = "boogie";
 	char *suffix = "suffixication";
 
-	(void) asprintf(&tst_string, "%s@{%s}%s", prefix, var, suffix);
+	asprintf(&tst_string, "%s@{%s}%s", prefix, var, suffix);
 	var_start = tst_string + strlen(prefix);
 	var_end = var_start + strlen(var) + strlen("@\{");
 	ret_struct = split_string(tst_string, var_start, var_end);


-- 
Kees Cook
Ubuntu Security Team



More information about the AppArmor mailing list