build: allow building with address sanitizer

[reinerh]

This allows building firejail (and other binaries) with address sanitizer (with --enable-asan).
I noticed that some were interested in this in #4592 (ping @rusty-snake @smitsohu).

I didn't rebuild configure, as I have a different autoconf version installed and it would only blow up the diff. If someone wants to test it, please run autoreconf -vfi before. (In my humble opinion generated files like configure should anyway not be part of the repository.)

Right now running some of the tools only finds memory leaks. Running firejail itself does not work for me, it fails with this error:

Error: proc 17925 cannot sync with peer: unexpected EOF

Running with --noprofile works, but I didn't figure out yet which setting is the one that breaks it.
Also so far I only tested it with gcc. clang might give different results.

[kmk3]

@reinerh commented on Oct 6:

I didn't rebuild configure, as I have a different autoconf version
installed and it would only blow up the diff. If someone wants to test it,
please run autoreconf -vfi before. (In my humble opinion generated files
like configure should anyway not be part of the repository.)

(Continued in #4595)

[rusty-snake]

Right now running some of the tools only finds memory leaks.

IIRC ASAN_OPTIONS=detect_leaks=0 firejail --…

[rusty-snake]

I didn't rebuild configure, as I have a different autoconf version installed and it would only blow up the diff. If someone wants to test it, please run autoreconf -vfi before.

diff --git a/configure b/configure
index 33a4ca9f..a7628cf1 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ enable_option_checking
 enable_analyzer
 enable_apparmor
 enable_selinux
+enable_asan
 enable_dbusproxy
 enable_output
 enable_usertmpfs
@@ -1370,6 +1371,7 @@ Optional Features:
   --enable-analyzer       enable GCC static analyzer
   --enable-apparmor       enable apparmor
   --enable-selinux        SELinux labeling support
+  --enable-asan           enable address sanitizer
   --disable-dbusproxy     disable dbus proxy
   --disable-output        disable --output logging
   --disable-usertmpfs     disable tmpfs as regular user
@@ -3532,6 +3534,19 @@ if test "x$enable_selinux" = "xyes"; then :
 
 fi
 
+# Check whether --enable-asan was given.
+if test "${enable_asan+set}" = set; then :
+  enableval=$enable_asan;
+fi
+
+if test "x$enable_asan" = "xyes"; then :
+
+       EXTRA_CFLAGS="$EXTRA_CFLAGS -fsanitize=address"
+       EXTRA_LDFLAGS="$EXTRA_LDFLAGS -fsanitize=address"
+
+fi
+
+
 
 
 

[rusty-snake]

Hmm if I ./configure --enable-asan && make it spams stderr full with AddressSanitizer:DEADLYSIGNAL (more than 1000 lines per second) after src/fseccomp/fseccomp default seccomp. Anyone else getting this?

[reinerh]

Hmm if I ./configure --enable-asan && make it spams stderr full with AddressSanitizer:DEADLYSIGNAL (more than 1000 lines per second) after src/fseccomp/fseccomp default seccomp. Anyone else getting this?

No, I don't get that error. It runs without output, procudes a seccomp file and exits with 0.

[reinerh]

I'm wondering whether the flag should be changed to be more flexible to support different sanitizers.
E.g. --with-sanitizer= and allow selection of either address, memory, or undefined.

[kmk3]

@reinerh commented on Oct 7:

I'm wondering whether the flag should be changed to be more flexible to
support different sanitizers. E.g. --with-sanitizer= and allow selection of
either address, memory

Which flag(s) would memory add?

or undefined.

What does it do by itself? From the docs, to me it looks like each suboption
has to be explicitly enabled:

https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html:

-fsanitize=undefined

    Enable UndefinedBehaviorSanitizer, a fast undefined behavior detector.
    Various computations are instrumented to detect undefined behavior at
    runtime.  Current suboptions are:

    -fsanitize=shift

	This option enables checking that the result of a shift operation is
	not undefined.  Note that what exactly is considered undefined differs
	slightly between C and C++, as well as between ISO C90 and C99, etc.
	This option has two suboptions, -fsanitize=shift-base and
	-fsanitize=shift-exponent.

    -fsanitize=shift-exponent

	This option enables checking that the second argument of a shift
	operation is not negative and is smaller than the precision of the
	promoted first argument.

    -fsanitize=shift-base

	If the second argument of a shift operation is within range, check that
	the result of a shift operation is not undefined.  Note that what
	exactly is considered undefined differs slightly between C and C++, as
	well as between ISO C90 and C99, etc.

    -fsanitize=integer-divide-by-zero

        Detect integer division by zero.

    [...]

Or does it enable all the suboptions at once?

[rusty-snake]

Which flag(s) would memory add?

MSAN

or undefined.

What does it do by itself? From the docs, to me it looks like each suboption has to be explicitly enabled:

Or does it enable all the suboptions at once?

clang:

-fsanitize=undefined: All of the checks listed above other than float-divide-by-zero, unsigned-integer-overflow, implicit-conversion, local-bounds and the nullability-* group of checks.

For gcc I'm not sure.

[reinerh]

I've now changed it to allow selection of the sanitizer (--enable-sanitizer=[address,memory,undefined]).
I'm also not completely sure what suboptions the undefined sanitizer is enabling, but the default should be fine.

[matu3ba]

@reinerh neomutt uses a simpler setup with export variables: https://neomutt.org/dev/analysis/asan

ASAN requires make distclean anyway, so providing instructions in the wiki looks sufficient to me. neovim does it the same way.

Having proper scripts looks cleaner though, mhm. How much do you expect things to change?

[rusty-snake]

ASAN_OPTIONS is the envvar used by ASAN for runtime configuration. The setup is simpler because it does not support MSAN or UBSAN.

[kmk3]

AC_ARG_ENABLE([sanitizer],
    AS_HELP_STRING([--enable-sanitizer=@<:@address,memory,undefined@:>@], [enable address/memory/undefined sanitizer (debug)]))
AS_IF([test "x$enable_sanitizer" = "xaddress" -o "x$enable_sanitizer" = "xmemory" -o "x$enable_sanitizer" = "xundefined"],
	[AX_CHECK_COMPILE_FLAG([-fsanitize=$enable_sanitizer],[
		EXTRA_CFLAGS="$EXTRA_CFLAGS -fsanitize=$enable_sanitizer -fno-omit-frame-pointer"
		EXTRA_LDFLAGS="$EXTRA_LDFLAGS -fsanitize=$enable_sanitizer"
],[AC_MSG_ERROR([$enable_sanitizer sanitizer not supported])]
)])

I personally avoid using -a and -o test options, as they are less
portable than && and || and are also potentially more ambiguous. From
test(1p) of POSIX.1-2017[1]:

APPLICATION USAGE

The XSI extensions specifying the -a and -o binary primaries and the '(' and
')' operators have been marked obsolescent. (Many expressions using them are
ambiguously defined by the grammar depending on the specific expressions
being evaluated.) Scripts using these expressions should be converted to the
forms given below. Even though many implementations will continue to support
these obsolescent forms, scripts should be extremely careful when dealing
with user-supplied input that could be confused with these and other
primaries and operators. Unless the application developer knows all the
cases that produce input to the script, invocations like:

test "$1" -a "$2"

should be written as:

test "$1" && test "$2"

to avoid problems if a user supplied values such as $1 set to '!' and $2 set
to the null string. in the shell. [...]

I think that the following would be the more "autoconf way" to do it:

AS_IF([AS_CASE(["$enable_sanitizer"], [address], [], [memory], [], [undefined],
    [], [false])], [
	# ...
])

Though I don't think it's needed in this case since AX_CHECK_COMPILE_FLAG is
already being used anyway.

---

By the way, from my testing in the current version, AX_CHECK_COMPILE_FLAG is
not failing when it should:

$ autoconf && ./configure --enable-sanitizer=foobar >/dev/null
$

Note: It works on the "suggested changes" version.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

[kmk3]

@reinerh Could you squash (or fixup) all commits in this PR (and the suggestion
if you agree with it) and force-push?

If not, then can I do it?

Before merging, I would like to make sure that all amend commits (i.e.: all
commits in the case of this PR) have been squashed. So far, there is only 1
logical change (add the new option), but there are 5 commits involved.

Commits which amend commits that aren't on master are unnecessary and make
e.g.: understanding the history of a file harder than it needs to be,
especially when using git blame recursively.

Note: The issue is not with adding amend commits to an existing PR per se, but
with merging them as is.

Note2: This concern is not specific to this PR (I've seen amend commits merged
in other PRs), but I remember autoconf being hard to grasp and reading the code
and commits in this repo helped (so I'd like to help avoid unnecessary commits
in e.g.: configure.ac's history when it can be helped).

Note3: This should probably be a discussion, but I'm putting it here first
because it's relevant to the PR.

[reinerh]

@kmk3 done, thanks for your sugggestions, I have applied them.

[reinerh]

@reinerh neomutt uses a simpler setup with export variables: https://neomutt.org/dev/analysis/asan

Our configure script supports that as well:

$ EXTRA_CFLAGS="-fsanitize=address" ./configure
[...]
Configuration options:
   EXTRA_CFLAGS: -fsanitize=address [...]

But having a command line flag that is also shown in --help should be a bit more convenient. :-)

ASAN requires make distclean anyway, so providing instructions in the wiki looks sufficient to me. neovim does it the same way.

I think that problem is more generic and does not only apply to sanitizer options. Also when other compiler flags change, everything should be rebuilt ideally. I need to investigate how that could be improved.

Having proper scripts looks cleaner though, mhm. How much do you expect things to change?

What do you mean by what I expect to change? For these specific sanitization flags I don't expect much more changes.
I'm not sure if a script is really needed. I would expect that users who want to run with sanitization know that they need to recompile everything.
But mentioning it in the wiki somewhere probably does not hurt. :-)