[Asterisk-Dev] potentially nasty bugs in string pools usage

[Luigi Rizzo]

On Fri, Jan 06, 2006 at 01:40:17PM -0600, Kevin P. Fleming wrote:
> Luigi Rizzo wrote:
> 
> > ok. but please also write down some commments on who is allowed to work
> > on the allocator and when. With malloc, you rely on malloc's implementation;
> > with a different allocator, the allocator needs to rely on some other lock,
> > but this cannot be the structure's lock because not all structs have one,
> > and even for those who do, it is not clear (and not documented) which
> > strings are protected by the lock and which are not.
> 
> I will think about this some more over the next few days; given the 
> application, it may make some sense to just document that string fields 
> are not something that you can preserve pointers to while you manipulate 
> the structure, and that you have to make copies of the fields in extreme 
> cases. I'm not sure that's the best option, but it's one way to solve it.

you are commenting the realloc problem, which is simmple to solve as
it was suggested before - don't use realloc, and if you need more
memory allocate a new block linked to the previous one.

what _I_ was addressing (I admit, i could have been more clear)
is the locking problem when two threads are trying
to use the allocator on the same structure. This doesn't affect
already allocated strings.

> > For performance, you could just vsnprintf() into a reasonably large
> > local buffer (say 1k instead of 1 byte as you do now), and then
> > copy the result. The copy is a lot cheaper than re-parsing the
> > format string and running expensive conversions e.g. from numbers
> > to digits (anyways, rerunning the snprintf involves copying as
> > well).  Probably a size of 1024 will cover 99% of the cases.
> 
> That is very true, and even 512 bytes would be enough, since the fields 
> that we are replacing were never that large to begin with.

yes. pick a number, instrument the code, we can put a 'show pool stats'
command to see how effective is the cache :)

> > BTW i don't understad why you want to use a macro for
> > ast_string_field_index_build() when you can just make it a var-adic
> > function which then calls vsnprintf as many times as needed.
> > You have to go through the var-adic function anyways.
> 
> If it was a direct function call, the caller would have to provide the 
> address of the hidden 'implementation' bits that are inside the 
> structure. The macro provides a way to pass only a pointer to the 
> structure instance, but still learn where those implementation bits are 
> located, thus hiding the implementation.

if you are really anxious about hiding, you can use the first level
macro, ast_string_field_build(), to pass as many parameters as needed
to the actual var-adic function.

> > i don't buy how it individually malloc'ed strings could use more
> > memory than a single pool. unless the malloc in your system has an
> > unreasonably large minimum chunk size. Can you explain that ?
> 
> Some of the string beings stored in my tests were quite short, and 
> malloc() on my system seems to consume 16 bytes plus the allocation 
> request. That may be due to the fact that I'm running on a 64-bit box, 
> though.

yes some malloc's are bad with short strings :)

> > I was just commenting that since some of the bugs introduced by this
> > change could be rather obscure (see the realloc one and the locking
> > issues), personally i would first try to push the new interface in
> > as many places as possible, and at a later time (or in parallel,
> > if you like) address the performance issues underneath.
> > I'd be happy with a 'safe_stringfield' mode where strings are
> > malloced individually, and a default mode where you use your
> > allocator. So when a bug comes out you try to see if it is also
> > present in the safe mode and this can help a lot in debugging.
> 
> That's certainly a good idea as well. We could even do that when 
> MALLOC_DEBUG is defined... Mark suggested adding some performance 
> metrics (viewable via a CLI command) to the string field manager when 
> MALLOC_DEBUG is on, so we can see how it is being used as well.

cheers
luigi