Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd: add support for building with address and undefined behavior sanitizers #15059

Merged
merged 6 commits into from
Feb 18, 2025
+68 −15
Merged

cmd: add support for building with address and undefined behavior sanitizers #15059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d72aa4
cmd: add support for building C code with address sanitizer
bboozzoo Feb 11, 2025
3ec4af0
github: unit-tests: add a step to build with asan
bboozzoo Feb 11, 2025
8928434
cmd: make: enable ubsan when building with sanitizers
bboozzoo Feb 13, 2025
8b515e6
github: unit-test: tweak step name
bboozzoo Feb 13, 2025
773c5c9
woke: ignore cmd/configure.ac cmd/Makefile.am
bboozzoo Feb 13, 2025
76dfa04
cmd: tweak help message for --enable-sanitize
bboozzoo Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions .github/workflows/unit-tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,27 @@ jobs:
cd cmd/
./autogen.sh
make -j$(nproc)
make clean

- name: Build Go
if: ${{ inputs.code == 'go' }}
run: |
go build ./...

- name: Test C
- name: Test C (distcheck)
if: ${{ inputs.code == 'c' }}
run: |
cd cmd/ && make distcheck


- name: Test C (check & ASan & UBsan)
if: ${{ inputs.code == 'c' }}
run: |
cd cmd/
./autogen.sh --enable-sanitize
make -j$(nproc)
make check
make clean

- name: Set SNAPD_DEBUG=1
if: ${{ inputs.snapd-debug }}
run: echo "SNAPD_DEBUG=1" >> $GITHUB_ENV
Expand Down
2 changes: 2 additions & 0 deletions .woke.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ ignore_files:
- packaging/ubuntu-14.04/changelog
- packaging/ubuntu-16.04/changelog
- tests/lib/snaps/store/test-snapd-ovmf/snapcraft.yaml
- cmd/configure.ac
- cmd/Makefile.am
51 changes: 38 additions & 13 deletions cmd/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ noinst_PROGRAMS =
noinst_LIBRARIES =

AM_CFLAGS = $(CHECK_CFLAGS)
if ENABLE_SANITIZE
SANITIZE_CFLAGS = -fsanitize=address -fsanitize=undefined
SANITIZE_LDFLAGS = -fsanitize=address -fsanitize=undefined
endif

if USE_INTERNAL_BPF_HEADERS
VENDOR_BPF_HEADERS_CFLAGS = -I$(srcdir)/libsnap-confine-private/bpf/vendor
Expand All @@ -30,11 +34,18 @@ check: check-unit-tests

.PHONY: check-unit-tests
if WITH_UNIT_TESTS
# valgrind and asan cannot be used together
if ENABLE_SANITIZE
UNIT_TESTS_CMD =
else
UNIT_TESTS_CMD = $(if $(HAVE_VALGRIND),$(HAVE_VALGRIND) --leak-check=full)
endif

check-unit-tests: snap-confine/unit-tests system-shutdown/unit-tests libsnap-confine-private/unit-tests snap-device-helper/unit-tests
$(if $(HAVE_VALGRIND),$(HAVE_VALGRIND) --leak-check=full) ./libsnap-confine-private/unit-tests
$(if $(HAVE_VALGRIND),$(HAVE_VALGRIND) --leak-check=full) ./snap-confine/unit-tests
$(if $(HAVE_VALGRIND),$(HAVE_VALGRIND) --leak-check=full) ./system-shutdown/unit-tests
$(if $(HAVE_VALGRIND),$(HAVE_VALGRIND) --leak-check=full) ./snap-device-helper/unit-tests
$(UNIT_TESTS_CMD) ./libsnap-confine-private/unit-tests
$(UNIT_TESTS_CMD) ./snap-confine/unit-tests
$(UNIT_TESTS_CMD) ./system-shutdown/unit-tests
$(UNIT_TESTS_CMD) ./snap-device-helper/unit-tests
else
check-unit-tests:
echo "unit tests are disabled (rebuild with --enable-unit-tests)"
Expand Down Expand Up @@ -123,11 +134,11 @@ libsnap_confine_private_a_SOURCES += \
libsnap-confine-private/bpf-support.c \
libsnap-confine-private/bpf-support.h
endif
libsnap_confine_private_a_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS)
libsnap_confine_private_a_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS) $(SANITIZE_CFLAGS)

noinst_LIBRARIES += libsnap-confine-private-debug.a
libsnap_confine_private_debug_a_SOURCES = $(libsnap_confine_private_a_SOURCES)
libsnap_confine_private_debug_a_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS) -DSNAP_CONFINE_DEBUG_BUILD=1
libsnap_confine_private_debug_a_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS) $(SANITIZE_CFLAGS) -DSNAP_CONFINE_DEBUG_BUILD=1

