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
Tue Oct 20 02:02:46 UTC 2020


On Fri, Oct 16, 2020 at 07:52:48PM -0500, Arran Cudbard-Bell via samba-technical wrote:
> 
> >> An alternative that'd still satisfy our immediate need would be to have talloc_set_memlimit simply fail when someone tried to apply it to a pool (as you suggested), and add an optional flag that'd prevent allocations from occurring outside of the pool.
> > 
> > Please wrap your responses to 80 columns :-). Makes
> > quoting your replies really hard :-).
> 
> Will do :)
> 
> > 
> > What you're asking for is more complexity in an
> > already overly complex part of the code (which
> > to be honest I wasn't even sure people were
> > using :-).
> > 
> > I think you can do what you need by allocating
> > a pool as a talloc child of a context, and setting
> > the memlimit on the that context.
> 
> I just tried this and it didn't work, the reallocs still fail.
> 
> This is likely because the limit needs to be the size of the pool plus 
> headers.  I don't believe there's any way for the caller to know the size
> of these headers, but maybe you know better :)
> 
> talloc_get_size() returns 0 when called on the ctx or the pool as the
> docs suggest it should.
> 
> Do you have any idea how I could determine the correct value to
> pass to talloc_set_memlimit?
> 
> -Arran

I think it's a bug. If you can rebuild talloc can you
check with this (not well tested yet :-) patch ?

I'm planning to add regression tests around this.

With this patch talloc_memlimits on pools should
work.
-------------- next part --------------
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index e476f3e2d05..3fcdaefdc04 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1833,13 +1833,6 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		return NULL;
 	}
 
-	if (tc->limit && (size > tc->size)) {
-		if (!talloc_memlimit_check(tc->limit, (size - tc->size))) {
-			errno = ENOMEM;
-			return NULL;
-		}
-	}
-
 	/* handle realloc inside a talloc_pool */
 	if (unlikely(tc->flags & TALLOC_FLAG_POOLMEM)) {
 		pool_hdr = tc->pool;
@@ -1904,6 +1897,19 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		pool_hdr->object_count--;
 
 		if (new_ptr == NULL) {
+			/*
+			 * Couldn't allocate from pool (pool size
+			 * counts as already allocated for memlimit
+			 * purposes). We must check memory limit
+			 * before any real malloc.
+			 */
+			if (tc->limit && (size > tc->size)) {
+				if (!talloc_memlimit_check(tc->limit,
+						(size - tc->size))) {
+					errno = ENOMEM;
+					return NULL;
+				}
+			}
 			new_ptr = malloc(TC_HDR_SIZE+size);
 			malloced = true;
 			new_size = size;
@@ -1917,6 +1923,17 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		/* We're doing malloc then free here, so record the difference. */
 		old_size = tc->size;
 		new_size = size;
+		/*
+		 * We must check memory limit
+		 * before any real malloc.
+		 */
+		if (tc->limit && (size > tc->size)) {
+			if (!talloc_memlimit_check(tc->limit,
+					(size - tc->size))) {
+				errno = ENOMEM;
+				return NULL;
+			}
+		}
 		new_ptr = malloc(size + TC_HDR_SIZE);
 		if (new_ptr) {
 			memcpy(new_ptr, tc, MIN(tc->size, size) + TC_HDR_SIZE);
@@ -2020,6 +2037,19 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0);
 
 		if (new_ptr == NULL) {
+			/*
+			 * Couldn't allocate from pool (pool size
+			 * counts as already allocated for memlimit
+			 * purposes). We must check memory limit
+			 * before any real malloc.
+			 */
+			if (tc->limit && (size > tc->size)) {
+				if (!talloc_memlimit_check(tc->limit,
+						(size - tc->size))) {
+					errno = ENOMEM;
+					return NULL;
+				}
+			}
 			new_ptr = malloc(TC_HDR_SIZE+size);
 			malloced = true;
 			new_size = size;
@@ -2035,6 +2065,17 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
 		/* We're doing realloc here, so record the difference. */
 		old_size = tc->size;
 		new_size = size;
+		/*
+		 * We must check memory limit
+		 * before any real realloc.
+		 */
+		if (tc->limit && (size > tc->size)) {
+			if (!talloc_memlimit_check(tc->limit,
+					(size - tc->size))) {
+				errno = ENOMEM;
+				return NULL;
+			}
+		}
 		new_ptr = realloc(tc, size + TC_HDR_SIZE);
 	}
 got_new_ptr:


More information about the samba-technical mailing list