[apparmor] [PATCH] parser: Add make variable to build against local or system libapparmor

Sat Dec 21 07:06:26 UTC 2013

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.

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.

> This patch also fixes issues around the inclusion of the apparmor.h
> header. Previously, the in-tree apparmor.h was always being included
> even if the parser was being linked against the system libapparmor.
> Parser source files should no longer include apparmor.h as the Makefile
> includes the correct header at build time using the pre-processor's
> -include option.
> 
> This is fragile and definitely not ideal, but there is a valid reason
> for doing it this way. -I../libraries/libapparmor/src/ was previously
> used, but that is incorrect because there are header file collisions in
> libraries/libapparmor/src/ and parser/. For example, a parser.h exists
> in both directories.

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.

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.

> Per-target modification of EXTRA_CXXFLAGS is performed for source files
> needing to include apparmor.h. Those targets were also updated to depend
> on the local apparmor.h when building against the in-tree libapparmor.
> When building against the system libapparmor, the variable used in the
> dependency list is empty. Likewise, a libapparmor.la dependency is added

I think you mean libapparmor.a dependency here (which is what is
appropriate).

> to the apparmor_parser target when building against the in-tree
> apparmor.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> 
> This turned out being much uglier than I initially expected. This patch should
> be fine to merge, but I'd love for someone to suggest something cleaner. I
> particularly don't like including apparmor.h through a pre-processor option.

The other approach that addresses the issue you pointed out would be
to separate out the apparmor.h header out, as I mentioned above. And
while I admit it's a bit ugly and has potential pitfalls around
_GNU_SOURCE, it's not horrible.

Anyway, Acked-by: Steve Beattie <steve at nxnw.org>. We can follow up with
subsequent changes.

I ended up, after trying different approaches, taking the same approach
with compiling the apparmor regression tests; patch follows.

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.

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>
---
 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 \
+	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>

-- 
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/20131220/243e8605/attachment.pgp>
