From 2d2022a5094bb82598e695425ba54f8e29643a01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 13:49:31 +0200 Subject: [PATCH 1/8] Make the debugging flags local to the memory context Previously, the isc_mem_debugging would be single global variable that would affect the behavior of the memory context whenever it would be changed which could be after some allocation were already done. Change the memory debugging options to be local to the memory context and immutable, so all allocations within the same memory context are treated the same. --- lib/isc/mem.c | 42 ++++++++++++++++++++++-------------------- tests/isc/mem_test.c | 32 ++++++++++++++------------------ 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index f828672119..767f910c13 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -132,6 +132,7 @@ static uint64_t totallost; struct isc_mem { unsigned int magic; unsigned int flags; + unsigned int debugging; isc_mutex_t lock; bool checkfree; struct stats stats[STATS_BUCKETS + 1]; @@ -184,23 +185,23 @@ struct isc_mempool { */ #if !ISC_MEM_TRACKLINES -#define ADD_TRACE(a, b, c, d, e) -#define DELETE_TRACE(a, b, c, d, e) +#define ADD_TRACE(mctx, ptr, size, file, line) +#define DELETE_TRACE(mctx, ptr, size, file, line) #define ISC_MEMFUNC_SCOPE #else /* if !ISC_MEM_TRACKLINES */ #define TRACE_OR_RECORD (ISC_MEM_DEBUGTRACE | ISC_MEM_DEBUGRECORD) -#define SHOULD_TRACE_OR_RECORD(ptr) \ - ((isc_mem_debugging & TRACE_OR_RECORD) != 0 && ptr != NULL) +#define SHOULD_TRACE_OR_RECORD(mctx, ptr) \ + (((mctx)->debugging & TRACE_OR_RECORD) != 0 && ptr != NULL) -#define ADD_TRACE(a, b, c, d, e) \ - if (SHOULD_TRACE_OR_RECORD(b)) { \ - add_trace_entry(a, b, c, d, e); \ +#define ADD_TRACE(mctx, ptr, size, file, line) \ + if (SHOULD_TRACE_OR_RECORD(mctx, ptr)) { \ + add_trace_entry(mctx, ptr, size, file, line); \ } -#define DELETE_TRACE(a, b, c, d, e) \ - if (SHOULD_TRACE_OR_RECORD(b)) { \ - delete_trace_entry(a, b, c, d, e); \ +#define DELETE_TRACE(mctx, ptr, size, file, line) \ + if (SHOULD_TRACE_OR_RECORD(mctx, ptr)) { \ + delete_trace_entry(mctx, ptr, size, file, line); \ } static void @@ -239,7 +240,7 @@ add_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size FLARG) { MCTXLOCK(mctx); - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((mctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "add %p size %zu file %s line %u mctx %p\n", ptr, size, file, line, mctx); } @@ -284,7 +285,7 @@ delete_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size, MCTXLOCK(mctx); - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((mctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "del %p size %zu file %s line %u mctx %p\n", ptr, size, file, line, mctx); } @@ -456,7 +457,7 @@ isc__mem_shutdown(void) { } static void -mem_create(isc_mem_t **ctxp, unsigned int flags) { +mem_create(isc_mem_t **ctxp, unsigned int debugging, unsigned int flags) { isc_mem_t *ctx = NULL; REQUIRE(ctxp != NULL && *ctxp == NULL); @@ -466,6 +467,7 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) { *ctx = (isc_mem_t){ .magic = MEM_MAGIC, + .debugging = debugging, .flags = flags, .checkfree = true, }; @@ -490,7 +492,7 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) { ISC_LIST_INIT(ctx->pools); #if ISC_MEM_TRACKLINES - if ((isc_mem_debugging & ISC_MEM_DEBUGRECORD) != 0) { + if ((ctx->debugging & ISC_MEM_DEBUGRECORD) != 0) { unsigned int i; ctx->debuglist = @@ -605,7 +607,7 @@ isc__mem_detach(isc_mem_t **ctxp FLARG) { if (isc_refcount_decrement(&ctx->references) == 1) { isc_refcount_destroy(&ctx->references); #if ISC_MEM_TRACKLINES - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((ctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "destroy mctx %p file %s line %u\n", ctx, file, line); } @@ -662,7 +664,7 @@ isc__mem_destroy(isc_mem_t **ctxp FLARG) { *ctxp = NULL; #if ISC_MEM_TRACKLINES - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((ctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "destroy mctx %p file %s line %u\n", ctx, file, line); } @@ -713,7 +715,7 @@ hi_water(isc_mem_t *ctx) { (void)atomic_compare_exchange_strong(&ctx->maxinuse, &maxinuse, inuse); - if ((isc_mem_debugging & ISC_MEM_DEBUGUSAGE) != 0) { + if ((ctx->debugging & ISC_MEM_DEBUGUSAGE) != 0) { fprintf(stderr, "maxinuse = %lu\n", (unsigned long)inuse); } @@ -1189,7 +1191,7 @@ isc__mempool_create(isc_mem_t *restrict mctx, const size_t element_size, }; #if ISC_MEM_TRACKLINES - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((mctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "create pool %p file %s line %u mctx %p\n", mpctx, file, line, mctx); } @@ -1229,7 +1231,7 @@ isc__mempool_destroy(isc_mempool_t **restrict mpctxp FLARG) { mctx = mpctx->mctx; #if ISC_MEM_TRACKLINES - if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { + if ((mctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "destroy pool %p file %s line %u mctx %p\n", mpctx, file, line, mctx); } @@ -1747,7 +1749,7 @@ error: void isc__mem_create(isc_mem_t **mctxp FLARG) { - mem_create(mctxp, isc_mem_defaultflags); + mem_create(mctxp, isc_mem_debugging, isc_mem_defaultflags); #if ISC_MEM_TRACKLINES if ((isc_mem_debugging & ISC_MEM_DEBUGTRACE) != 0) { fprintf(stderr, "create mctx %p file %s line %u\n", *mctxp, diff --git a/tests/isc/mem_test.c b/tests/isc/mem_test.c index f2bbdbd3f4..183b2e7891 100644 --- a/tests/isc/mem_test.c +++ b/tests/isc/mem_test.c @@ -296,7 +296,7 @@ ISC_RUN_TEST_IMPL(isc_mem_reget) { ISC_RUN_TEST_IMPL(isc_mem_noflags) { isc_result_t result; isc_mem_t *mctx2 = NULL; - char buf[4096], *p, *q; + char buf[4096], *p; FILE *f; void *ptr; @@ -305,13 +305,14 @@ ISC_RUN_TEST_IMPL(isc_mem_noflags) { UNUSED(state); - isc_mem_create(&mctx2); isc_mem_debugging = 0; + isc_mem_create(&mctx2); ptr = isc_mem_get(mctx2, 2048); assert_non_null(ptr); isc__mem_printactive(mctx2, f); isc_mem_put(mctx2, ptr, 2048); isc_mem_destroy(&mctx2); + isc_mem_debugging = ISC_MEM_DEBUGRECORD; isc_stdio_close(f); memset(buf, 0, sizeof(buf)); @@ -325,15 +326,7 @@ ISC_RUN_TEST_IMPL(isc_mem_noflags) { buf[sizeof(buf) - 1] = 0; p = strchr(buf, '\n'); - assert_non_null(p); - assert_in_range(p, 0, buf + sizeof(buf) - 3); - p += 2; - q = strchr(p, '\n'); - assert_non_null(q); - *q = '\0'; - assert_string_equal(p, "None."); - - isc_mem_debugging = ISC_MEM_DEBUGRECORD; + assert_null(p); } /* test mem with record flag */ @@ -390,13 +383,14 @@ ISC_RUN_TEST_IMPL(isc_mem_traceflag) { UNUSED(state); + isc_mem_debugging = ISC_MEM_DEBUGRECORD | ISC_MEM_DEBUGTRACE; isc_mem_create(&mctx2); - isc_mem_debugging = ISC_MEM_DEBUGTRACE; ptr = isc_mem_get(mctx2, 2048); assert_non_null(ptr); isc__mem_printactive(mctx2, f); isc_mem_put(mctx2, ptr, 2048); isc_mem_destroy(&mctx2); + isc_mem_debugging = ISC_MEM_DEBUGRECORD; isc_stdio_close(f); memset(buf, 0, sizeof(buf)); @@ -412,9 +406,13 @@ ISC_RUN_TEST_IMPL(isc_mem_traceflag) { buf[sizeof(buf) - 1] = 0; - assert_memory_equal(buf, "add ", 4); + assert_memory_equal(buf, "create ", 6); p = strchr(buf, '\n'); assert_non_null(p); + + assert_memory_equal(p + 1, "add ", 4); + p = strchr(p + 1, '\n'); + assert_non_null(p); p = strchr(p + 1, '\n'); assert_non_null(p); assert_in_range(p, 0, buf + sizeof(buf) - 3); @@ -422,8 +420,6 @@ ISC_RUN_TEST_IMPL(isc_mem_traceflag) { p = strchr(p + 1, '\n'); assert_non_null(p); assert_memory_equal(p + 1, "del ", 4); - - isc_mem_debugging = ISC_MEM_DEBUGRECORD; } #endif /* if ISC_MEM_TRACKLINES */ @@ -502,9 +498,6 @@ ISC_TEST_ENTRY(isc_mem_inuse) ISC_TEST_ENTRY(isc_mem_zeroget) ISC_TEST_ENTRY(isc_mem_reget) -#if !defined(__SANITIZE_THREAD__) -ISC_TEST_ENTRY(isc_mem_benchmark) -#endif /* __SANITIZE_THREAD__ */ #if ISC_MEM_TRACKLINES ISC_TEST_ENTRY(isc_mem_noflags) ISC_TEST_ENTRY(isc_mem_recordflag) @@ -515,6 +508,9 @@ ISC_TEST_ENTRY(isc_mem_recordflag) */ ISC_TEST_ENTRY(isc_mem_traceflag) #endif /* if ISC_MEM_TRACKLINES */ +#if !defined(__SANITIZE_THREAD__) +ISC_TEST_ENTRY(isc_mem_benchmark) +#endif /* __SANITIZE_THREAD__ */ ISC_TEST_LIST_END From a30e75db86f408fec1c727d6a02c054420e1ee48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 15:35:58 +0200 Subject: [PATCH 2/8] Check for working __builtin_mul_overflow() implementation Instead of using generic HAVE_BUILTIN_OVERFLOW, we need to check whether the overflow functions actually work as there was a bug in GCC that it would not detect mul overflow when compiled with `-m32` option without optimizations and the bug was fixed only for GCC 6.5+ and 7.3+/8+. For further details see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82274 --- configure.ac | 32 ++++++++++++++++++++++++++++---- lib/isc/time.c | 8 ++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/configure.ac b/configure.ac index 609f138efe..fc643e57e6 100644 --- a/configure.ac +++ b/configure.ac @@ -1220,16 +1220,40 @@ AC_LINK_IFELSE( ) # -# Check for __builtin_uadd_overflow +# Check for __builtin_*_overflow # -AC_MSG_CHECKING([compiler support for __builtin_*_overflow()]) +AC_MSG_CHECKING([compiler support for __builtin_add_overflow()]) AC_LINK_IFELSE( [AC_LANG_PROGRAM( [[#include ]], - [[return (__builtin_uadd_overflow(UINT_MAX, UINT_MAX, &(unsigned int){ 0 }));]] + [[return (__builtin_add_overflow((unsigned int)UINT_MAX, (unsigned int)UINT_MAX, &(unsigned int){ 0 }));]] )], [AC_MSG_RESULT([yes]) - AC_DEFINE([HAVE_BUILTIN_OVERFLOW], [1], [define if the compiler supports __builtin_*_overflow().]) + AC_DEFINE([HAVE_BUILTIN_ADD_OVERFLOW], [1], [define if the compiler supports __builtin_add_overflow().]) + ], + [AC_MSG_RESULT([no]) + ]) + +AC_MSG_CHECKING([compiler support for __builtin_sub_overflow()]) +AC_LINK_IFELSE( + [AC_LANG_PROGRAM( + [[#include ]], + [[return (__builtin_sub_overflow((unsigned int)0, (unsigned int)UINT_MAX, &(unsigned int){ 0 }));]] + )], + [AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_BUILTIN_SUB_OVERFLOW], [1], [define if the compiler supports __builtin_sub_overflow().]) + ], + [AC_MSG_RESULT([no]) + ]) + +AC_MSG_CHECKING([compiler support for __builtin_mul_overflow()]) +AC_LINK_IFELSE( + [AC_LANG_PROGRAM( + [[#include ]], + [[return (__builtin_mul_overflow(UINT64_C(UINT64_MAX), UINT64_C(UINT64_MAX), &(uint64_t){ 0 }));]] + )], + [AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_BUILTIN_MUL_OVERFLOW], [1], [define if the compiler supports __builtin_mul_overflow().]) ], [AC_MSG_RESULT([no]) ]) diff --git a/lib/isc/time.c b/lib/isc/time.c index 5690160aae..26077befe8 100644 --- a/lib/isc/time.c +++ b/lib/isc/time.c @@ -194,8 +194,8 @@ isc_time_add(const isc_time_t *t, const isc_interval_t *i, isc_time_t *result) { REQUIRE(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S); /* Seconds */ -#if HAVE_BUILTIN_OVERFLOW - if (__builtin_uadd_overflow(t->seconds, i->seconds, &result->seconds)) { +#if HAVE_BUILTIN_ADD_OVERFLOW + if (__builtin_add_overflow(t->seconds, i->seconds, &result->seconds)) { return (ISC_R_RANGE); } #else @@ -225,8 +225,8 @@ isc_time_subtract(const isc_time_t *t, const isc_interval_t *i, REQUIRE(t->nanoseconds < NS_PER_S && i->nanoseconds < NS_PER_S); /* Seconds */ -#if HAVE_BUILTIN_OVERFLOW - if (__builtin_usub_overflow(t->seconds, i->seconds, &result->seconds)) { +#if HAVE_BUILTIN_SUB_OVERFLOW + if (__builtin_sub_overflow(t->seconds, i->seconds, &result->seconds)) { return (ISC_R_RANGE); } #else From a32d06dd4228650d461dfb018489287d7a470eb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 13:54:33 +0200 Subject: [PATCH 3/8] Use custom isc_mem based allocator for libuv The libuv library provides a way to replace the default allocator with user supplied allocator (malloc, realloc, calloc and free). Create a memory context specifically for libuv to allow tracking the memory usage that has originated from within libuv. This requires libuv >= 1.38.0 which provides uv_library_shutdown() function that assures no more allocations will be made. --- lib/isc/include/isc/uv.h | 11 ++++++ lib/isc/lib.c | 3 ++ lib/isc/uv.c | 72 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/lib/isc/include/isc/uv.h b/lib/isc/include/isc/uv.h index 476d8010c7..8f80b65f82 100644 --- a/lib/isc/include/isc/uv.h +++ b/lib/isc/include/isc/uv.h @@ -107,3 +107,14 @@ isc__uverr2result(int uverr, bool dolog, const char *file, unsigned int line, }) #endif + +/* + * Internal + */ + +void +isc__uv_initialize(void); +void +isc__uv_shutdown(void); +void +isc__uv_setdestroycheck(bool check); diff --git a/lib/isc/lib.c b/lib/isc/lib.c index dd42ea792b..5a9515dd8d 100644 --- a/lib/isc/lib.c +++ b/lib/isc/lib.c @@ -18,6 +18,7 @@ #include #include #include +#include #include "config.h" #include "mem_p.h" @@ -46,11 +47,13 @@ isc__initialize(void) { isc__mem_initialize(); isc__tls_initialize(); isc__trampoline_initialize(); + isc__uv_initialize(); (void)isc_os_ncpus(); } void isc__shutdown(void) { + isc__uv_shutdown(); isc__trampoline_shutdown(); isc__tls_shutdown(); isc__mem_shutdown(); diff --git a/lib/isc/uv.c b/lib/isc/uv.c index 62c34415a5..deb5252874 100644 --- a/lib/isc/uv.c +++ b/lib/isc/uv.c @@ -13,6 +13,7 @@ #include +#include #include #include @@ -97,3 +98,74 @@ isc__uverr2result(int uverr, bool dolog, const char *file, unsigned int line, return (ISC_R_UNEXPECTED); } } + +#if UV_VERSION_HEX >= UV_VERSION(1, 38, 0) +static isc_mem_t *isc__uv_mctx = NULL; + +static void * +isc__uv_malloc(size_t size) { + return (isc_mem_allocate(isc__uv_mctx, size)); +} + +static void * +isc__uv_realloc(void *ptr, size_t size) { + return (isc_mem_reallocate(isc__uv_mctx, ptr, size)); +} + +static void * +isc__uv_calloc(size_t count, size_t size) { + void *ptr; + size_t res; +#if HAVE_BUILTIN_MUL_OVERFLOW + bool overflow = __builtin_mul_overflow(count, size, &res); + RUNTIME_CHECK(!overflow); +#else + res = count * size; + REQUIRE(count == 0 || res / count == size); +#endif + + ptr = isc_mem_allocate(isc__uv_mctx, res); + memset(ptr, 0, res); + + return (ptr); +} + +static void +isc__uv_free(void *ptr) { + if (ptr == NULL) { + return; + } + isc_mem_free(isc__uv_mctx, ptr); +} +#endif /* UV_VERSION_HEX >= UV_VERSION(1, 38, 0) */ + +void +isc__uv_initialize(void) { +#if UV_VERSION_HEX >= UV_VERSION(1, 38, 0) + int r; + isc_mem_create(&isc__uv_mctx); + isc_mem_setname(isc__uv_mctx, "uv"); + isc_mem_setdestroycheck(isc__uv_mctx, false); + + r = uv_replace_allocator(isc__uv_malloc, isc__uv_realloc, + isc__uv_calloc, isc__uv_free); + UV_RUNTIME_CHECK(uv_replace_allocator, r); +#endif /* UV_VERSION_HEX >= UV_VERSION(1, 38, 0) */ +} + +void +isc__uv_shutdown(void) { +#if UV_VERSION_HEX >= UV_VERSION(1, 38, 0) + uv_library_shutdown(); + isc_mem_destroy(&isc__uv_mctx); +#endif /* UV_VERSION_HEX < UV_VERSION(1, 38, 0) */ +} + +void +isc__uv_setdestroycheck(bool check) { +#if UV_VERSION_HEX >= UV_VERSION(1, 38, 0) + isc_mem_setdestroycheck(isc__uv_mctx, check); +#else + UNUSED(check); +#endif /* UV_VERSION_HEX >= UV_VERSION(1, 6, 0) */ +} From 236d4b773965ca5485ed300b26ddac68c53f3abc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 14:17:48 +0200 Subject: [PATCH 4/8] Use custom isc_mem based allocator for OpenSSL The OpenSSL library provides a way to replace the default allocator with user supplied allocator (malloc, realloc, and free). Create a memory context specifically for OpenSSL to allow tracking the memory usage that has originated from within OpenSSL. This will provide a separate memory context for OpenSSL to track the allocations and when shutting down the application it will check that all OpenSSL allocations were returned to the allocator. --- lib/isc/Makefile.am | 1 - lib/isc/include/isc/tls.h | 9 +++ lib/isc/lib.c | 1 - lib/isc/tls.c | 151 ++++++++++++++++++++++++++++---------- lib/isc/tls_p.h | 20 ----- 5 files changed, 121 insertions(+), 61 deletions(-) delete mode 100644 lib/isc/tls_p.h diff --git a/lib/isc/Makefile.am b/lib/isc/Makefile.am index 35657e889e..19a1600cbe 100644 --- a/lib/isc/Makefile.am +++ b/lib/isc/Makefile.am @@ -204,7 +204,6 @@ libisc_la_SOURCES = \ time.c \ timer.c \ tls.c \ - tls_p.h \ tm.c \ trampoline.c \ trampoline_p.h \ diff --git a/lib/isc/include/isc/tls.h b/lib/isc/include/isc/tls.h index 10893a898a..26c684d6a2 100644 --- a/lib/isc/include/isc/tls.h +++ b/lib/isc/include/isc/tls.h @@ -562,3 +562,12 @@ isc_tlsctx_cache_find( * 'pstore' still might get initialised as there is one to many * relation between stores and contexts. */ + +void +isc__tls_initialize(void); + +void +isc__tls_shutdown(void); + +void +isc__tls_setdestroycheck(bool check); diff --git a/lib/isc/lib.c b/lib/isc/lib.c index 5a9515dd8d..a815f50ea4 100644 --- a/lib/isc/lib.c +++ b/lib/isc/lib.c @@ -24,7 +24,6 @@ #include "mem_p.h" #include "mutex_p.h" #include "os_p.h" -#include "tls_p.h" #include "trampoline_p.h" #ifndef ISC_CONSTRUCTOR diff --git a/lib/isc/tls.c b/lib/isc/tls.c index 15576d3020..ca2788e391 100644 --- a/lib/isc/tls.c +++ b/lib/isc/tls.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -49,15 +50,11 @@ #include #include "openssl_shim.h" -#include "tls_p.h" #define COMMON_SSL_OPTIONS \ (SSL_OP_NO_COMPRESSION | SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION) -static isc_once_t init_once = ISC_ONCE_INIT; -static isc_once_t shut_once = ISC_ONCE_INIT; -static atomic_bool init_done = false; -static atomic_bool shut_done = false; +static isc_mem_t *isc__tls_mctx = NULL; #if OPENSSL_VERSION_NUMBER < 0x10100000L static isc_mutex_t *locks = NULL; @@ -80,26 +77,116 @@ isc__tls_set_thread_id(CRYPTO_THREADID *id) { } #endif +#if !defined(LIBRESSL_VERSION_NUMBER) +/* + * This was crippled with LibreSSL, so just skip it: + * https://cvsweb.openbsd.org/src/lib/libcrypto/Attic/mem.c + */ + +#if ISC_MEM_TRACKLINES +/* + * We use the internal isc__mem API here, so we can pass the file and line + * arguments passed from OpenSSL >= 1.1.0 to our memory functions for better + * tracking of the OpenSSL allocations. Without this, we would always just see + * isc__tls_{malloc,realloc,free} in the tracking output, but with this in place + * we get to see the places in the OpenSSL code where the allocations happen. + */ + +static void * +isc__tls_malloc_ex(size_t size, const char *file, int line) { + return (isc__mem_allocate(isc__tls_mctx, size, file, + (unsigned int)line)); +} + +static void * +isc__tls_realloc_ex(void *ptr, size_t size, const char *file, int line) { + return (isc__mem_reallocate(isc__tls_mctx, ptr, size, file, + (unsigned int)line)); +} + static void -tls_initialize(void) { - REQUIRE(!atomic_load(&init_done)); +isc__tls_free_ex(void *ptr, const char *file, int line) { + if (ptr == NULL) { + return; + } + isc__mem_free(isc__tls_mctx, ptr, file, (unsigned int)line); +} + +#elif OPENSSL_VERSION_NUMBER >= 0x10100000L + +static void * +isc__tls_malloc_ex(size_t size, const char *file, int line) { + UNUSED(file); + UNUSED(line); + return (isc_mem_allocate(isc__tls_mctx, size)); +} + +static void * +isc__tls_realloc_ex(void *ptr, size_t size, const char *file, int line) { + UNUSED(file); + UNUSED(line); + return (isc_mem_reallocate(isc__tls_mctx, ptr, size)); +} + +static void +isc__tls_free_ex(void *ptr, const char *file, int line) { + UNUSED(file); + UNUSED(line); + if (ptr == NULL) { + return; + } + isc__mem_free(isc__tls_mctx, ptr); +} + +#endif /* ISC_MEM_TRACKLINES */ + +#if OPENSSL_VERSION_NUMBER < 0x10100000L +static void +isc__tls_free(void *ptr) { + isc__tls_free_ex(ptr, __FILE__, __LINE__); +} + +#endif + +#endif /* !defined(LIBRESSL_VERSION_NUMBER) */ + +void +isc__tls_initialize(void) { + isc_mem_create(&isc__tls_mctx); + isc_mem_setname(isc__tls_mctx, "OpenSSL"); + isc_mem_setdestroycheck(isc__tls_mctx, false); + +#if !defined(LIBRESSL_VERSION_NUMBER) + /* + * CRYPTO_set_mem_(_ex)_functions() returns 1 on success or 0 on + * failure, which means OpenSSL already allocated some memory. There's + * nothing we can do about it. + */ +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + (void)CRYPTO_set_mem_functions(isc__tls_malloc_ex, isc__tls_realloc_ex, + isc__tls_free_ex); +#else + (void)CRYPTO_set_mem_ex_functions(isc__tls_malloc_ex, + isc__tls_realloc_ex, isc__tls_free); +#endif +#endif /* !defined(LIBRESSL_VERSION_NUMBER) */ #if OPENSSL_VERSION_NUMBER >= 0x10100000L - RUNTIME_CHECK(OPENSSL_init_ssl(OPENSSL_INIT_ENGINE_ALL_BUILTIN | - OPENSSL_INIT_LOAD_CONFIG, - NULL) == 1); + uint64_t opts = OPENSSL_INIT_ENGINE_ALL_BUILTIN | + OPENSSL_INIT_LOAD_CONFIG; +#if defined(OPENSSL_INIT_NO_ATEXIT) + /* + * We call OPENSSL_cleanup() manually, in a correct order, thus disable + * the automatic atexit() handler. + */ + opts |= OPENSSL_INIT_NO_ATEXIT; +#endif + + RUNTIME_CHECK(OPENSSL_init_ssl(opts, NULL) == 1); #else nlocks = CRYPTO_num_locks(); - /* - * We can't use isc_mem API here, because it's called too - * early and when the isc_mem_debugging flags are changed - * later. - * - * Actually, since this is a single allocation at library load - * and deallocation at library unload, using the standard - * allocator without the tracking is fine for this purpose. - */ - locks = calloc(nlocks, sizeof(locks[0])); + locks = isc_mem_get(isc__tls_mctx, nlocks * sizeof(locks[0])); + memset(locks, 0, nlocks * sizeof(locks[0])); isc_mutexblock_init(locks, nlocks); CRYPTO_set_locking_callback(isc__tls_lock_callback); CRYPTO_THREADID_set_callback(isc__tls_set_thread_id); @@ -127,22 +214,10 @@ tls_initialize(void) { "cannot be initialized (see the `PRNG not " "seeded' message in the OpenSSL FAQ)"); } - - atomic_compare_exchange_enforced(&init_done, &(bool){ false }, true); } void -isc__tls_initialize(void) { - isc_result_t result = isc_once_do(&init_once, tls_initialize); - REQUIRE(result == ISC_R_SUCCESS); - REQUIRE(atomic_load(&init_done)); -} - -static void -tls_shutdown(void) { - REQUIRE(atomic_load(&init_done)); - REQUIRE(!atomic_load(&shut_done)); - +isc__tls_shutdown(void) { #if OPENSSL_VERSION_NUMBER >= 0x10100000L OPENSSL_cleanup(); #else @@ -161,19 +236,17 @@ tls_shutdown(void) { if (locks != NULL) { isc_mutexblock_destroy(locks, nlocks); - free(locks); + isc_mem_put(isc__tls_mctx, locks, nlocks * sizeof(locks[0])); locks = NULL; } #endif - atomic_compare_exchange_enforced(&shut_done, &(bool){ false }, true); + isc_mem_destroy(&isc__tls_mctx); } void -isc__tls_shutdown(void) { - isc_result_t result = isc_once_do(&shut_once, tls_shutdown); - REQUIRE(result == ISC_R_SUCCESS); - REQUIRE(atomic_load(&shut_done)); +isc__tls_setdestroycheck(bool check) { + isc_mem_setdestroycheck(isc__tls_mctx, check); } void diff --git a/lib/isc/tls_p.h b/lib/isc/tls_p.h deleted file mode 100644 index c0bd6937ce..0000000000 --- a/lib/isc/tls_p.h +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright (C) Internet Systems Consortium, Inc. ("ISC") - * - * SPDX-License-Identifier: MPL-2.0 - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * See the COPYRIGHT file distributed with this work for additional - * information regarding copyright ownership. - */ - -#pragma once - -void -isc__tls_initialize(void); - -void -isc__tls_shutdown(void); From e537fea86103aab02c52423cc1df76bfe7b039e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 14:36:54 +0200 Subject: [PATCH 5/8] Use custom isc_mem based allocator for libxml2 The libxml2 library provides a way to replace the default allocator with user supplied allocator (malloc, realloc, strdup and free). Create a memory context specifically for libxml2 to allow tracking the memory usage that has originated from within libxml2. This will provide a separate memory context for libxml2 to track the allocations and when shutting down the application it will check that all libxml2 allocations were returned to the allocator. Additionally, move the xmlInitParser() and xmlCleanupParser() calls from bin/named/main.c to library constructor/destructor in libisc library. --- bin/named/main.c | 9 +---- lib/isc/Makefile.am | 2 + lib/isc/include/isc/xml.h | 25 +++++++++++++ lib/isc/lib.c | 3 ++ lib/isc/xml.c | 79 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 8 deletions(-) create mode 100644 lib/isc/include/isc/xml.h create mode 100644 lib/isc/xml.c diff --git a/bin/named/main.c b/bin/named/main.c index 541564b98f..3eba68479d 100644 --- a/bin/named/main.c +++ b/bin/named/main.c @@ -44,6 +44,7 @@ #include #include #include +#include #include #include @@ -1471,10 +1472,6 @@ main(int argc, char *argv[]) { (void)ProfilerStart(NULL); #endif /* ifdef HAVE_GPERFTOOLS_PROFILER */ -#ifdef HAVE_LIBXML2 - xmlInitParser(); -#endif /* HAVE_LIBXML2 */ - /* * Technically, this call is superfluous because on startup of the main * program, the portable "C" locale is selected by default. This @@ -1593,10 +1590,6 @@ main(int argc, char *argv[]) { named_os_shutdown(); -#ifdef HAVE_LIBXML2 - xmlCleanupParser(); -#endif /* HAVE_LIBXML2 */ - #ifdef HAVE_GPERFTOOLS_PROFILER ProfilerStop(); #endif /* ifdef HAVE_GPERFTOOLS_PROFILER */ diff --git a/lib/isc/Makefile.am b/lib/isc/Makefile.am index 19a1600cbe..8e1b71f98e 100644 --- a/lib/isc/Makefile.am +++ b/lib/isc/Makefile.am @@ -105,6 +105,7 @@ libisc_la_HEADERS = \ include/isc/utf8.h \ include/isc/util.h \ include/isc/uv.h \ + include/isc/xml.h \ include/isc/work.h libisc_la_SOURCES = \ @@ -210,6 +211,7 @@ libisc_la_SOURCES = \ url.c \ utf8.c \ uv.c \ + xml.c \ work.c libisc_la_CPPFLAGS = \ diff --git a/lib/isc/include/isc/xml.h b/lib/isc/include/isc/xml.h new file mode 100644 index 0000000000..e4af80172f --- /dev/null +++ b/lib/isc/include/isc/xml.h @@ -0,0 +1,25 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#pragma once + +#include + +void +isc__xml_initialize(void); + +void +isc__xml_shutdown(void); + +void +isc__xml_setdestroycheck(bool check); diff --git a/lib/isc/lib.c b/lib/isc/lib.c index a815f50ea4..a04352622c 100644 --- a/lib/isc/lib.c +++ b/lib/isc/lib.c @@ -19,6 +19,7 @@ #include #include #include +#include #include "config.h" #include "mem_p.h" @@ -47,11 +48,13 @@ isc__initialize(void) { isc__tls_initialize(); isc__trampoline_initialize(); isc__uv_initialize(); + isc__xml_initialize(); (void)isc_os_ncpus(); } void isc__shutdown(void) { + isc__xml_shutdown(); isc__uv_shutdown(); isc__trampoline_shutdown(); isc__tls_shutdown(); diff --git a/lib/isc/xml.c b/lib/isc/xml.c new file mode 100644 index 0000000000..e7fbb19cdd --- /dev/null +++ b/lib/isc/xml.c @@ -0,0 +1,79 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#include +#include +#include + +#ifdef HAVE_LIBXML2 +#include +#include + +static isc_mem_t *isc__xml_mctx = NULL; + +static void * +isc__xml_malloc(size_t size) { + return (isc_mem_allocate(isc__xml_mctx, size)); +} + +static void * +isc__xml_realloc(void *ptr, size_t size) { + return (isc_mem_reallocate(isc__xml_mctx, ptr, size)); +} + +static char * +isc__xml_strdup(const char *str) { + return (isc_mem_strdup(isc__xml_mctx, str)); +} + +static void +isc__xml_free(void *ptr) { + if (ptr == NULL) { + return; + } + isc_mem_free(isc__xml_mctx, ptr); +} + +#endif /* HAVE_LIBXML2 */ + +void +isc__xml_initialize(void) { +#ifdef HAVE_LIBXML2 + isc_mem_create(&isc__xml_mctx); + isc_mem_setname(isc__xml_mctx, "libxml2"); + isc_mem_setdestroycheck(isc__xml_mctx, false); + + RUNTIME_CHECK(xmlGcMemSetup(isc__xml_free, isc__xml_malloc, + isc__xml_malloc, isc__xml_realloc, + isc__xml_strdup) == 0); + + xmlInitParser(); +#endif /* HAVE_LIBXML2 */ +} + +void +isc__xml_shutdown(void) { +#ifdef HAVE_LIBXML2 + xmlCleanupParser(); + isc_mem_destroy(&isc__xml_mctx); +#endif /* HAVE_LIBXML2 */ +} + +void +isc__xml_setdestroycheck(bool check) { +#ifdef HAVE_LIBXML2 + isc_mem_setdestroycheck(isc__xml_mctx, check); +#else + UNUSED(check); +#endif +} From d1cc847ab072df88e7bfc51e5b955fad00011ffc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 16:06:42 +0200 Subject: [PATCH 6/8] Check the libuv, OpenSSL and libxml2 memory context on exit As we can't check the deallocations done in the library memory contexts by default because it would always fail on non-clean exit (that happens on error or by calling exit() early), we just want to enable the checks to be done on normal exit. --- bin/dig/Makefile.am | 3 ++- bin/dig/dighost.c | 9 +++++++++ bin/named/main.c | 4 ++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/bin/dig/Makefile.am b/bin/dig/Makefile.am index 32dea5bbfd..b36f0f4525 100644 --- a/bin/dig/Makefile.am +++ b/bin/dig/Makefile.am @@ -6,7 +6,8 @@ AM_CPPFLAGS += \ $(LIBISCCFG_CFLAGS) \ $(LIBIRS_CFLAGS) \ $(LIBBIND9_CFLAGS) \ - $(LIBIDN2_CFLAGS) + $(LIBIDN2_CFLAGS) \ + $(LIBUV_CFLAGS) LDADD += \ libdighost.la \ diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index 519c7ef0b1..a8ecf3f55f 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -54,8 +54,11 @@ #include #include #include +#include #include #include +#include +#include #include #include @@ -4701,6 +4704,12 @@ destroy_libs(void) { } isc_managers_destroy(&mctx, &loopmgr, &netmgr, &taskmgr); + + isc__tls_setdestroycheck(true); + isc__uv_setdestroycheck(true); + isc__xml_setdestroycheck(true); + + isc_mem_checkdestroyed(stderr); } #ifdef HAVE_LIBIDN2 diff --git a/bin/named/main.c b/bin/named/main.c index 3eba68479d..236c55d942 100644 --- a/bin/named/main.c +++ b/bin/named/main.c @@ -1584,6 +1584,10 @@ main(int argc, char *argv[]) { &named_g_taskmgr); isc_mem_checkdestroyed(stderr); + isc__tls_setdestroycheck(true); + isc__uv_setdestroycheck(true); + isc__xml_setdestroycheck(true); + named_main_setmemstats(NULL); named_os_closedevnull(); From 3b31f7f5630578f2158e8e1f09147332fba6cd72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 27 Sep 2022 13:35:26 +0200 Subject: [PATCH 7/8] 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. --- .gitlab-ci.yml | 5 +++-- bin/dig/dighost.c | 2 ++ bin/named/main.c | 5 ++++- configure.ac | 10 ++++++++++ doc/dev/dev.md | 13 +++++++++++++ lib/isc/xml.c | 2 +- 6 files changed, 33 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index c2b5986df2..3d7e9eab3a 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 a8ecf3f55f..827461cb76 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 236c55d942..0f3896e00b 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 fc643e57e6..037b10a0b6 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 be26ba6a33..3ee7c3c937 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. + #### 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 e7fbb19cdd..9b5e52c818 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); From be3a159b548c5be194bf8e351f5474a711673bc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 23 Sep 2022 14:42:35 +0200 Subject: [PATCH 8/8] Add CHANGES note for [GL #3559] --- CHANGES | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index b6d9b2c5e3..10bf4b476f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,11 @@ +5987. [func] Provide custom isc_mem based allocators for libuv, + OpenSSL and libxml2 libraries that support replacing + the internal allocators. [GL #3559] + +5986. [func] Make the memory context debugging options local to + the memory context and make it immutable for the memory + context lifetime. [GL #3559] + 5985. [func] Bump the minimal libuv version to 1.34.0. [GL #3567] 5984. [func] 'named -V' now reports the list of supported