GNOME Bugzilla – Bug 761429
Add support for building NM with GCC and Clang sanitizers
Last modified: 2016-06-03 20:49:23 UTC
Recent GCC and Clang versions can compile applications with runtime checks for some programming errors as bad memory accesses, undefined behaviors, memory leaks, etc. Maybe it would be useful to provide configure options to enable such features (--enable-address-sanitizer, --enable-undefined-sanitizer, ?). The address sanitizer could replace the usage of valgrind when running the test suite since it's faster and can detect a broader range of errors. https://github.com/google/sanitizers http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Pushed bg/asan-bgo761429, please review.
pushed one fixup. There seems to be memory leaks in ifupdown tests.
(In reply to Thomas Haller from comment #2) > pushed one fixup. Looks good. > There seems to be memory leaks in ifupdown tests. I've added some commits to fix the leaks in 'make check'.
Could you reorder the commits, to first fix the tests and then enable the sanitizer? The libnm-util/tests/test-libnm-linking test never terminates and breaks tests.
(In reply to Thomas Haller from comment #4) > The libnm-util/tests/test-libnm-linking test never terminates and breaks > tests. It's because g_error is no longer working as expected to abort the program... that is also bad for other tests, because we use that commonly to denote a fatal failure (in tests). Unless easily fixable, we could re-define g_error in nm-glib.h.
(In reply to Thomas Haller from comment #5) > (In reply to Thomas Haller from comment #4) > > > The libnm-util/tests/test-libnm-linking test never terminates and breaks > > tests. > > It's because g_error is no longer working as expected to abort the > program... that is also bad for other tests, because we use that commonly to > denote a fatal failure (in tests). > > Unless easily fixable, we could re-define g_error in nm-glib.h. Mmh, it's working here. Can you check if you have something trying to write a core file for the failed program (like systemd-coredump or abrt)? Since the program is built with address sanitizer, its address space should be very big and the dump can take a long time.
I had abrt running, so the coredump didn't take so long it never finished. After uninstalling abrt, systemd-coredump takes over. That one is faster, but still taking 12 seconds. Can we do something to avoid writing these core dumps?
(In reply to Thomas Haller from comment #7) > Can we do something to avoid writing these core dumps? Apart from setting RLIMIT_CORE to 0 (which we already do, see _test_libnm_linking_setup_child_process in test-general.c), I suspect there is nothing that can be done. systemd (but only from v229) honors such setting and won't generate a core dump when the limit is set to 0. abrt instead uses RLIMIT_CORE only to decide whether a core dump should be generated in the working directory (but will process the dump anyway). So the only solution for now is to disable core dumps globally.
(In reply to Beniamino Galvani from comment #8) > (In reply to Thomas Haller from comment #7) > So the only solution for now is to disable core dumps globally. That is a bit unsatisfying, but well. Could you somewhere add a comment about this? Maybe in configure.ac around the --enable-*-sanitizer configure flag? Another question I had, is why did you add two separate flags and not just one --enable-sanitizer? Anyway, branch looks good to me as is.
(In reply to Thomas Haller from comment #9) > Could you somewhere add a comment about this? Maybe in configure.ac around > the --enable-*-sanitizer configure flag? Done. > Another question I had, is why did you add two separate flags and not just > one --enable-sanitizer? The two were added in different GCC versions and thus it's possible that someone wants to use only the address sanitizer but not the undefined one. > Anyway, branch looks good to me as is. I admit that it's a bit hacky but the cause is that we compile libnm with -fsanitize and this has the disadvantage that every application linking against it must be either compiled with -fsanitize or preload the asan shared object through LD_PRELOAD; otherwise it will refuse to load. So, an alternative is to remove the -fsanitize flags when compiling libnm/ (and thus also libnm-core/ and introspection/) and enable it only in the core (and maybe clients). This will simpler, as clients don't need to preload the library (and Makefiles don't need all the hacks), but the libnm library code will not be checked. What do you think?
Dropped the import of attributes.m4 and extended the existing m4/compiler_options.m4. Also, disabled the linking test when using asan. Repushed bg/asan-bgo761429.
After discussion on IRC, re-imported attributes.m4. The plan is to replace our custom compiler macros with ones from the new file, but that can be done later as a separate work.
+1 I'm fine with this as it is.
Merged: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b1e267cd8ac371ff97186cdb986314643041cd6a