GNOME Bugzilla – Bug 783220
Add ASan and UBSan to GJS
Last modified: 2017-08-25 03:39:22 UTC
This patch adds ASan and UBSan to GJS configure and build process. The patch is safe since nothing will change except the user enable one of the new options (checks): * --enable-asan * --enable-ubsan As I told once, ASan detects memory leaks in GJS [1]. I never had luck with UBSan in general (I guess it was not stable). Anyway, it worked just fine for GJS; no warnings or errors. ---- [1] in fact, it is complicated. We are facing problems while calling routines in linked libraries. It is deeper.
Created attachment 352833 [details] [review] Add sanitizer options
Review of attachment 352833 [details] [review]: Really nice! When running the tests with asan, there is a use-after-free which actually may be related to bug 781799, so this is really helpful. I had to remove -Wno-undefined from libgjs_la_LDFLAGS in Makefile.am for this to work. Otherwise it fails with undefined symbols while linking libgjs.la. I am pretty sure that's not what I'm supposed to do :-) Perhaps the sanitizer flags need to be added to there instead of to AM_LDFLAGS? In that case I would suggest doing AC_SUBST([SAN_FLAGS]) in configure.ac, and adding $(SAN_FLAGS) in the appropriate places in each make target. Here as well, consider adding a paragraph of instructions to doc/Hacking.md. ::: configure.ac @@ +212,3 @@ + +AS_IF([test "x$enable_asan" = "xyes"], [ + AX_CHECK_COMPILE_FLAG([-fsanitize=address], [ I think this can be simplified using AX_APPEND_COMPILE_FLAGS (https://www.gnu.org/software/autoconf-archive/ax_append_compile_flags.html#ax_append_compile_flags). Maybe something like this, although you would have to split it into two separate variables. AX_APPEND_COMPILE_FLAGS([-fsanitize=address -fno-omit-frame-pointer -g], [SAN_CFLAGS]) AX_APPEND_LINK_FLAGS([-fsanitize=address -fno-omit-frame-pointer -g], [SAN_LDFLAGS]) @@ +221,3 @@ + ]) + GJS_CFLAGS="$GJS_CFLAGS $SAN_FLAGS" + AX_CHECK_COMPILE_FLAG([-fsanitize=address], [ Should be AM_LDFLAGS, otherwise we will clobber the user's linker flags (see for more information https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html) @@ +227,3 @@ + [AS_HELP_STRING([--enable-ubsan], [Build with undefined behavior sanitizer support @<:@default: no@:>@])]) + + AX_CHECK_COMPILE_FLAG([-fno-omit-frame-pointer -g], [ Is it possible to have --enable-asan and --enable-ubsan at the same time? If so, then you should not overwrite SAN_FLAGS from asan with ubsan. If not, then you should check for the case where they are both specified and exit with AC_MSG_ERROR.
(In reply to Philip Chimento from comment #2) > > I had to remove -Wno-undefined from libgjs_la_LDFLAGS in Makefile.am for > this to work. Otherwise it fails with undefined symbols while linking > libgjs.la. I tested this inside Travis CI and I saw no errors. If you add -ldl to LD_FLAGS does it work? Anyway: I've got a new test machine, I will try to reproduce locally. I'm attaching a new patch. I will start my testing using it because it is clear. Later, I can try the approach you suggested.
Created attachment 357600 [details] [review] Add sanitizer build options
I'm still futzing around trying to get it to work with Clang, but one suggestion I thought of in the meantime is combining the two flags. It looks like they are meant to be able to be used together, so no reason to enable one and not the other, right? In any case I found the answer to the -no-undefined question at https://github.com/google/sanitizers/issues/380
I think you were right about LDFLAGS versus AM_LDFLAGS before. Otherwise the sanitizer flags are not added when compiling GIRs with g-ir-scanner. Here's a patch that works for me. Let me know if it works for you as well, and I'll commit it.
Created attachment 358038 [details] [review] maint: add ASAN Address Sanitizer AddressSanitizer (or ASan) is a programming tool that detects memory corruption bugs such as buffer overflows or use after free. AddressSanitizer is based on compiler instrumentation. UndefinedBehaviorSanitizer (or UBSan) is a fast undefined behavior detector. It modifies the program at compile-time to catch errors such as using misaligned or null pointer and signed integer overflow.
Review of attachment 357600 [details] [review]: Updated with no-undefined fix and LDFLAGS.
Oh yeah, and also let me know what you think of combining the two flags into one. (If you are able to check today, then we can include it in today's 1.49.91. Otherwise we can put it in 1.49.92 in two weeks.
The new patch seems to work fine to me (sorry, I'm having troubles in my dev machine). - You can combine both flags or use any one of them. - While testing, I prefer to not combine them. But, this is personal, anyone else might prefer to test using, e.g. --enable-sanitizers. Your call. BTW: ASAN and UBSAN are becoming better. An updated compiler is advised (yes, a warning about ancient LTS distros). Please, let me know if you want a note in about this.
Created attachment 358295 [details] [review] Add sanitizer to doc BTW: there is some moz38 inside docs.
Review of attachment 358295 [details] [review]: +1
I squashed both together. Thanks! Attachment 358038 [details] pushed as 9883073 - maint: add ASAN Address Sanitizer I also fixed the mozjs38 in Hacking.md, thanks for pointing it out.