From 5d2441eb59bd8d3b51384ccdf5666e3f77b08389 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 23 Apr 2024 09:48:37 -0400 Subject: [PATCH 1/3] Makefile: only use ASAN for debug+clang Previously if `$CC` was `clang` the `Makefile` would configure `CFLAGS` and `LDFLAGS` with address sanitizer (ASAN) enabled. We only want this to happen for _debug_ builds that are using `clang`, since it's generally considered bad practice to ship sanitizer builds as release artifacts. This commit updates the logic to require `PROFILE=debug`, and then switches CI to use that profile when running tests. The Valgrind test continues to use `PROFILE=release` because ASAN and Valgrind will conflict, raising the error "ASan runtime does not come first in initial library list;". It may be possible to use both with more care/configuration, but for now we'll just use valgrind with a release build. --- .github/workflows/libssl.yaml | 6 +++--- rustls-libssl/Makefile | 2 ++ rustls-libssl/simpleserv.sh | 8 ++++++++ 3 files changed, 13 insertions(+), 3 deletions(-) create mode 100755 rustls-libssl/simpleserv.sh diff --git a/.github/workflows/libssl.yaml b/.github/workflows/libssl.yaml index 18b73b8..cce577a 100644 --- a/.github/workflows/libssl.yaml +++ b/.github/workflows/libssl.yaml @@ -39,8 +39,8 @@ jobs: with: toolchain: ${{ matrix.rust }} - - run: make PROFILE=release test - - run: make PROFILE=release integration + - run: make PROFILE=debug test + - run: make PROFILE=debug integration valgrind: name: Valgrind @@ -55,7 +55,7 @@ jobs: run: sudo apt-get update && sudo apt-get install -y valgrind - name: Install build dependencies run: sudo apt-get update && sudo apt-get install -y openssl libssl3 libssl-dev lld - - run: VALGRIND="valgrind -q" make test integration + - run: VALGRIND="valgrind -q" make PROFILE=release test integration docs: name: Check for documentation errors diff --git a/rustls-libssl/Makefile b/rustls-libssl/Makefile index 1092470..d7c5acf 100644 --- a/rustls-libssl/Makefile +++ b/rustls-libssl/Makefile @@ -4,10 +4,12 @@ CARGOFLAGS += --locked CFLAGS := -Werror -Wall -Wextra -Wpedantic -g $(shell pkg-config --cflags openssl) PROFILE := debug +ifeq ($(PROFILE), debug) ifeq ($(CC), clang) CFLAGS += -fsanitize=address -fsanitize=undefined LDFLAGS += -fsanitize=address endif +endif ifeq ($(PROFILE), release) CFLAGS += -O3 diff --git a/rustls-libssl/simpleserv.sh b/rustls-libssl/simpleserv.sh new file mode 100755 index 0000000..cd6c6c9 --- /dev/null +++ b/rustls-libssl/simpleserv.sh @@ -0,0 +1,8 @@ +#!/usr/bin/env bash +openssl s_server \ + -cert test-ca/rsa/end.cert \ + -cert_chain test-ca/rsa/inter.cert \ + -key test-ca/rsa/end.key \ + -alpn "hello,world" \ + -accept localhost:4443 \ + -rev From e46753c806c0fe37b756f3946d97b482409a953b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 23 Apr 2024 09:49:43 -0400 Subject: [PATCH 2/3] Makefile: enable ASAN for GCC Previously the `Makefile` conditionally enabled address sanitizer (ASAN) based on whether the compiler was `clang`. Since GCC has supported address sanitizer since GCC 4.8[0] we can remove this check and always use ASAN for both supported compilers. The only snag is that I observed linker errors about undefined references to `__ubsan_handle_type_mismatch` unless I fix the `LDFLAGS` to also include `-fsanitize=undefined`. This was already included in the `CFLAGS`. [0]: https://github.com/google/sanitizers/wiki/AddressSanitizer#getting-addresssanitizer --- rustls-libssl/Makefile | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rustls-libssl/Makefile b/rustls-libssl/Makefile index d7c5acf..9c7f592 100644 --- a/rustls-libssl/Makefile +++ b/rustls-libssl/Makefile @@ -5,10 +5,8 @@ CFLAGS := -Werror -Wall -Wextra -Wpedantic -g $(shell pkg-config --cflags openss PROFILE := debug ifeq ($(PROFILE), debug) -ifeq ($(CC), clang) CFLAGS += -fsanitize=address -fsanitize=undefined - LDFLAGS += -fsanitize=address -endif + LDFLAGS += -fsanitize=address -fsanitize=undefined endif ifeq ($(PROFILE), release) From def66087acfeee21c718222de2e529ad72ddfcb5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 23 Apr 2024 09:52:07 -0400 Subject: [PATCH 3/3] ci: verify ASAN presence/absence as appropriate --- .github/workflows/libssl.yaml | 14 ++++++++++++++ rustls-libssl/simpleserv.sh | 8 -------- 2 files changed, 14 insertions(+), 8 deletions(-) delete mode 100755 rustls-libssl/simpleserv.sh diff --git a/.github/workflows/libssl.yaml b/.github/workflows/libssl.yaml index cce577a..3a9d011 100644 --- a/.github/workflows/libssl.yaml +++ b/.github/workflows/libssl.yaml @@ -41,6 +41,20 @@ jobs: - run: make PROFILE=debug test - run: make PROFILE=debug integration + # Note: we only check the client/server binaries here, assuming that + # is sufficient for any other test binaries. + - name: Verify debug builds were using ASAN + run: | + nm target/client | grep '__asan_init' + nm target/server | grep '__asan_init' + - name: Build release binaries + run: | + make clean + make PROFILE=release + - name: Verify release builds were not using ASAN + run: | + nm target/client | grep -v '__asan_init' + nm target/server | grep -v '__asan_init' valgrind: name: Valgrind diff --git a/rustls-libssl/simpleserv.sh b/rustls-libssl/simpleserv.sh deleted file mode 100755 index cd6c6c9..0000000 --- a/rustls-libssl/simpleserv.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env bash -openssl s_server \ - -cert test-ca/rsa/end.cert \ - -cert_chain test-ca/rsa/inter.cert \ - -key test-ca/rsa/end.key \ - -alpn "hello,world" \ - -accept localhost:4443 \ - -rev