talloc: talloc_set_memlimit causes all reallocs to fail when used on pools. talloc_set_memlimit not enforced correctly on pools.

Jeremy Allison jra at samba.org
Fri Oct 16 19:40:55 UTC 2020


On Fri, Oct 16, 2020 at 12:36:53PM -0700, Jeremy Allison via samba-technical wrote:
> On Fri, Oct 16, 2020 at 01:59:11PM -0500, Arran Cudbard-Bell via samba-technical wrote:
> > Apologies if this is a repost.  The original message didn't make it into the archives, so I'm assuming it also didn't make it to mailing list subscribers.
> > 
> > First I'd like to thank the contributors to the Samba project.  We've been using talloc since ~2013 and it has made an immeasurably large contribution to code clarity and the general stability of FreeRADIUS.
> > 
> > -
> > 
> > Reviewing the talloc code shows provisions (and test cases) for applying memlimits to pools, unfortunately in practice, memlimit functionality on pools seems fairly broken.
> > 
> > 1. Calling talloc_set_memlimit on a pool results in a limit structure being created with limit->cur_size being set to the size of the pool + chunk header + pool header.  This seems to effectively mark the entire pool as already in use.
> 
> As suspected, this one is on me :-(.
> 
> commit 4159a78ed7eda340758e22286f16186987a20f2f
> Author: Jeremy Allison <jra at samba.org>
> Date:   Tue Aug 27 12:46:09 2013 -0700
> 
>     Change _talloc_total_mem_internal() to ignore memory allocated from a pool when calculating limit size.
>     
>     We must only count normal tallocs, or a talloc pool itself.
>     
>     Signed-off-by: Jeremy Allison <jra at samba.org>
>     Reviewed-by: Simo Sorce <idra at samba.org>
> 
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index 7b827ca0c15..1e25dfde4e1 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -1817,7 +1817,14 @@ static size_t _talloc_total_mem_internal(const void *ptr,
>                 break;
>         case TOTAL_MEM_LIMIT:
>                 if (likely(tc->name != TALLOC_MAGIC_REFERENCE)) {
> -                       total = tc->size + TC_HDR_SIZE;
> +                       /*
> +                        * Don't count memory allocated from a pool
> +                        * when calculating limits. Only count the
> +                        * pool itself.
> +                        */
> +                       if (!(tc->flags & TALLOC_FLAG_POOLMEM)) {
> +                               total = tc->size + TC_HDR_SIZE;
> +                       }
>                 }
> 
> I'm not sure talloc_set_memlimit() should actually *work*
> when applied to a pool pointer. Maybe we should just
> return an error ?

OK, looking at the tests - here seem to be the semantics
of talloc_set_memlimit() on pools:

        /* Test memlimits with pools. */
        pool = talloc_pool(NULL, 10*1024);
        torture_assert("memlimit", pool != NULL,
                "failed: alloc should not fail due to memory limit\n");
        talloc_set_memlimit(pool, 10*1024);
        for (i = 0; i < 9; i++) {
                l1 = talloc_size(pool, 1024);
                torture_assert("memlimit", l1 != NULL,
                        "failed: alloc should not fail due to memory limit\n");
        }
        /* The next alloc should fail. */
        l2 = talloc_size(pool, 1024);
        torture_assert("memlimit", l2 == NULL,
                        "failed: alloc should fail due to memory limit\n");

        /* Moving one of the children shouldn't change the limit,
           as it's still inside the pool. */
        root = talloc_new(NULL);
        talloc_steal(root, l1);
        l2 = talloc_size(pool, 1024);
        torture_assert("memlimit", l2 == NULL,
                        "failed: alloc should fail due to memory limit\n");

        talloc_free(pool);
        talloc_free(root);

A pool counts as already allocated memory, so setting
limit->cur_size to the size of the pool seems correct to
me.



More information about the samba-technical mailing list