[apparmor] [PATCH] parser: Add make variable to build against local or system libapparmor
Seth Arnold
seth.arnold at canonical.com
Sat Dec 21 07:40:13 UTC 2013
On Fri, Dec 20, 2013 at 11:06:26PM -0800, Steve Beattie wrote:
> On Tue, Dec 10, 2013 at 01:36:10PM -0800, Seth Arnold wrote:
> > Is building against the in-tree version the "best" default?
>
> Yes. If there's been any development of the parser that depends on
> features of libapparmor that are newer than your system's libapparmor,
> then your compilation will fail. As a specific example, trunk's
> apparmor regression tests fail to compile on Ubuntu 12.04 LTS, because
> the query_label test program depends on the query interface added to
> libapparmor, which isn't present in 12.04's version of libapparmor.
>
> This will become more of an issue if we push some of the functionality
> currently in the parser into libapparmor, as is (long-term) planned.
Okay, I'm sold. :)
> On Fri, Dec 06, 2013 at 08:57:57PM -0800, Tyler Hicks wrote:
> > By default, statically link against the in-tree libapparmor. If the
> > in-tree libapparmor is not yet built, print a helpful error message.
> > To build against the system libapparmor, the SYSTEM_LIBAPPARMOR make
> > variable can be set on the command line like so:
> >
> > $ make SYSTEM_LIBAPPARMOR=1
>
> I suspect we also want a helpful message if SYSTEM_LIBAPPARMOR is set
> but the system libraries and/or header cannot be found, indicating
> that a libapparmor devel package needs to be installed.
>
> I am curious if John will complain about the length of the
> SYSTEM_LIBAPPARMOR variable, if he has to remember to define it on
> his make invocations.
hehe, good point. No need to force us all to have shell aliases..
> Ah, that's a very good point. I don't object to this approach for
> the parser. Another solution would be to move the public facing
> headers outside of the src/ tree (include/ ? include/sys/ ?) and
> add that location to the search path when building against in-tree
> libapparmor. That would also serve us should libapparmor grow
> additional external headers.
I like the idea of clearly separating internal from external headers. Off
the cuff it feels like it'd be a fair amount of churn up front but I think
the long-term benefits of keeping the ABI documented seperately from the
internal implementation details is worth the hassle.
> One thing to be careful of is if the specific file needs included bits
> that are only available when _GNU_SOURCE is defined that happens to
> come from a header that also is included from libapparmor's apparmor.h;
> because the -include is treated as if it were the first thing in the
> compiled source file, _GNU_SOURCE would need to be defined on the
> compilation command line.
Nice find.
> I chose a different variable name "USE_SYSTEM" because I'd like to also
> decide to use which parser in the tests based on that flag (the in-tree
> version versus the system parser), with the ability to override with a
> user specified parser as well.
This muddles my head. It feels like the tests ought to be compiled using
whichever libraries and headers were used for the parser that's going to
be tested... so when run from top-level Makefile, SYSTEM_LIBAPPARMOR would
need to be passed through as USE_SYSTEM.
Of course we might want to expose different libraries for the tests than
the parser, just to test that condition.
We may need more variables, perhaps:
For the tools, when building them, "system" or "source":
TOOLS_USE_LIBAPPARMOR
TOOLS_USE_PARSER
For the tests, when building them, "system" or "source":
TESTS_USE_LIBAPPRMOR
TESTS_USE_PARSER
Is this insanity? Or clean separation of concerns? They sound the same.
> The patch touches more files precisely because I hit the _GNU_SOURCE
> issue I mentioned above: certain files needed _GNU_SOURCE defined
> before the processing of headers that were included via apparmor.h,
> which led to warnings about the redefinition of both _GNU_SOURCE and
> _XOPEN_SOURCE (in particular, defining _GNU_SOURCE causes _XOPEN_SOURCE
> to be set to 700, not 500).
>
> Signed-off-by: Steve Beattie <steve at nxnw.org>
In any event, this patch looks good with one small typo fix inline.
Acked-by: Seth Arnold <seth.arnold at canonical.com>
Thanks
> ---
> tests/regression/apparmor/Makefile | 41 +++++++++++++++++++++-----
> tests/regression/apparmor/changehat.h | 1
> tests/regression/apparmor/changehat_wrapper.c | 2 -
> tests/regression/apparmor/changeprofile.c | 1
> tests/regression/apparmor/clone.c | 2 -
> tests/regression/apparmor/dbus_common.c | 3 +
> tests/regression/apparmor/dbus_eavesdrop.c | 3 +
> tests/regression/apparmor/dbus_message.c | 3 +
> tests/regression/apparmor/dbus_service.c | 3 +
> tests/regression/apparmor/env_check.c | 3 +
> tests/regression/apparmor/fchdir.c | 2 -
> tests/regression/apparmor/introspect.c | 2 -
> tests/regression/apparmor/onexec.c | 1
> tests/regression/apparmor/openat.c | 2 -
> tests/regression/apparmor/pwrite.c | 3 +
> tests/regression/apparmor/query_label.c | 1
> tests/regression/apparmor/unix_fd_client.c | 4 +-
> tests/regression/apparmor/unix_fd_server.c | 3 +
> 18 files changed, 54 insertions(+), 26 deletions(-)
>
> Index: b/tests/regression/apparmor/Makefile
> ===================================================================
> --- a/tests/regression/apparmor/Makefile
> +++ b/tests/regression/apparmor/Makefile
> @@ -1,10 +1,44 @@
> # Copyright (C) 2002-2005 Novell/SUSE
> +# Copyright (C) 2013 Canonical, Ltd
> #
> # This program is free software; you can redistribute it and/or
> # modify it under the terms of the GNU General Public License as
> # published by the Free Software Foundation, version 2 of the
> # License.
>
> +ifdef USE_SYSTEM
> + # use the system libapparmor headers and library
> + LIBAPPARMOR = $(shell if pkg-config --exists libapparmor ; then \
> + pkg-config --silence-errors --libs libapparmor ; \
> + elif ldconfig -p | grep -q libapparmor\.so$$ ; then \
> + echo -lapparmor ; \
> + fi )
> + ifeq ($(strip $(LIBAPPARMOR)),)
> + $(error Unable to find libapparmor installed on this echo system; either \
Delete the word 'echo' in the line above.
> + install libapparmor devel packages, set the LIBAPPARMOR variable \
> + manually, or build against in-tree libapparmor)
> + endif # LIBAPPARMOR not set
> + LIBAPPARMOR_H = /usr/include/sys/apparmor.h
> + LDLIBS += $(LIBAPPARMOR)
> +
> +else # !USE_SYSTEM
> + # use in-tree versions
> + LIBAPPARMOR_SRC := ../../../libraries/libapparmor/src/
> + LIBAPPARMOR_H = $(LIBAPPARMOR_SRC)/apparmor.h
> + LIBAPPARMOR_PATH := $(LIBAPPARMOR_SRC)/.libs/
> + ifeq ($(realpath $(LIBAPPARMOR_PATH)/libapparmor.a),)
> + $(error $(LIBAPPARMOR_PATH)/libapparmor.a is missing; either build against \
> + the in-tree libapparmor by building it first and then trying again \
> + (see the top-level README for help\) or build against the system \
> + libapparmor by adding USE_SYSTEM=1 to your make command.)
> + endif
> +
> + CFLAGS += -L$(LIBAPPARMOR_PATH)
> + LDLIBS += -Wl,-Bstatic -lapparmor -Wl,-Bdynamic -lpthread
> +endif # USE_SYSTEM
> +
> +CFLAGS += -D_GNU_SOURCE -Wall -Wstrict-prototypes -include $(LIBAPPARMOR_H)
> +
> SRC=access.c \
> introspect.c \
> changeprofile.c \
> @@ -82,13 +116,6 @@ ifneq (,$(findstring $(shell uname -i),i
> SRC+=syscall_ioperm.c syscall_iopl.c
> endif
>
> -LIBAPPARMOR:=$(shell if ldconfig -p | grep -q libapparmor\.so ; then \
> - echo -lapparmor ; \
> - fi )
> -
> -CFLAGS+=-Wall -Wstrict-prototypes
> -LDLIBS+=$(LIBAPPARMOR)
> -
> EXEC=$(SRC:%.c=%)
>
> TESTS=access \
> Index: b/tests/regression/apparmor/changehat.h
> ===================================================================
> --- a/tests/regression/apparmor/changehat.h
> +++ b/tests/regression/apparmor/changehat.h
> @@ -1,6 +1,5 @@
> #include <fcntl.h>
> #include <string.h>
> -#include <sys/apparmor.h>
>
> #define SD_ID_MAGIC 0x8c235e38
>
> Index: b/tests/regression/apparmor/changeprofile.c
> ===================================================================
> --- a/tests/regression/apparmor/changeprofile.c
> +++ b/tests/regression/apparmor/changeprofile.c
> @@ -17,7 +17,6 @@
> #include <stdlib.h>
> #include <linux/unistd.h>
>
> -#include <sys/apparmor.h>
> #include "changehat.h"
>
> int main(int argc, char *argv[])
> Index: b/tests/regression/apparmor/introspect.c
> ===================================================================
> --- a/tests/regression/apparmor/introspect.c
> +++ b/tests/regression/apparmor/introspect.c
> @@ -18,8 +18,6 @@
> #include <stdlib.h>
> #include <linux/unistd.h>
>
> -#include <sys/apparmor.h>
> -
> int main(int argc, char *argv[])
> {
> int rc;
> Index: b/tests/regression/apparmor/onexec.c
> ===================================================================
> --- a/tests/regression/apparmor/onexec.c
> +++ b/tests/regression/apparmor/onexec.c
> @@ -18,7 +18,6 @@
> #include <signal.h>
> #include <linux/unistd.h>
>
> -#include <sys/apparmor.h>
> #include "changehat.h"
>
> int main(int argc, char *argv[])
> Index: b/tests/regression/apparmor/query_label.c
> ===================================================================
> --- a/tests/regression/apparmor/query_label.c
> +++ b/tests/regression/apparmor/query_label.c
> @@ -1,7 +1,6 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> -#include <sys/apparmor.h>
>
> #define OPT_EXPECT "--expect="
> #define OPT_EXPECT_LEN strlen(OPT_EXPECT)
> Index: b/tests/regression/apparmor/pwrite.c
> ===================================================================
> --- a/tests/regression/apparmor/pwrite.c
> +++ b/tests/regression/apparmor/pwrite.c
> @@ -9,7 +9,8 @@
> * License.
> */
>
> -#define _XOPEN_SOURCE 500
> +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */
> +
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> Index: b/tests/regression/apparmor/changehat_wrapper.c
> ===================================================================
> --- a/tests/regression/apparmor/changehat_wrapper.c
> +++ b/tests/regression/apparmor/changehat_wrapper.c
> @@ -15,7 +15,7 @@
> * inherited profile that is currently in a subprofile will stay that
> * way across fork()/exec(). */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
>
> #include <stdio.h>
> #include <unistd.h>
> Index: b/tests/regression/apparmor/clone.c
> ===================================================================
> --- a/tests/regression/apparmor/clone.c
> +++ b/tests/regression/apparmor/clone.c
> @@ -7,7 +7,7 @@
> * License.
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
>
> #include <stdio.h>
> #include <unistd.h>
> Index: b/tests/regression/apparmor/dbus_common.c
> ===================================================================
> --- a/tests/regression/apparmor/dbus_common.c
> +++ b/tests/regression/apparmor/dbus_common.c
> @@ -22,7 +22,8 @@
> *
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> Index: b/tests/regression/apparmor/dbus_eavesdrop.c
> ===================================================================
> --- a/tests/regression/apparmor/dbus_eavesdrop.c
> +++ b/tests/regression/apparmor/dbus_eavesdrop.c
> @@ -23,7 +23,8 @@
> *
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> Index: b/tests/regression/apparmor/dbus_message.c
> ===================================================================
> --- a/tests/regression/apparmor/dbus_message.c
> +++ b/tests/regression/apparmor/dbus_message.c
> @@ -23,7 +23,8 @@
> *
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> Index: b/tests/regression/apparmor/fchdir.c
> ===================================================================
> --- a/tests/regression/apparmor/fchdir.c
> +++ b/tests/regression/apparmor/fchdir.c
> @@ -7,7 +7,7 @@
> * License.
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
>
> #include <stdio.h>
> #include <unistd.h>
> Index: b/tests/regression/apparmor/openat.c
> ===================================================================
> --- a/tests/regression/apparmor/openat.c
> +++ b/tests/regression/apparmor/openat.c
> @@ -7,7 +7,7 @@
> * License.
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
>
> #include <stdio.h>
> #include <stdlib.h>
> Index: b/tests/regression/apparmor/unix_fd_client.c
> ===================================================================
> --- a/tests/regression/apparmor/unix_fd_client.c
> +++ b/tests/regression/apparmor/unix_fd_client.c
> @@ -1,5 +1,3 @@
> -#define _XOPEN_SOURCE 500
> -
> /*
> * Copyright (C) 2002-2005 Novell/SUSE
> *
> @@ -9,6 +7,8 @@
> * License.
> */
>
> +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <unistd.h>
> #include <sys/types.h>
> Index: b/tests/regression/apparmor/unix_fd_server.c
> ===================================================================
> --- a/tests/regression/apparmor/unix_fd_server.c
> +++ b/tests/regression/apparmor/unix_fd_server.c
> @@ -12,7 +12,8 @@
>
> /* this is very ugly */
>
> -#define _XOPEN_SOURCE 500
> +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <unistd.h>
> #include <sys/types.h>
> Index: b/tests/regression/apparmor/dbus_service.c
> ===================================================================
> --- a/tests/regression/apparmor/dbus_service.c
> +++ b/tests/regression/apparmor/dbus_service.c
> @@ -23,7 +23,8 @@
> *
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> Index: b/tests/regression/apparmor/env_check.c
> ===================================================================
> --- a/tests/regression/apparmor/env_check.c
> +++ b/tests/regression/apparmor/env_check.c
> @@ -6,7 +6,8 @@
> * License.
> */
>
> -#define _GNU_SOURCE
> +/* needs _GNU_SOURCE defined */
> +
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20131220/71d9e083/attachment-0001.pgp>
More information about the AppArmor
mailing list