talloc: Other minor issues/queries

Jeremy Allison jra at samba.org
Wed Oct 21 18:37:01 UTC 2020


On Wed, Oct 21, 2020 at 01:31:39PM -0500, Arran Cudbard-Bell wrote:
> 
> Sorry yes, I should have been more specific.  It clears the the existing
> name, and sets the name to be the program text.
> 
> There's a number of places where this happens. All the *_append_* 
> functions drop the name of the chunk and set it it to be the string 
> content.
> 
> For buffers its less relevant, but for structures... Well it just seems
> like a rather unfriendly thing for a type validation system to do.
> It seems like talloc_realloc* was originally envisioned primarily as a way
> of extending arrays or buffers, and not working with flexible array 
> members in structs, or struct "compositing"
> (https://github.com/FreeRADIUS/freeradius-server/blob/master/src/lib/util/ext.c).
> 
> The cost to fix this would appear to be two bits in the flags field.
> One to record whether the name was explicitly specified, one to
> record whether the name was originally const or not.
> 
> There'd actually be some efficiency benefits to recording const vs not 
> const in talloc_free_children. talloc_free_children currently walks 
> over the list of children searching for a chunk that provides the name
> for the context.
> 
> If we knew the context had a const name, then we could avoid that 
> additional O(n) operation in the majority of cases.
> 
> I'm happy to put together patches...  Recording const vs non-const
> wouldn't seem to be that contentious.  Preserving explicitly set names/
> types seems like it'd need more discussion.

That seems reasonable to me, but I'd rather Metze also
comment on this. It is a behavior change.

> > Second, I'd like to remove the memlimit code. Can you be
> > *really* clear as to what you are using them for, and
> > can we find some other way to do this ?
> 
> We want to ensure heap allocations can't occur when a pool is full.
> 
> a. In cases where these pools are composed of mprotected pages,
>     or pages that will be mprotected.
> b. Where we want to trigger allocation of an additional "slab" of memory
>     when an existing pool is used up.  This is used where we're performing
>     configuration parsing on startup.
> 
> That's it, that's really our only use cases for memlimit. We never use 
> them on any other type of chunk.
> 
> A flag saying "don't allow heap allocations" would be absolutely fine
> for us in all cases, and much simpler to implement.

OK - that sounds *much* easier. An additional 'uint32_t flags'
argument passed into talloc_pool() and talloc_pooled_object()
with a 'CONFINE_TO_POOL' flags being the only one defined
at present would be the way to go IMHO.

This would have to be a new major rev of the library
as we'd be removing the memlimit code as well in order
to make the internals of talloc understandable again :-).

Jeremy.



More information about the samba-technical mailing list