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

[Kevin P. Fleming]

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.

> 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.

> 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.

> 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.

> 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.