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

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

[Siddhesh Poyarekar]

Avoid allocating multiple blocks in convert_hostent_to_gaih_addrtuple
because it's not possible to free the second block in the caller
gaih_inet.  Instead, use realloc and fix up pointers in the result list.

Resolves BZ #28852.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
This is for 2.36; I'll backport the fix to 2.35 after the release is
cut.

Tested check and xcheck on x86_64 and also verified using valgrind that
the reproducer in the bugzilla is fixed.  mtrace doesn't show this leak
for some reason, so the reproducer could not be adapted to a glibc test
case.

 sysdeps/posix/getaddrinfo.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

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;
 }
 
-- 
2.34.1

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

[Florian Weimer]

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

Reading the code, I can't really see if this assumption is true or
not. 8-(

Thanks,
Florian

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

[Siddhesh Poyarekar]

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