From aeb3d1cab3f5dfdc8f56d5d4c8623244a05cc01d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 22 Sep 2021 18:48:03 +0200 Subject: [PATCH 1/5] Add isc_mem_reget() function to realloc isc_mem_get allocations The isc_mem_get() and isc_mem_put() functions are leaving the memory allocation size tracking to the users of the API, while isc_mem_allocate() and isc_mem_free() would track the sizes internally. This allowed to have isc_mem_rellocate() to manipulate the memory allocations by the later set, but not the former set of the functions. This commit introduces isc_mem_reget(ctx, old_ptr, old_size, new_size) function that operates on the memory allocations with external size tracking completing the API. --- lib/isc/include/isc/mem.h | 7 ++++- lib/isc/mem.c | 60 ++++++++++++++++++++++++++++++++------- lib/isc/tests/mem_test.c | 36 +++++++++++++++++++++++ 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/lib/isc/include/isc/mem.h b/lib/isc/include/isc/mem.h index 0e0d24a3b1..a513890cc7 100644 --- a/lib/isc/include/isc/mem.h +++ b/lib/isc/include/isc/mem.h @@ -134,7 +134,9 @@ extern unsigned int isc_mem_defaultflags; #define ISCMEMFUNC(sfx) isc__mem_##sfx #define ISCMEMPOOLFUNC(sfx) isc__mempool_##sfx -#define isc_mem_get(c, s) ISCMEMFUNC(get)((c), (s)_ISC_MEM_FILELINE) +#define isc_mem_get(c, s) ISCMEMFUNC(get)((c), (s)_ISC_MEM_FILELINE) +#define isc_mem_reget(c, p, o, n) \ + ISCMEMFUNC(reget)((c), (p), (o), (n)_ISC_MEM_FILELINE) #define isc_mem_allocate(c, s) ISCMEMFUNC(allocate)((c), (s)_ISC_MEM_FILELINE) #define isc_mem_reallocate(c, p, s) \ ISCMEMFUNC(reallocate)((c), (p), (s)_ISC_MEM_FILELINE) @@ -483,6 +485,9 @@ void ISCMEMFUNC(free)(isc_mem_t *, void *_ISC_MEM_FLARG); ISC_ATTR_MALLOC_DEALLOCATOR_IDX(ISCMEMFUNC(put), 2) void *ISCMEMFUNC(get)(isc_mem_t *, size_t _ISC_MEM_FLARG); +ISC_ATTR_DEALLOCATOR_IDX(ISCMEMFUNC(put), 2) +void *ISCMEMFUNC(reget)(isc_mem_t *, void *, size_t, size_t _ISC_MEM_FLARG); + ISC_ATTR_MALLOC_DEALLOCATOR_IDX(ISCMEMFUNC(free), 2) void *ISCMEMFUNC(allocate)(isc_mem_t *, size_t _ISC_MEM_FLARG); diff --git a/lib/isc/mem.c b/lib/isc/mem.c index e598afce8b..c368959fa7 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -349,6 +349,24 @@ mem_put(isc_mem_t *ctx, void *mem, size_t size) { sdallocx(mem, size, 0); } +static inline void * +mem_realloc(isc_mem_t *ctx, void *old_ptr, size_t old_size, size_t new_size) { + void *new_ptr = NULL; + + new_ptr = rallocx(old_ptr, new_size, 0); + + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { + ssize_t diff_size = new_size - old_size; + void *diff_ptr = (uint8_t *)new_ptr + old_size; + if (diff_size > 0) { + /* Mnemonic for "beef". */ + memset(diff_ptr, 0xbe, diff_size); + } + } + + return (new_ptr); +} + #define stats_bucket(ctx, size) \ ((size / STATS_BUCKET_SIZE) >= STATS_BUCKETS \ ? &ctx->stats[STATS_BUCKETS] \ @@ -872,6 +890,37 @@ isc__mem_allocate(isc_mem_t *ctx, size_t size FLARG) { return (ptr); } +void * +isc__mem_reget(isc_mem_t *ctx, void *old_ptr, size_t old_size, + size_t new_size FLARG) { + void *new_ptr = NULL; + + if (ISC_UNLIKELY(old_ptr == NULL)) { + REQUIRE(old_size == 0); + new_ptr = isc__mem_get(ctx, new_size FLARG_PASS); + } else if (ISC_UNLIKELY(new_size == 0)) { + isc__mem_put(ctx, old_ptr, old_size FLARG_PASS); + } else { + DELETE_TRACE(ctx, old_ptr, old_size, file, line); + mem_putstats(ctx, old_ptr, old_size); + + new_ptr = mem_realloc(ctx, old_ptr, old_size, new_size); + + mem_getstats(ctx, new_size); + ADD_TRACE(ctx, new_ptr, new_size, file, line); + + /* + * We want to postpone the call to water in edge case + * where the realloc will exactly hit on the boundary of + * the water and we would call water twice. + */ + CALL_LO_WATER(ctx); + CALL_HI_WATER(ctx); + } + + return (new_ptr); +} + void * isc__mem_reallocate(isc_mem_t *ctx, void *old_ptr, size_t new_size FLARG) { void *new_ptr = NULL; @@ -888,16 +937,7 @@ isc__mem_reallocate(isc_mem_t *ctx, void *old_ptr, size_t new_size FLARG) { DELETE_TRACE(ctx, old_ptr, old_size, file, line); mem_putstats(ctx, old_ptr, old_size); - new_ptr = rallocx(old_ptr, new_size, 0); - - if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { - ssize_t diff_size = new_size - old_size; - void *diff_ptr = (uint8_t *)new_ptr + old_size; - if (diff_size >= 0) { - /* Mnemonic for "beef". */ - memset(diff_ptr, 0xbe, diff_size); - } - } + new_ptr = mem_realloc(ctx, old_ptr, old_size, new_size); /* Recalculate the real allocated size */ new_size = sallocx(new_ptr, 0); diff --git a/lib/isc/tests/mem_test.c b/lib/isc/tests/mem_test.c index e848200e2a..d5b46f2a1b 100644 --- a/lib/isc/tests/mem_test.c +++ b/lib/isc/tests/mem_test.c @@ -220,6 +220,40 @@ isc_mem_inuse_test(void **state) { isc_mem_destroy(&mctx2); } +#define REGET_INIT_SIZE 1024 +#define REGET_GROW_SIZE 2048 +#define REGET_SHRINK_SIZE 512 + +static void +isc_mem_reget_test(void **state) { + uint8_t *data = isc_mem_get(test_mctx, REGET_INIT_SIZE); + + UNUSED(state); + + for (size_t i = 0; i < REGET_INIT_SIZE; i++) { + data[i] = i % UINT8_MAX; + } + + data = isc_mem_reget(test_mctx, data, REGET_INIT_SIZE, REGET_GROW_SIZE); + + for (size_t i = 0; i < REGET_INIT_SIZE; i++) { + assert_int_equal(data[i], i % UINT8_MAX); + } + + for (size_t i = REGET_GROW_SIZE; i > 0; i--) { + data[i - 1] = i % UINT8_MAX; + } + + data = isc_mem_reget(test_mctx, data, REGET_GROW_SIZE, + REGET_SHRINK_SIZE); + + for (size_t i = REGET_SHRINK_SIZE; i > 0; i--) { + assert_int_equal(data[i - 1], i % UINT8_MAX); + } + + isc_mem_put(test_mctx, data, REGET_SHRINK_SIZE); +} + #if ISC_MEM_TRACKLINES /* test mem with no flags */ @@ -436,6 +470,8 @@ main(void) { _teardown), cmocka_unit_test_setup_teardown(isc_mem_inuse_test, _setup, _teardown), + cmocka_unit_test_setup_teardown(isc_mem_reget_test, _setup, + _teardown), #if !defined(__SANITIZE_THREAD__) cmocka_unit_test_setup_teardown(isc_mem_benchmark, _setup, From 4cdb3abf273992194e6a284e869239d21dd5d925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 23 Sep 2021 21:19:25 +0200 Subject: [PATCH 2/5] Return non-NULL pointer on zero-sized allocations and reallocations Previously, the zero-sized allocations would return NULL pointer and the caller had to make sure to not dereference such pointer. The C standard defines the zero-sized calls to malloc() as implementation specific and jemalloc mallocx() with zero size would be undefined behaviour. This complicated the code as it had to handle such cases in a special manner in all allocator and deallocator functions. Now, for realloc(), the situation is even more complicated. In C standard up to C11, the behavior would be implementation defined, and actually some implementation would free to orig ptr and some would not. Since C17 (via DR400) would deprecate such usage and since C23, the behaviour would be undefined. This commits changes helper mem_get(), mem_put() and mem_realloc() functions to grow the zero-allocation from 0 to sizeof(void *). This way we get a predicable behaviour that all the allocations will always return valid pointer. --- lib/isc/mem.c | 54 +++++++++++++++++++++------------------- lib/isc/tests/mem_test.c | 28 ++++++++++++++++++++- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index c368959fa7..67235bf41f 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -70,11 +70,12 @@ unsigned int isc_mem_defaultflags = ISC_MEMFLAG_DEFAULT; * Constants. */ -#define ALIGNMENT 8U /*%< must be a power of 2 */ -#define ALIGNMENT_SIZE sizeof(size_info) -#define DEBUG_TABLE_COUNT 512U -#define STATS_BUCKETS 512U -#define STATS_BUCKET_SIZE 32U +#define ZERO_ALLOCATION_SIZE sizeof(void *) +#define ALIGNMENT 8U /*%< must be a power of 2 */ +#define ALIGNMENT_SIZE sizeof(size_info) +#define DEBUG_TABLE_COUNT 512U +#define STATS_BUCKETS 512U +#define STATS_BUCKET_SIZE 32U /* * Types. @@ -323,12 +324,21 @@ unlock: } #endif /* ISC_MEM_TRACKLINES */ +#define ADJUST_ZERO_ALLOCATION_SIZE(s) \ + if (ISC_UNLIKELY(s == 0)) { \ + s = ZERO_ALLOCATION_SIZE; \ + } + /*! * Perform a malloc, doing memory filling and overrun detection as necessary. */ static inline void * mem_get(isc_mem_t *ctx, size_t size) { - char *ret = mallocx(size, 0); + char *ret = NULL; + + ADJUST_ZERO_ALLOCATION_SIZE(size); + + ret = mallocx(size, 0); if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { memset(ret, 0xbe, size); /* Mnemonic for "beef". */ @@ -343,6 +353,8 @@ mem_get(isc_mem_t *ctx, size_t size) { /* coverity[+free : arg-1] */ static inline void mem_put(isc_mem_t *ctx, void *mem, size_t size) { + ADJUST_ZERO_ALLOCATION_SIZE(size); + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { memset(mem, 0xde, size); /* Mnemonic for "dead". */ } @@ -353,6 +365,8 @@ static inline void * mem_realloc(isc_mem_t *ctx, void *old_ptr, size_t old_size, size_t new_size) { void *new_ptr = NULL; + ADJUST_ZERO_ALLOCATION_SIZE(new_size); + new_ptr = rallocx(old_ptr, new_size, 0); if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { @@ -607,7 +621,7 @@ isc__mem_putanddetach(isc_mem_t **ctxp, void *ptr, size_t size FLARG) { REQUIRE(ctxp != NULL && VALID_CONTEXT(*ctxp)); REQUIRE(ptr != NULL); - REQUIRE(size > 0); + REQUIRE(size != 0); ctx = *ctxp; *ctxp = NULL; @@ -735,9 +749,7 @@ isc__mem_get(isc_mem_t *ctx, size_t size FLARG) { REQUIRE(VALID_CONTEXT(ctx)); - if (ISC_LIKELY(size != 0)) { - ptr = mem_get(ctx, size); - } + ptr = mem_get(ctx, size); mem_getstats(ctx, size); ADD_TRACE(ctx, ptr, size, file, line); @@ -750,15 +762,11 @@ isc__mem_get(isc_mem_t *ctx, size_t size FLARG) { void isc__mem_put(isc_mem_t *ctx, void *ptr, size_t size FLARG) { REQUIRE(VALID_CONTEXT(ctx)); - REQUIRE(ISC_LIKELY(ptr != NULL && size != 0) || - ISC_UNLIKELY(ptr == NULL && size == 0)); DELETE_TRACE(ctx, ptr, size, file, line); mem_putstats(ctx, ptr, size); - if (ISC_LIKELY(ptr != NULL)) { - mem_put(ctx, ptr, size); - } + mem_put(ctx, ptr, size); CALL_LO_WATER(ctx); } @@ -875,12 +883,10 @@ isc__mem_allocate(isc_mem_t *ctx, size_t size FLARG) { REQUIRE(VALID_CONTEXT(ctx)); - if (ISC_LIKELY(size != 0)) { - ptr = mem_get(ctx, size); + ptr = mem_get(ctx, size); - /* Recalculate the real allocated size */ - size = sallocx(ptr, 0); - } + /* Recalculate the real allocated size */ + size = sallocx(ptr, 0); mem_getstats(ctx, size); ADD_TRACE(ctx, ptr, size, file, line); @@ -963,16 +969,12 @@ isc__mem_free(isc_mem_t *ctx, void *ptr FLARG) { REQUIRE(VALID_CONTEXT(ctx)); - if (ISC_LIKELY(ptr != NULL)) { - size = sallocx(ptr, 0); - } + size = sallocx(ptr, 0); DELETE_TRACE(ctx, ptr, size, file, line); mem_putstats(ctx, ptr, size); - if (ISC_LIKELY(ptr != NULL)) { - mem_put(ctx, ptr, size); - } + mem_put(ctx, ptr, size); CALL_LO_WATER(ctx); } diff --git a/lib/isc/tests/mem_test.c b/lib/isc/tests/mem_test.c index d5b46f2a1b..9e35af3fc7 100644 --- a/lib/isc/tests/mem_test.c +++ b/lib/isc/tests/mem_test.c @@ -220,21 +220,44 @@ isc_mem_inuse_test(void **state) { isc_mem_destroy(&mctx2); } +static void +isc_mem_zeroget_test(void **state) { + uint8_t *data = NULL; + UNUSED(state); + + data = isc_mem_get(test_mctx, 0); + assert_non_null(data); + isc_mem_put(test_mctx, data, 0); +} + #define REGET_INIT_SIZE 1024 #define REGET_GROW_SIZE 2048 #define REGET_SHRINK_SIZE 512 static void isc_mem_reget_test(void **state) { - uint8_t *data = isc_mem_get(test_mctx, REGET_INIT_SIZE); + uint8_t *data = NULL; UNUSED(state); + /* test that we can reget NULL */ + data = isc_mem_reget(test_mctx, NULL, 0, REGET_INIT_SIZE); + assert_non_null(data); + isc_mem_put(test_mctx, data, REGET_INIT_SIZE); + + /* test that we can re-get a zero-length allocation */ + data = isc_mem_get(test_mctx, 0); + assert_non_null(data); + + data = isc_mem_reget(test_mctx, data, 0, REGET_INIT_SIZE); + assert_non_null(data); + for (size_t i = 0; i < REGET_INIT_SIZE; i++) { data[i] = i % UINT8_MAX; } data = isc_mem_reget(test_mctx, data, REGET_INIT_SIZE, REGET_GROW_SIZE); + assert_non_null(data); for (size_t i = 0; i < REGET_INIT_SIZE; i++) { assert_int_equal(data[i], i % UINT8_MAX); @@ -246,6 +269,7 @@ isc_mem_reget_test(void **state) { data = isc_mem_reget(test_mctx, data, REGET_GROW_SIZE, REGET_SHRINK_SIZE); + assert_non_null(data); for (size_t i = REGET_SHRINK_SIZE; i > 0; i--) { assert_int_equal(data[i - 1], i % UINT8_MAX); @@ -470,6 +494,8 @@ main(void) { _teardown), cmocka_unit_test_setup_teardown(isc_mem_inuse_test, _setup, _teardown), + cmocka_unit_test_setup_teardown(isc_mem_zeroget_test, _setup, + _teardown), cmocka_unit_test_setup_teardown(isc_mem_reget_test, _setup, _teardown), From 15d6249260bc7e9ded7890d95a31c568e7dac586 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 22 Sep 2021 20:01:20 +0200 Subject: [PATCH 3/5] Use isc_mem_reget() when growing buffer dynamically Previously, we cannot use isc_mem_reallocate() for growing the buffer dynamically, because the memory was allocated using the isc_mem_get()/isc_mem_put() API. With the introduction of the isc_mem_reget() function, we can use grow/shrink the memory directly without always moving the memory around as the allocator might have reserved some extra space after the initial allocation. --- lib/isc/buffer.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/lib/isc/buffer.c b/lib/isc/buffer.c index 1db0eb026e..05bf4143b4 100644 --- a/lib/isc/buffer.c +++ b/lib/isc/buffer.c @@ -553,8 +553,7 @@ isc_buffer_allocate(isc_mem_t *mctx, isc_buffer_t **dynbuffer, isc_result_t isc_buffer_reserve(isc_buffer_t **dynbuffer, unsigned int size) { - unsigned char *bdata; - uint64_t len; + size_t len; REQUIRE(dynbuffer != NULL); REQUIRE(ISC_BUFFER_VALID(*dynbuffer)); @@ -581,18 +580,9 @@ isc_buffer_reserve(isc_buffer_t **dynbuffer, unsigned int size) { return (ISC_R_NOMEMORY); } - /* - * XXXMUKS: This is far more expensive than plain realloc() as - * it doesn't remap pages, but does ordinary copy. So is - * isc_mem_reallocate(), which has additional issues. - */ - bdata = isc_mem_get((*dynbuffer)->mctx, (unsigned int)len); - - memmove(bdata, (*dynbuffer)->base, (*dynbuffer)->length); - isc_mem_put((*dynbuffer)->mctx, (*dynbuffer)->base, - (*dynbuffer)->length); - - (*dynbuffer)->base = bdata; + (*dynbuffer)->base = isc_mem_reget((*dynbuffer)->mctx, + (*dynbuffer)->base, + (*dynbuffer)->length, len); (*dynbuffer)->length = (unsigned int)len; return (ISC_R_SUCCESS); From 8edbd0929fa4e28139798a50950efc2dac491d0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 22 Sep 2021 20:03:43 +0200 Subject: [PATCH 4/5] Use isc_mem_reget() to handle the internal active handle cache The netmgr, has an internal cache for freed active handles. This cache was allocated using isc_mem_allocate()/isc_mem_free() API because it was simpler to reallocate the cache when we needed to grow it. The new isc_mem_reget() function could be used here reducing the need to use isc_mem_allocate() API which is tad bit slower than isc_mem_get() API. --- lib/isc/netmgr/netmgr.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index cd051ee517..5b1ff7cdb8 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -1258,8 +1258,10 @@ nmsocket_cleanup(isc_nmsocket_t *sock, bool dofree FLARG) { isc_astack_destroy(sock->inactivereqs); sock->magic = 0; - isc_mem_free(sock->mgr->mctx, sock->ah_frees); - isc_mem_free(sock->mgr->mctx, sock->ah_handles); + isc_mem_put(sock->mgr->mctx, sock->ah_frees, + sock->ah_size * sizeof(sock->ah_frees[0])); + isc_mem_put(sock->mgr->mctx, sock->ah_handles, + sock->ah_size * sizeof(sock->ah_handles[0])); isc_mutex_destroy(&sock->lock); isc_condition_destroy(&sock->scond); #if HAVE_LIBNGHTTP2 @@ -1471,9 +1473,9 @@ isc___nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type, isc_nm_attach(mgr, &sock->mgr); sock->uv_handle.handle.data = sock; - sock->ah_frees = isc_mem_allocate( - mgr->mctx, sock->ah_size * sizeof(sock->ah_frees[0])); - sock->ah_handles = isc_mem_allocate( + sock->ah_frees = isc_mem_get(mgr->mctx, + sock->ah_size * sizeof(sock->ah_frees[0])); + sock->ah_handles = isc_mem_get( mgr->mctx, sock->ah_size * sizeof(sock->ah_handles[0])); ISC_LINK_INIT(&sock->quotacb, link); for (size_t i = 0; i < 32; i++) { @@ -1638,12 +1640,14 @@ isc___nmhandle_get(isc_nmsocket_t *sock, isc_sockaddr_t *peer, LOCK(&sock->lock); /* We need to add this handle to the list of active handles */ if ((size_t)atomic_load(&sock->ah) == sock->ah_size) { - sock->ah_frees = - isc_mem_reallocate(sock->mgr->mctx, sock->ah_frees, - sock->ah_size * 2 * sizeof(size_t)); - sock->ah_handles = isc_mem_reallocate( + sock->ah_frees = isc_mem_reget( + sock->mgr->mctx, sock->ah_frees, + sock->ah_size * sizeof(sock->ah_frees[0]), + sock->ah_size * 2 * sizeof(sock->ah_frees[0])); + sock->ah_handles = isc_mem_reget( sock->mgr->mctx, sock->ah_handles, - sock->ah_size * 2 * sizeof(isc_nmhandle_t *)); + sock->ah_size * sizeof(sock->ah_handles[0]), + sock->ah_size * 2 * sizeof(sock->ah_handles[0])); for (size_t i = sock->ah_size; i < sock->ah_size * 2; i++) { sock->ah_frees[i] = i; From d72d0149b0f34e39130ebd14ba1b968b2435fbb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 23 Sep 2021 10:24:01 +0200 Subject: [PATCH 5/5] Add CHANGES note for [GL !5440] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index f527025ed9..a777a0500e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5721. [func] New isc_mem_reget() realloc-like function was + introduced into the libisc API, and zero-sized + allocations now return non-NULL pointers. [GL !5440] + 5720. [contrib] Remove old-style DLZ drivers that had to be enabled during compile time. [GL #2814]