On Thu, Dec 21, 2023 at 3:13 PM René Scharfe <l.s.r@xxxxxx> wrote:
>
> Memory pool allocations that require a new block and would fill at
> least half of it are handled specially. Before 158dfeff3d (mem-pool:
> add life cycle management functions, 2018-07-02) they used to be
> allocated outside of the pool. This patch made mem_pool_alloc() create
> a bespoke block instead, to allow releasing it when the pool gets
> discarded.
>
> Unfortunately mem_pool_alloc() returns a pointer to the start of such a
> bespoke block, i.e. to the struct mp_block at its top. When the caller
> writes to it, the management information gets corrupted. This affects
> mem_pool_discard() and -- if there are no other blocks in the pool --
> also mem_pool_alloc().
>
> Return the payload pointer of bespoke blocks, just like for smaller
> allocations, to protect the management struct.
>
> Also update next_free to mark the block as full. This is only strictly
> necessary for the first allocated block, because subsequent ones are
> inserted after the current block and never considered for further
> allocations, but it's easier to just do it in all cases.
>
> Add a basic unit test to demonstate the issue by using mem_pool_calloc()
demonstrate (missing 'r')?
> with a tiny block size, which forces the creation of a bespoke block.
> Without the mem_pool_alloc() fix it reports zeroed pointers:
>
> ok 1 - mem_pool_calloc returns 100 zeroed bytes with big block
> # check "((pool->mp_block->next_free) != (((void*)0))) == 1" failed at t/unit-tests/t-mem-pool.c:22
> # left: 0
> # right: 1
> # check "((pool->mp_block->end) != (((void*)0))) == 1" failed at t/unit-tests/t-mem-pool.c:23
> # left: 0
> # right: 1
> not ok 2 - mem_pool_calloc returns 100 zeroed bytes with tiny block
> 1..2
>
> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
> ---
> Makefile | 1 +
> mem-pool.c | 6 +++---
> t/unit-tests/t-mem-pool.c | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 3 deletions(-)
> create mode 100644 t/unit-tests/t-mem-pool.c
>
> diff --git a/Makefile b/Makefile
> index 88ba7a3c51..15990ff312 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1340,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1collisiondetection/%
> THIRD_PARTY_SOURCES += sha1dc/%
>
> UNIT_TEST_PROGRAMS += t-basic
> +UNIT_TEST_PROGRAMS += t-mem-pool
> UNIT_TEST_PROGRAMS += t-strbuf
> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
> diff --git a/mem-pool.c b/mem-pool.c
> index c34846d176..e8d976c3ee 100644
> --- a/mem-pool.c
> +++ b/mem-pool.c
> @@ -99,9 +99,9 @@ void *mem_pool_alloc(struct mem_pool *pool, size_t len)
>
> if (!p) {
> if (len >= (pool->block_alloc / 2))
> - return mem_pool_alloc_block(pool, len, pool->mp_block);
> -
> - p = mem_pool_alloc_block(pool, pool->block_alloc, NULL);
> + p = mem_pool_alloc_block(pool, len, pool->mp_block);
> + else
> + p = mem_pool_alloc_block(pool, pool->block_alloc, NULL);
> }
>
> r = p->next_free;
> diff --git a/t/unit-tests/t-mem-pool.c b/t/unit-tests/t-mem-pool.c
> new file mode 100644
> index 0000000000..2295779b0b
> --- /dev/null
> +++ b/t/unit-tests/t-mem-pool.c
> @@ -0,0 +1,34 @@
> +#include "test-lib.h"
> +#include "mem-pool.h"
> +
> +#define check_ptr(a, op, b) check_int(((a) op (b)), ==, 1)
> +
> +static void setup_static(void (*f)(struct mem_pool *), size_t block_alloc)
> +{
> + struct mem_pool pool = { .block_alloc = block_alloc };
> + f(&pool);
> + mem_pool_discard(&pool, 0);
> +}
> +
> +static void t_calloc_100(struct mem_pool *pool)
> +{
> + size_t size = 100;
> + char *buffer = mem_pool_calloc(pool, 1, size);
> + for (size_t i = 0; i < size; i++)
> + check_int(buffer[i], ==, 0);
> + if (!check_ptr(pool->mp_block, !=, NULL))
> + return;
> + check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);
> + check_ptr(pool->mp_block->next_free, !=, NULL);
> + check_ptr(pool->mp_block->end, !=, NULL);
> +}
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(setup_static(t_calloc_100, 1024 * 1024),
> + "mem_pool_calloc returns 100 zeroed bytes with big block");
> + TEST(setup_static(t_calloc_100, 1),
> + "mem_pool_calloc returns 100 zeroed bytes with tiny block");
> +
> + return test_done();
> +}
> --
> 2.43.0
Nice catch; looks good to me. Out of curiosity, how'd you find the issue?
