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