Merge branch 'ondrej/introduce-isc_mem_reget' into 'main'

Add isc_mem_reget() function to realloc isc_mem_get allocations

See merge request isc-projects/bind9!5440

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]
 

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;
+	size_t len;
-	uint64_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);
 	}
 
-	/*
+	(*dynbuffer)->base = isc_mem_reget((*dynbuffer)->mctx,
-	 * XXXMUKS: This is far more expensive than plain realloc() as
+					   (*dynbuffer)->base,
-	 * it doesn't remap pages, but does ordinary copy. So is
+					   (*dynbuffer)->length, len);
-	 * 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)->length = (unsigned int)len;
 
 	return (ISC_R_SUCCESS);

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);
 

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 ZERO_ALLOCATION_SIZE sizeof(void *)
-#define ALIGNMENT_SIZE	  sizeof(size_info)
+#define ALIGNMENT	     8U /*%< must be a power of 2 */
-#define DEBUG_TABLE_COUNT 512U
+#define ALIGNMENT_SIZE	     sizeof(size_info)
-#define STATS_BUCKETS	  512U
+#define DEBUG_TABLE_COUNT    512U
-#define STATS_BUCKET_SIZE 32U
+#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,12 +353,34 @@ 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". */
 	}
 	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;
+
+	ADJUST_ZERO_ALLOCATION_SIZE(new_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);
+		}
+	}
+
+	return (new_ptr);
+}
+
 #define stats_bucket(ctx, size)                      \
 	((size / STATS_BUCKET_SIZE) >= STATS_BUCKETS \
 		 ? &ctx->stats[STATS_BUCKETS]        \
@@ -589,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;
@@ -717,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);
@@ -732,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);
 }
@@ -857,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 */
+	/* Recalculate the real allocated size */
-		size = sallocx(ptr, 0);
+	size = sallocx(ptr, 0);
-	}
 
 	mem_getstats(ctx, size);
 	ADD_TRACE(ctx, ptr, size, file, line);
@@ -872,6 +896,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 +943,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);
+		new_ptr = mem_realloc(ctx, old_ptr, old_size, new_size);
-
-		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);
-			}
-		}
 
 		/* Recalculate the real allocated size */
 		new_size = sallocx(new_ptr, 0);
@@ -923,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);
 }

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_put(sock->mgr->mctx, sock->ah_frees,
-	isc_mem_free(sock->mgr->mctx, sock->ah_handles);
+		    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(
+	sock->ah_frees = isc_mem_get(mgr->mctx,
-		mgr->mctx, sock->ah_size * sizeof(sock->ah_frees[0]));
+				     sock->ah_size * sizeof(sock->ah_frees[0]));
-	sock->ah_handles = isc_mem_allocate(
+	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 =
+		sock->ah_frees = isc_mem_reget(
-			isc_mem_reallocate(sock->mgr->mctx, sock->ah_frees,
+			sock->mgr->mctx, sock->ah_frees,
-					   sock->ah_size * 2 * sizeof(size_t));
+			sock->ah_size * sizeof(sock->ah_frees[0]),
-		sock->ah_handles = isc_mem_reallocate(
+			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;

lib/isc/tests/mem_test.c

@@ -220,6 +220,64 @@ 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 = 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);
+	}
+
+	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);
+	assert_non_null(data);
+
+	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 +494,10 @@ 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),
 
 #if !defined(__SANITIZE_THREAD__)
 		cmocka_unit_test_setup_teardown(isc_mem_benchmark, _setup,