Re: [Patch] Fix another unbound alloca (BZ 13761)

[Jeff Law <law at redhat dot com>]

On 06/22/2012 04:14 PM, Roland McGrath wrote:
> You've added a "free (dataset);" call, but DATASET is never malloc'd.
> If it's not alloca'd, it's from mempool_alloc.
>
> I don't understand the nscd code well enough off hand to be sure it's
> appropriate to use mempool_alloc for whatever the "(he != NULL)" case
> means.  If it is, then you don't need to free it because those pools are
> GC'd--so you need less change than you did, the 'alloca_used' variable is
> fine as it was.  If it's not, then you need to use malloc for the new third
> case (he != NULL && !__libc_use_alloca (...)), handle that error case
> somehow, and do yet more bookkeeping to free it only when you used malloc.
It seems to me that freeing dataset is just wrong if it's allocated via 
mempool_alloc and things in mempool_alloc are GC'd.

ISTM the safe thing to do would be something like:

     if (he == NULL)
        dataset = (struct dataset *) mempool_alloc (db, total + n, 1);
     else if (! __libc_use_alloca (alloca_used + total + n))
        dataset = (struct dataset *) malloc (...);


And just track when dataset is explicitly malloc'd, freeing dataset in 
that case and that case only.  For the code which conditionally adds the 
record to the database, do not add it in the case where the record was 
allocated via malloc (as we free it).  That's obviously safe as we only 
malloc in a subset of the cases where we used to alloca.

While it might be safe/appropriate to use mempools in the case where the 
needed space is large and transient, it's hard to prove.

Thoughts?

jeff