[squid-dev] [PATCH] Bug 4438 - string pool refactoring

[Alex Rousskov]

On 03/25/2016 07:08 AM, Amos Jeffries wrote:
> This was audited off-list, and a slightly more polished version applied
> to trunk as rev.14604

I have not seen the off-list audit, but the committed version is buggy IMHO:


>  MemPools &
>  MemPools::GetInstance()
>  {
>      /* Must use this idiom, as we can be double-initialised
>       * if we are called during static initialisations.
>       */
> -    if (!Instance)
> -        Instance = new MemPools;
> -    return *Instance;
> +    static MemPools Instance;
> +    return Instance;
>  }


This change solves the initialization order bug but introduces the
destruction order bug: The Instance object may be gone before its last
use. Please fix and check for similar bugs in the new code.


> +static MemAllocator *&
> +GetPool(size_t type)
> +{
> +    static MemAllocator *pools[MEM_MAX];
> +    static bool initialized = false;
> +
> +    if (!initialized) {
> +        memset(pools, '\0', sizeof(pools));
> +        initialized = true;
> +    }
> +
> +    return pools[type];
> +}

>  /* find appropriate pool and use it (pools always init buffer with 0s) */
>  void *
>  memAllocate(mem_type type)
>  {
> -    assert(MemPools[type]);
> -    return MemPools[type]->alloc();
> +    assert(GetPool(type));
> +    return GetPool(type)->alloc();
>  }

AFAICT, this combination will assert if memAllocate() (or any similar
external caller) is called "too early". Since memAllocate() is not
static, you cannot control when it is called. Please fix so that
GetPool() always returns a usable pool. Returning a _reference_ to that
pool instead of a never-nil pointer would be appropriate.


> +static MemAllocator *&
> +GetStrPool(size_t type)

Wrong return type AFAICT: This function always returns a usable pool so
it should return a reference and the callers should be adjusted to avoid
treating non-existent nil string pools specially. The latter adjustment
may expose other necessary changes.


HTH,

Alex.