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 03:53:08 UTC 2020
On Mon, Oct 19, 2020 at 07:02:46PM -0700, Jeremy Allison via samba-technical wrote:
> 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.
Slightly better version that does the size accounting
correctly in the 'can't allocate from existing pool'
case.
-------------- next part --------------
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index e476f3e2d05..2e7a803d995 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,24 @@ _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)) {
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = malloc(TC_HDR_SIZE+size);
malloced = true;
new_size = size;
@@ -1917,6 +1928,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 > old_size)) {
+ if (!talloc_memlimit_check(tc->limit,
+ (size - old_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 +2042,24 @@ _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)) {
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = malloc(TC_HDR_SIZE+size);
malloced = true;
new_size = size;
@@ -2035,6 +2075,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 > old_size)) {
+ if (!talloc_memlimit_check(tc->limit,
+ (size - old_size))) {
+ errno = ENOMEM;
+ return NULL;
+ }
+ }
new_ptr = realloc(tc, size + TC_HDR_SIZE);
}
got_new_ptr:
More information about the samba-technical
mailing list