if WITH_UNIT_TESTS

Expand Down Expand Up @@ -165,7 +176,8 @@ libsnap_confine_private_unit_tests_SOURCES = \
libsnap-confine-private/test-utils-test.c \
libsnap-confine-private/utils-test.c

libsnap_confine_private_unit_tests_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS) $(GLIB_CFLAGS)
libsnap_confine_private_unit_tests_CFLAGS = $(AM_CFLAGS) $(VENDOR_BPF_HEADERS_CFLAGS) $(GLIB_CFLAGS) $(SANITIZE_CFLAGS)
libsnap_confine_private_unit_tests_LDFLAGS = $(SANITIZE_LDFLAGS)
libsnap_confine_private_unit_tests_LDADD = $(GLIB_LIBS) libsnap-confine-private-test-support.a
libsnap_confine_private_unit_tests_CFLAGS += -D_ENABLE_FAULT_INJECTION
libsnap_confine_private_unit_tests_STATIC =
Expand Down Expand Up @@ -193,6 +205,7 @@ noinst_PROGRAMS += decode-mount-opts/decode-mount-opts
decode_mount_opts_decode_mount_opts_SOURCES = \
decode-mount-opts/decode-mount-opts.c
decode_mount_opts_decode_mount_opts_LDADD = libsnap-confine-private.a
decode_mount_opts_decode_mount_opts_LDFLAGS = $(SANITIZE_LDFLAGS)
decode_mount_opts_decode_mount_opts_STATIC =

if STATIC_LIBCAP
Expand Down Expand Up @@ -246,8 +259,8 @@ snap_confine_snap_confine_SOURCES = \
snap-confine/user-support.c \
snap-confine/user-support.h

snap_confine_snap_confine_CFLAGS = $(AM_CFLAGS) -DLIBEXECDIR=\"$(libexecdir)\" -DNATIVE_LIBDIR=\"$(libdir)\"
snap_confine_snap_confine_LDFLAGS = $(AM_LDFLAGS)
snap_confine_snap_confine_CFLAGS = $(AM_CFLAGS) -DLIBEXECDIR=\"$(libexecdir)\" -DNATIVE_LIBDIR=\"$(libdir)\" $(SANITIZE_CFLAGS)
snap_confine_snap_confine_LDFLAGS = $(AM_LDFLAGS) $(SANITIZE_LDFLAGS)
snap_confine_snap_confine_LDADD = libsnap-confine-private.a
snap_confine_snap_confine_CFLAGS += $(LIBUDEV_CFLAGS)
snap_confine_snap_confine_LDADD += $(snap_confine_snap_confine_extra_libs)
Expand Down Expand Up @@ -324,9 +337,9 @@ snap_confine_unit_tests_SOURCES = \
snap-confine/seccomp-support-test.c \
snap-confine/snap-confine-args-test.c \
snap-confine/snap-confine-invocation-test.c
snap_confine_unit_tests_CFLAGS = $(snap_confine_snap_confine_CFLAGS) $(GLIB_CFLAGS)
snap_confine_unit_tests_CFLAGS = $(snap_confine_snap_confine_CFLAGS) $(GLIB_CFLAGS) $(SANITIZE_CFLAGS)
snap_confine_unit_tests_LDADD = $(snap_confine_snap_confine_LDADD) $(GLIB_LIBS) libsnap-confine-private-test-support.a
snap_confine_unit_tests_LDFLAGS = $(snap_confine_snap_confine_LDFLAGS)
snap_confine_unit_tests_LDFLAGS = $(snap_confine_snap_confine_LDFLAGS) $(SANITIZE_LDFLAGS)
snap_confine_unit_tests_STATIC = $(snap_confine_snap_confine_STATIC)

# Use a hacked rule if we're doing static build. This allows us to inject the LIBS += .. rule below.
Expand Down Expand Up @@ -415,7 +428,7 @@ snap_device_helper_snap_device_helper_SOURCES = \
snap-device-helper/main.c \
snap-device-helper/snap-device-helper.c \
snap-device-helper/snap-device-helper.h
snap_device_helper_snap_device_helper_LDFLAGS = $(AM_LDFLAGS)
snap_device_helper_snap_device_helper_LDFLAGS = $(AM_LDFLAGS) $(SANITIZE_LDFLAGS)
snap_device_helper_snap_device_helper_LDADD = libsnap-confine-private.a

if WITH_UNIT_TESTS
Expand All @@ -441,7 +454,7 @@ EXTRA_DIST += snap-discard-ns/snap-discard-ns.rst

