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.

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 ?