From: Ondřej Surý <ondrej@isc.org>
Date: Tue, 27 Sep 2022 11:35:26 +0000 (+0200)
Subject: Add autoconf option to enable memory leak detection in libraries
X-Git-Tag: v9.19.6~27^2~1
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3b31f7f5630578f2158e8e1f09147332fba6cd72;p=thirdparty%2Fbind9.git

Add autoconf option to enable memory leak detection in libraries

There's a known memory leak in the engine_pkcs11 at the time of writing
this and it interferes with the named ability to check for memory leaks
in the OpenSSL memory context by default.

Add an autoconf option to explicitly enable the memory leak detection,
and use it in the CI except for pkcs11 enabled builds.  When this gets
fixed in the engine_pkc11, the option can be enabled by default.
---

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c2b5986df2c..3d7e9eab3ad 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -230,6 +230,7 @@ stages:
       --with-cmocka
       --with-libxml2
       --with-json-c
+      --enable-leak-detection
       $EXTRA_CONFIGURE
       || (test -s config.log && cat config.log; exit 1)
 
@@ -757,7 +758,7 @@ gcc:sid:amd64:
     CC: gcc
     CFLAGS: "${CFLAGS_COMMON} -O3 -DOPENSSL_API_COMPAT=10100"
     # For the jemalloc ./configure option, see https://gitlab.isc.org/isc-projects/bind9/-/issues/3444
-    EXTRA_CONFIGURE: "--with-libidn2 --without-lmdb --without-jemalloc ${WITH_READLINE}"
+    EXTRA_CONFIGURE: "--with-libidn2 --without-lmdb --without-jemalloc --disable-leak-detection ${WITH_READLINE}"
     RUN_MAKE_INSTALL: 1
   <<: *debian_sid_amd64_image
   <<: *build_job
@@ -1037,7 +1038,7 @@ clang:bullseye:amd64:
     CC: ${CLANG}
     CFLAGS: "${CFLAGS_COMMON} -Wenum-conversion"
     # See https://gitlab.isc.org/isc-projects/bind9/-/issues/3444
-    EXTRA_CONFIGURE: "--without-jemalloc"
+    EXTRA_CONFIGURE: "--without-jemalloc --disable-leak-detection"
   <<: *debian_bullseye_amd64_image
   <<: *build_job
 
diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c
index a8ecf3f55fe..827461cb766 100644
--- a/bin/dig/dighost.c
+++ b/bin/dig/dighost.c
@@ -4705,9 +4705,11 @@ destroy_libs(void) {
 
 	isc_managers_destroy(&mctx, &loopmgr, &netmgr, &taskmgr);
 
+#if ENABLE_LEAK_DETECTION
 	isc__tls_setdestroycheck(true);
 	isc__uv_setdestroycheck(true);
 	isc__xml_setdestroycheck(true);
+#endif
 
 	isc_mem_checkdestroyed(stderr);
 }
diff --git a/bin/named/main.c b/bin/named/main.c
index 236c55d942f..0f3896e00b0 100644
--- a/bin/named/main.c
+++ b/bin/named/main.c
@@ -1582,11 +1582,14 @@ main(int argc, char *argv[]) {
 
 	isc_managers_destroy(&named_g_mctx, &named_g_loopmgr, &named_g_netmgr,
 			     &named_g_taskmgr);
-	isc_mem_checkdestroyed(stderr);
 
+#if ENABLE_LEAK_DETECTION
 	isc__tls_setdestroycheck(true);
 	isc__uv_setdestroycheck(true);
 	isc__xml_setdestroycheck(true);
+#endif
+
+	isc_mem_checkdestroyed(stderr);
 
 	named_main_setmemstats(NULL);
 
diff --git a/configure.ac b/configure.ac
index fc643e57e64..037b10a0b69 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1419,6 +1419,16 @@ AS_IF([test "$with_jemalloc" = "no"],
 
 AM_CONDITIONAL([HAVE_JEMALLOC], [test "$with_jemalloc" = "yes"])
 
+#
+# Check memory leaks in external libraries
+#
+# [pairwise: --enable-leak-detection, --disable-leak-detection]
+AC_ARG_ENABLE([leak-detection],
+	      [AS_HELP_STRING([--enable-leak-detection],[enable the memory leak detection in external libraries (libxml2, libuv, OpenSSL) (disabled by default)])],
+	      [],[enable_leak_detection=no])
+AS_CASE([$enable_leak_detection],
+	[yes],[AC_DEFINE([ENABLE_LEAK_DETECTION], [1], [Define to enable memory leak detection in external libraries])])
+
 #
 # was --with-tuning specified?
 #
diff --git a/doc/dev/dev.md b/doc/dev/dev.md
index be26ba6a33d..3ee7c3c9378 100644
--- a/doc/dev/dev.md
+++ b/doc/dev/dev.md
@@ -580,6 +580,19 @@ loops.
 None of these allocation functions, including `isc_mempool_get()`, can
 fail.  If no memory is available for allocation, the program will abort.
 
+The memory context can be set to check if all memory allocated via the said
+memory context was freed before the memory context was destroyed by calling
+`isc_mem_checkdestroyed()`.  This could lead to false positives on abnormal
+shutdowns, so the checking is only enabled in `dig` and `named` applications on
+normal shutdown.
+
+The memory context are normally used only for internal allocations, but several
+external libraries allow replacing their allocators (namely libxml2, libuv and
+OpenSSL).  As there has been known memory leak in the OpenSSL when
+`engine_pkcs11` is loaded, memory checking at destroy is disabled by default in
+the memory contexts used for external libraries and it needs to be enabled with
+a `--enable-leak-detection` autoconf option.
+
 #### <a name="lists"></a>Lists
 
 A set of macros are provided for creating, modifying and iterating
diff --git a/lib/isc/xml.c b/lib/isc/xml.c
index e7fbb19cdd0..9b5e52c8186 100644
--- a/lib/isc/xml.c
+++ b/lib/isc/xml.c
@@ -71,7 +71,7 @@ isc__xml_shutdown(void) {
 
 void
 isc__xml_setdestroycheck(bool check) {
-#ifdef HAVE_LIBXML2
+#if HAVE_LIBXML2
 	isc_mem_setdestroycheck(isc__xml_mctx, check);
 #else
 	UNUSED(check);