Re: [PATCH] Restore mempools functionality for MemBlob

On Thu, Mar 31, 2011 at 2:27 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 03/30/2011 05:58 PM, Kinkie wrote:
>> +    // if pools are not yet ready, make sure that
>> +    // the requested size is not poolable
>> +    if (!MemIsInitialized)
>> +        net_size=1+StrPoolsAttrs[mem_str_pool_count-1].obj_size;
>> +
>
> Depending on the timing of the memAllocString() call and other factors,
> net_size may become 1 after the above. Under other circumstances, it may
> become smaller than it was. Both outcomes would be wrong: net_size can
> only go up.

Fixed. Thanks.

>> /* free buffer allocated with memAllocString() */
>>  void
>>  memFreeString(size_t size, void *buf)
>>  {
>> -    int i;
>>      MemAllocator *pool = NULL;
>> -    assert(size && buf);
>> +    assert(buf);
>>
>> -    for (i = 0; i < mem_str_pool_count; ++i) {
>> +    for (unsigned int i = 0; i < mem_str_pool_count; ++i) {
>>          if (size <= StrPoolsAttrs[i].obj_size) {
>>              assert(size == StrPoolsAttrs[i].obj_size);
>>              pool = StrPools[i].pool;
>>              break;
>>          }
>>      }
>>
>>      memMeterDec(StrCountMeter);
>>      memMeterDel(StrVolumeMeter, size);
>>      pool ? pool->freeOne(buf) : xfree(buf);
>>  }
>
> If memFreeString() is called before StrPoolsAttrs are assigned, the
> above will work, I think, but purely by luck. The code will iterate the
> StrPoolsAttrs array in invalid state. And if the "size" is zero, the
> code will even think that it found the right pool (but that pool will be
> NULL so we get lucky again). A samilar comment applies to memAllocString().
> BTW, zero-size free and allocation should be supported, IMO, and your
> removal of the corresponding assert seems to agree with that.

I had thought of that code path, it's not by luck.
I've added a guard against uninitlaized mempools at free time. It's an
unlikely issue for the main use-case, but I agree it's better to err
on the safe side.
The guard against zero-sized allocations was removed because xcalloc
has explicit code to support them.

> And more whitespace changes.
Reverted.

New version is attached.

Thanks.

-- 
    /kinkie