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 19:49:17 UTC 2020
On Tue, Oct 20, 2020 at 12:16:54PM -0700, Jeremy Allison via samba-technical wrote:
> On Tue, Oct 20, 2020 at 01:42:00PM -0500, Arran Cudbard-Bell wrote:
> >
> >
> > > On Oct 20, 2020, at 1:33 PM, Jeremy Allison <jra at samba.org> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 01:03:14PM -0500, Arran Cudbard-Bell wrote:
> > >>
> > >> Then there's another issue with object_count ending
> > >> up off by one, which means talloc_free_children doesn't
> > >> actually return memory to the pool, and that messes up
> > >> some of the other tests I'm adding. Just tracking down
> > >> when and why this happens now.... It might have been a
> > >> pre-existing issue and not related to this patch, I'm just
> > >> seeing it because of using talloc_free_children to reset
> > >> the pool between some tests.
> >
> > Apologies, there was a steal I didn't spot in the tests.
> > One of the chunks was moved out of the pool into the root ctx.
> >
> > Explicitly freeing the chunk or stealing it back into the pool
> > means talloc_free_children works as expected.
>
> FYI, once you've gotten everything working can you
> send your test cases to me so I can add them into
> the regression test suite for talloc ?
>
> Then we'll add a new bug in bugzilla, update the minor library
> version number and create a gitlab MR.
OK, here's what I've got so far in terms of bugfixes/cleanups.
Let me know when you're happy with your tests.
Jeremy.
-------------- next part --------------
From e192bf70a2492f5239b6ed0f4baeabb60f13e848 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 20 Oct 2020 10:52:55 -0700
Subject: [PATCH 1/3] WIP: Fix memlimit on pool realloc.
Signed-off-by: Jeremy Allison <jra at samba.org>
Signed-off-by: Arran Cudbard-Bell <a.cudbardb at freeradius.org>
---
lib/talloc/talloc.c | 69 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 62 insertions(+), 7 deletions(-)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index e476f3e2d05..accc859f77e 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,25 @@ _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) {
+ /*
+ * Note we're doing an extra malloc,
+ * on top of the pool size, so account
+ * for size only, not the difference
+ * between old and new size.
+ */
+ if (!talloc_memlimit_check(tc->limit, size)) {
+ _talloc_chunk_set_not_free(tc);
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = malloc(TC_HDR_SIZE+size);
malloced = true;
new_size = size;
@@ -1917,6 +1929,18 @@ _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 > old_size)) {
+ if (!talloc_memlimit_check(tc->limit,
+ (size - old_size))) {
+ _talloc_chunk_set_not_free(tc);
+ 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 +2044,25 @@ _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) {
+ /*
+ * Note we're doing an extra malloc,
+ * on top of the pool size, so account
+ * for size only, not the difference
+ * between old and new size.
+ */
+ if (!talloc_memlimit_check(tc->limit, size)) {
+ _talloc_chunk_set_not_free(tc);
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = malloc(TC_HDR_SIZE+size);
malloced = true;
new_size = size;
@@ -2035,6 +2078,18 @@ _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 > old_size)) {
+ if (!talloc_memlimit_check(tc->limit,
+ (size - old_size))) {
+ _talloc_chunk_set_not_free(tc);
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = realloc(tc, size + TC_HDR_SIZE);
}
got_new_ptr:
--
2.25.1
From 1706bf30739f91bb8e1f529f2acfc45bdf342173 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 20 Oct 2020 12:14:58 -0700
Subject: [PATCH 2/3] WIP: Fix pool object accounting when doing
talloc_realloc() in the ALWAYS_REALLOC compiled case.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/talloc/talloc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index accc859f77e..f28a22b799c 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1894,8 +1894,6 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
#if ALWAYS_REALLOC
if (pool_hdr) {
new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0);
- pool_hdr->object_count--;
-
if (new_ptr == NULL) {
/*
* Couldn't allocate from pool (pool size
@@ -1924,6 +1922,11 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
if (new_ptr) {
memcpy(new_ptr, tc, MIN(tc->size,size) + TC_HDR_SIZE);
TC_INVALIDATE_FULL_CHUNK(tc);
+ /*
+ * Only decrement the object count in the pool once
+ * we know we're returning a valid new_ptr.
+ */
+ pool_hdr->object_count--;
}
} else {
/* We're doing malloc then free here, so record the difference. */
--
2.25.1
From 9f849ea792d3f7e32fd354b4218fff368bee1e10 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Tue, 20 Oct 2020 12:18:10 -0700
Subject: [PATCH 3/3] lib: talloc: Cleanup. Use consistent preprocessor logic
macros.
Match other use of ALWAYS_REALLOC.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/talloc/talloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index f28a22b799c..078e58ca352 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -1891,7 +1891,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
*/
_talloc_chunk_set_free(tc, NULL);
-#if ALWAYS_REALLOC
+#if (ALWAYS_REALLOC != 0)
if (pool_hdr) {
new_ptr = tc_alloc_pool(tc, size + TC_HDR_SIZE, 0);
if (new_ptr == NULL) {
--
2.25.1
More information about the samba-technical
mailing list