Re: [PATCH] mem-pool: fix big allocations

[Elijah Newren <newren@gmail.com>]

On Thu, Dec 21, 2023 at 3:13 PM René Scharfe <l.s.r@web.de> 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@web.de>
> ---
>  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?