tests: add ASAN and LSAN suppression files for known libfyaml bugs

changes/+asdf-mapping-pop.bugfix

@@ -0,0 +1 @@
+Fixed small memory leak in `asdf_mapping_pop`.

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])],

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;

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}")

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 \

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

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