diff --git a/changes/+asdf-mapping-pop.bugfix b/changes/+asdf-mapping-pop.bugfix new file mode 100644 index 00000000..8f6cb8b7 --- /dev/null +++ b/changes/+asdf-mapping-pop.bugfix @@ -0,0 +1 @@ +Fixed small memory leak in `asdf_mapping_pop`. diff --git a/configure.ac b/configure.ac index baa2268a..9e789fe4 100644 --- a/configure.ac +++ b/configure.ac @@ -6,6 +6,7 @@ AC_INIT( [https://github.com/asdf-format/libasdf], ) AC_CONFIG_AUX_DIR([build-aux]) +AC_CANONICAL_HOST AM_INIT_AUTOMAKE([foreign subdir-objects]) # needed for strptime, at the very least... @@ -154,6 +155,10 @@ AS_IF([test "x$with_asan" = "xyes"], [ ASDF_BUILD_MODE="${ASDF_BUILD_MODE}asan " ]) +AM_CONDITIONAL([ASAN], [test "x$with_asan" = "xyes"]) + +AM_CONDITIONAL([HOST_LINUX], [case "$host_os" in linux*) true;; *) false;; esac]) + # --with-ubsan AC_ARG_WITH([ubsan], [AS_HELP_STRING([--with-ubsan], [Build with UndefinedBehaviorSanitizer support])], diff --git a/src/value.c b/src/value.c index 5a0315e4..ef58a819 100644 --- a/src/value.c +++ b/src/value.c @@ -792,12 +792,27 @@ asdf_value_t *asdf_mapping_pop(asdf_mapping_t *mapping, const char *key) { struct fy_node *key_node = asdf_node_of_string0(tree, key); - if (UNLIKELY(!key)) { + if (UNLIKELY(!key_node)) { ASDF_ERROR_OOM(value->file); return NULL; } - struct fy_node *node = fy_node_mapping_remove_by_key(value->node, key_node); + // fy_node_mapping_remove_by_key() changed its key-ownership contract + // between libfyaml versions: older ones free the passed lookup key (unless + // it is the pair's own stored key), newer ones leave it caller-owned (see + // libfyaml issue #261). Freeing our own throwaway key is therefore a + // double free on old libfyaml and a leak if omitted on new. To stay + // correct on both, look the pair up first: lookup never takes ownership + // of the key, so we can (and must) free our throwaway key node here. Then + // remove using the pair's *own* stored key, which both versions leave to + // the pair's normal teardown (no double free, no leak), but ugly... + struct fy_node_pair *pair = fy_node_mapping_lookup_pair(value->node, key_node); + fy_node_free(key_node); + + if (!pair) + return NULL; + + struct fy_node *node = fy_node_mapping_remove_by_key(value->node, fy_node_pair_key(pair)); if (!node) return NULL; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 251eb715..c1f758cf 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -49,6 +49,8 @@ add_compile_definitions(TEMP_DIR=\"${TEMP_DIR}\") set(runtime "WITH_CMAKE=set:YES;OBJC_DISABLE_INITIALIZE_FORK_SAFETY=set:YES;srcdir=set:${CMAKE_CURRENT_SOURCE_DIR};top_srcdir=set:${CMAKE_SOURCE_DIR};top_builddir=set:${CMAKE_BINARY_DIR};LD_LIBRARY_PATH=path_list_prepend:${CMAKE_BINARY_DIR}/src") +set(sanitizer_options "ASAN_OPTIONS=detect_leaks=1:suppressions=${CMAKE_CURRENT_SOURCE_DIR}/libfyaml.asan.supp;LSAN_OPTIONS=suppressions=${CMAKE_CURRENT_SOURCE_DIR}/libfyaml.lsan.supp") + # UBSan reports go to stderr, which munit captures and discards unless the test # fails; halt_on_error turns a report into an abort so the report is surfaced if (ENABLE_UBSAN) @@ -135,6 +137,18 @@ foreach(source_file ${source_files}) PROPERTIES SKIP_RETURN_CODE 127) + # The libfyaml backend has two known upstream bugs surfaced by the + # sanitizers: a heap-buffer-overflow in fy_node_get_path() (0.9.6) and a + # leak of a tag-token buffer on emit (<0.9). libfyaml.asan.supp and + # libfyaml.lsan.supp suppress just those, so genuine issues still surface. + # LeakSanitizer only works on Linux. + if(ENABLE_ASAN AND CMAKE_SYSTEM_NAME STREQUAL "Linux") + set_tests_properties(${test_executable} + PROPERTIES + ENVIRONMENT + "${sanitizer_options}") + endif() + set(ENV{OBJC_DISABLE_INITIALIZE_FORK_SAFETY} "YES") set_property(TEST ${test_executable} PROPERTY ENVIRONMENT_MODIFICATION "${runtime}") diff --git a/tests/Makefile.am b/tests/Makefile.am index 1522c8a2..10b6b459 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -58,6 +58,19 @@ if UBSAN TESTS_ENVIRONMENT += UBSAN_OPTIONS="$${UBSAN_OPTIONS:-print_stacktrace=1:halt_on_error=1}" endif +if ASAN +if HOST_LINUX +# Suppress two distinct known upstream libfyaml bugs so genuine issues still +# surface (each sanitizer uses its own suppression-file syntax and env var): +# - libfyaml.asan.supp: a heap-buffer-overflow in fy_node_get_path() (0.9.6) +# - libfyaml.lsan.supp: a leak of a tag-token buffer on emit (<0.9) +# LeakSanitizer only runs on Linux, so this is gated on HOST_LINUX. +TESTS_ENVIRONMENT += \ + ASAN_OPTIONS="$${ASAN_OPTIONS:-detect_leaks=1:suppressions=$(abs_srcdir)/libfyaml.asan.supp}" \ + LSAN_OPTIONS="$${LSAN_OPTIONS:-suppressions=$(abs_srcdir)/libfyaml.lsan.supp}" +endif +endif + TESTS = $(check_PROGRAMS) unit_test_base_cppflags = \ @@ -349,6 +362,8 @@ AUTOMAKE_OPTIONS = parallel-tests CMAKE_DIST = CMakeLists.txt EXTRA_DIST = $(TOOL_TESTS) $(CMAKE_DIST) EXTRA_DIST += \ + libfyaml.asan.supp \ + libfyaml.lsan.supp \ munit.h \ shell-test.sh \ util.h \ diff --git a/tests/libfyaml.asan.supp b/tests/libfyaml.asan.supp new file mode 100644 index 00000000..2193be87 --- /dev/null +++ b/tests/libfyaml.asan.supp @@ -0,0 +1,21 @@ +# AddressSanitizer suppressions for the libfyaml YAML backend. +# +# libfyaml 0.9.6 has a heap-buffer-overflow in its node-path bookkeeping: while +# building a node's path string it reads a few bytes past a heap allocation. +# ASan reports it as a "READ of size N" inside the intercepted strdup(), called +# from fy_token_get_scalar_path_key() <- fy_node_get_parent_address() <- +# fy_node_get_path(). In libasdf it is reached whenever a value is copied out +# of the tree (asdf_value_copy_impl() in src/value.c, e.g. via +# asdf_get_ndarray()). +# +# ASan can only suppress errors raised from *inside* interceptors, which this +# one is (strdup), so interceptor_via_fun matching an exported frame on the +# stack works. fy_node_get_path is the deepest reliably-exported libfyaml +# symbol here (the internal frames are stripped in the system library). +# +# CAVEAT: this suppresses *any* heap-buffer-overflow reported from an +# interceptor with fy_node_get_path on the stack, so it could in principle mask +# a genuine libasdf bug that handed a corrupt node to fy_node_get_path. libasdf +# does not itself read out of bounds here; the overflow lives entirely in +# libfyaml's path bookkeeping. Remove this once linked against a fixed libfyaml. +interceptor_via_fun:fy_node_get_path diff --git a/tests/libfyaml.lsan.supp b/tests/libfyaml.lsan.supp new file mode 100644 index 00000000..27f433ac --- /dev/null +++ b/tests/libfyaml.lsan.supp @@ -0,0 +1,13 @@ +# LeakSanitizer suppressions for the libfyaml YAML backend. +# +# libfyaml<0.9 leaks the malloc'd buffer backing a tag token whenever an emitted +# event carries a tag. See the analysis in +# https://github.com/Starlink/ast/pull/67#issuecomment-4970509982 +# Fixed upstream in +# https://github.com/pantoniou/libfyaml/commit/115365e1600b2c11d7cb94d59ca72586a990cdae +# +# The system library is stripped of its internal symbols, so the deepest named +# frame is the exported fy_emit_event_vcreate(). Matching on it is reasonably +# safe: AST's own allocations never pass through libfyaml's event creation, +# so this cannot hide a genuine libasdf leak. +leak:fy_emit_event_vcreate