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

Steve Beattie steve at nxnw.org
Sun May 8 02:28:10 UTC 2011


On Mon, May 02, 2011 at 03:53:17PM -0700, Kees Cook wrote:
> 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
> (include aare lib)

This part sounds good.

> and remove all the build-log silencing (since reviewing
> a failed build log wasn't useful without VERBOSE=1).

I'd like to keep build silencing as much as possible so that in a
standard build only important things are shown, like compiler warnings
and test failures, and then keep the VERBOSE or V = 1 option available
for diagnosing specific bits of the build process. We've had issues in
the past where because the build output was deemed "too noisy" by some
developers, running through the tests would be skipped during the
development process.

The unit tests should probably gain their own make target to ease
running them separate from the other tests, given the length of the
language parsing tests.

(I admit I'm a bit schizophrenic on this point, as e.g. I'm the one that
added the option to flex to dump scanner statistics to stdout. I should
probably submit a patch to only add that when VERBOSE is set.)

> The solution is a little bit ugly, but I feel it is an improvement over
> what was there.
> 
> -Kees
> 
> 
> -- 
> Kees Cook
> Ubuntu Security Team

> === modified file 'parser/Makefile'
> --- parser/Makefile	2011-03-17 17:50:53 +0000
> +++ parser/Makefile	2011-05-02 22:37:35 +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 \
> +SRCS = parser_common.c \
> +       parser_include.c parser_interface.c parser_lex.c parser_main.c \

I know you're trying to minimize the diff to ease review, but go ahead
and re-adjust these lines.

>         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
> @@ -105,12 +107,10 @@
>  endif
>  ifeq ($(VERBOSE),1)
>    BUILD_OUTPUT =
> -  Q =
>  else
>    BUILD_OUTPUT = > /dev/null 2>&1
> -  Q = @
>  endif
> -export Q VERBOSE BUILD_OUTPUT
> +export VERBOSE BUILD_OUTPUT
>  
>  po/${NAME}.pot: ${SRCS} ${HDRS}
>  	make -C po ${NAME}.pot NAME=${NAME} SOURCES="${SRCS} ${HDRS}"
> @@ -129,7 +129,7 @@
>  # targets arranged this way so that people who don't want full docs can
>  # pick specific targets they want.
>  main: 	$(TOOLS)
> -	$(Q)make -C po all
> +	make -C po all
>  
>  manpages:	$(MANPAGES)
>  
> @@ -141,11 +141,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 +212,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 +221,16 @@
>  
>  .SILENT: tests
>  tests: ${TESTS}
> -	sh -e -c 'for test in ${TESTS} ; do echo "*** running $${test}" && ./$${test} $(BUILD_OUTPUT) ; done'
> -	$(Q)make -s -C tst tests
> +	sh -e -c 'for test in ${TESTS} ; do echo "*** running $${test}" && ./$${test}; done'
> +	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 +291,6 @@
>  	rm -f $(YACC_C_FILES)
>  	rm -f parser_version.h
>  	rm -f $(NAME)*.tar.gz $(NAME)*.tgz
> -	rm -f libstdc++.a

Is there a reason you dropped this? At compilation, we create the
libstdc++.a symlink so that we can statically link the parser against
libstdc++ to keep the parser's dynamic links against things that should
be in /lib/, to make the parser usable for early booting situations
(libstdc++'s dynamic library lives in /usr/lib, making it unsuitable
for situations where /usr is nfs mounted).

>  	rm -f af_names.h
>  	rm -f cap_names.h
>  	rm -rf techdoc.aux techdoc.log techdoc.pdf techdoc.toc techdor.txt techdoc/

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20110507/cb6a6f83/attachment.pgp>


More information about the AppArmor mailing list