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