snap_discard_ns_snap_discard_ns_SOURCES = \
snap-discard-ns/snap-discard-ns.c
snap_discard_ns_snap_discard_ns_LDFLAGS = $(AM_LDFLAGS)
snap_discard_ns_snap_discard_ns_LDFLAGS = $(AM_LDFLAGS) $(SANITIZE_LDFLAGS)
snap_discard_ns_snap_discard_ns_LDADD = libsnap-confine-private.a
snap_discard_ns_snap_discard_ns_STATIC =

Expand All @@ -463,6 +476,7 @@ system_shutdown_system_shutdown_SOURCES = \
system-shutdown/system-shutdown-utils.h \
system-shutdown/system-shutdown.c
system_shutdown_system_shutdown_LDADD = libsnap-confine-private.a
system_shutdown_system_shutdown_LDFLAGS = $(SANITIZE_LDFLAGS)

if WITH_UNIT_TESTS
noinst_PROGRAMS += system-shutdown/unit-tests
Expand All @@ -471,13 +485,18 @@ system_shutdown_unit_tests_SOURCES = \
system_shutdown_unit_tests_LDADD = libsnap-confine-private.a libsnap-confine-private-test-support.a
system_shutdown_unit_tests_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
system_shutdown_unit_tests_LDADD += $(GLIB_LIBS)
system_shutdown_unit_tests_LDFLAGS = $(system_shutdown_system_shutdown_LDFLAGS)
endif

##
## snap-gdb-shim
##

if !ENABLE_SANITIZE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you do not add SANITIZE_* to snap_gdb_shim_snap_gdb_shim_*. So that would still build, right? Is it just to disable what we are not testing here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is so, we should maybe document in the AS_HELP_STRING of --enable-sanitize that it gives a partial build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of both. We don't test it and it won't build. Even with -static-libasan dlopen() is retained for some reason. I guess it just makes libasan parts to be linked statically, but still relies on hijacking libc's malloc dynamically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tweaked the help message.

# snap-gdb-shim is statically linked and cannot be built when using
# -fsanitize=address
libexec_PROGRAMS += snap-gdb-shim/snap-gdb-shim
endif

snap_gdb_shim_snap_gdb_shim_SOURCES = \
snap-gdb-shim/snap-gdb-shim.c
Expand All @@ -489,7 +508,11 @@ snap_gdb_shim_snap_gdb_shim_LDFLAGS = -static
## snap-gdbserver-shim
##

if !ENABLE_SANITIZE
# snap-gdbserver-shim is statically linked and cannot be built when using
# -fsanitize=address
libexec_PROGRAMS += snap-gdb-shim/snap-gdbserver-shim
endif

snap_gdb_shim_snap_gdbserver_shim_SOURCES = \
snap-gdb-shim/snap-gdbserver-shim.c
Expand All @@ -510,6 +533,7 @@ endif

snapd_generator_snapd_generator_SOURCES = snapd-generator/main.c
snapd_generator_snapd_generator_LDADD = libsnap-confine-private.a
snapd_generator_snapd_generator_LDFLAGS = $(SANITIZE_LDFLAGS)

##
## snapd-env-generator
Expand All @@ -525,6 +549,7 @@ endif

snapd_env_generator_snapd_env_generator_SOURCES = snapd-env-generator/main.c
snapd_env_generator_snapd_env_generator_LDADD = libsnap-confine-private.a
snapd_env_generator_snapd_env_generator_LDFLAGS = $(SANITIZE_LDFLAGS)
EXTRA_DIST += snapd-env-generator/snapd-env-generator.rst

if BUILD_HOST_BINARIES
Expand Down
16 changes: 16 additions & 0 deletions cmd/configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,22 @@ AS_IF([test "x$with_unit_tests" = "xyes"], [
AX_APPEND_COMPILE_FLAGS([-Werror], [CHECK_CFLAGS])
])

AC_ARG_ENABLE([sanitize],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only using asan at the moment, would it be better to just call it asan rather than sanitize?

AS_HELP_STRING([--enable-sanitize], [Build binaries with sanitizers (ASan and UBSan).
Note, this will perform a partial build, skipping binaries which
cannot be built with sanitizers enabled.]),
[case "$enableval" in
yes)
enable_sanitize=yes
;;
no)
enable_sanitize=no
;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-sanitize])
esac],
[enable_sanitize=no])
AM_CONDITIONAL([ENABLE_SANITIZE], [test "x$enable_sanitize" = "xyes"])

AC_SUBST([CHECK_CFLAGS])

AC_ARG_WITH([apparmorconfigdir],
Expand Down
Loading