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

[Jeremy Allison]

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.
> 
> 2. Because of 1. calling talloc_realloc on a chunk allocated inside a pool always fails (unless the operation would be a noop).  Even if the chunk size is well within the pool limits.
> 
> 3. Chunks allocated within a pool are never checked against the memlimit set on the pool.  This is actually the only reason why allocations within a pool work at all.  If the code path included a call to talloc_memlimit_check, normal allocations would fail the same as calls to talloc_realloc.
> 
> I've tried modifying the code so that only the children of the pool ctx are included in the the cur_size calculation, but this causes a "logic error in talloc_memlimit_shrink" error.
> 
> Happy to keep poking, but I figured someone more familiar with the spirit of the code would probably be able to make more rapid progress.
> 
> I've included some simple test cases at  the bottom of this email.

Thanks. Arghhh. I really hate the talloc_memlimit code :-).

Not sure how popular/used it is.

I would love to just strip it out, but as it's part of the
ABI that's too hard. Can you log a bug at bugzilla.samba.org
so we can track this ?

Thanks,

Jeremy.