Re: [PATCH] getaddrinfo: Fix leak with AI_ALL [BZ #28852]

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 04/02/2022 17:36, Florian Weimer wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
>> index 18dccd5924..652a1a43d4 100644
>> --- a/sysdeps/posix/getaddrinfo.c
>> +++ b/sysdeps/posix/getaddrinfo.c
>> @@ -199,9 +199,6 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
>>   				   struct hostent *h,
>>   				   struct gaih_addrtuple **result)
>>   {
>> -  while (*result)
>> -    result = &(*result)->next;
>> -
>>     /* Count the number of addresses in h->h_addr_list.  */
>>     size_t count = 0;
>>     for (char **p = h->h_addr_list; *p != NULL; ++p)
>> @@ -212,10 +209,30 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
>>     if (count == 0 || h->h_length > sizeof (((struct gaih_addrtuple) {}).addr))
>>       return true;
>>   
>> -  struct gaih_addrtuple *array = calloc (count, sizeof (*array));
>> +  struct gaih_addrtuple *array = *result;
>> +  size_t old = 0;
>> +
>> +  while (array)
>> +    {
>> +      old++;
>> +      array = array->next;
>> +    }
>> +
>> +  array = realloc (*result, (old + count) * sizeof (*array));
>> +
>>     if (array == NULL)
>>       return false;
>>   
>> +  *result = array;
>> +
>> +  /* Update the next pointers on reallocation.  */
>> +  for (size_t i = 0; i < old; i++)
>> +    array[i].next = array + i + 1;
>> +
>> +  array += old;
>> +
>> +  memset (array, 0, count * sizeof (*array));
>> +
>>     for (size_t i = 0; i < count; ++i)
>>       {
>>         if (family == AF_INET && req->ai_family == AF_INET6)
>> @@ -235,7 +252,6 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
>>     array[0].name = h->h_name;
>>     array[count - 1].next = NULL;
>>   
>> -  *result = array;
>>     return true;
>>   }
> 
> I think this assumes that the addrmem block (now managed by realloc) is
> always at the end of the “at” tuples list (which is not always backed by
> addrmem memory).  If that is not the case, the tail after the addrmem
> block is lost.

I couldn't find a way in which a block not backed by addrmem would 
follow these realloc'd blocks.  Every situation where a different method 
is used to allocate tuples (e.g. through gethostbyname4_r), the *pat is 
overwritten, causing older tuples to be lost.

This could happen for example if we have SUCCESS=CONTINUE in 
nsswitch.conf (is that even allowed?) and a gethostbyname[23]_r is 
followed by gethostbyname4_r.  I'm not sure if it is a situation we 
ought to support.

Does that make sense?  ISTM the whole thing could be simplified by 
dropping alloca and using malloc throughout; all this seems 
unnecessarily complicated.  Let me take a stab at that.

Thanks,
Siddhesh