diff mbox series

Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

Message ID 20230922171157.65-1-romain.geissler@amadeus.com
State Superseded
Headers
Series Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806. |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 fail Patch failed to apply

Commit Message

Romain Geissler Sept. 22, 2023, 5:11 p.m. UTC 
  This patch fixes a very recently added leak in getaddrinfo (which was
backported on release branches too).

I didn't spend much more than 5 minutes on investigating the code to end
up with this patch, so it may be wrong. Quickly testing it on my side,
it seems to work for me, but it definitely needs review from people who
actually know this part of the code ;)

Running a stripped down version of the newly added test
nss/nss_test_gai_hv2_canonname.c with valgrind results in exposure of
the leak:

> cat test.c

int main()
{
    char aHostName[256];
    gethostname(aHostName,255);

    struct addrinfo hints = {};
    struct addrinfo *result = NULL;

    hints.ai_family = AF_INET6;
    hints.ai_flags = AI_ALL | AI_V4MAPPED | AI_CANONNAME;

    int ret = getaddrinfo(aHostName, NULL, &hints, &result);

    if (ret != 0)
        return 1;
    freeaddrinfo(result);
    return 0;
}

> /opt/1A/toolchain/x86_64-v19/bin/gcc -g -o test test.c
> /opt/1A/toolchain/x86_64-v19/build-pack/default/bin/valgrind --leak-check=full ./test
   ... (snapped)
==68017== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
==68017==    at 0x4840745: malloc (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/build-pack/19.0.81.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==68017==    by 0x48E7CDA: strdup (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4936582: convert_hostent_to_gaih_addrtuple.isra.0 (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4936787: gethosts (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4938F37: getaddrinfo (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4011A5: main (test.c:17)
   ... (snapped)
---
 sysdeps/posix/getaddrinfo.c | 1 +
 1 file changed, 1 insertion(+)

Comments

DJ Delorie Sept. 22, 2023, 8:26 p.m. UTC | #1 
I think it may need an "if != NULL" check on it, in case we're exiting
due to error.  The only time h_name is set is via __strdup but that
__strdup may fail.
Romain Geissler Sept. 22, 2023, 8:31 p.m. UTC | #2 
> Le 22 sept. 2023 à 22:26, DJ Delorie <dj@redhat.com> a écrit :
> 
> 
> I think it may need an "if != NULL" check on it, in case we're exiting
> due to error.  The only time h_name is set is via __strdup but that
> __strdup may fail.
> 

Isn’t it fine to call free(NULL) (which should normally be a no-op) ? Or you suggest this to save a jump to a function for optimization reasons ?
DJ Delorie Sept. 22, 2023, 9:23 p.m. UTC | #3 
Romain GEISSLER <romain.geissler@amadeus.com> writes:
> Isn’t it fine to call free(NULL) (which should normally be a no-op) ?
> Or you suggest this to save a jump to a function for optimization
> reasons ?

Nope, I'm just showing my age again.  Of course it's fine to free(NULL)
these days ;-)

There's a bunch of conditionals around that code, but they're checking
some other flag variable.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Siddhesh Poyarekar Sept. 22, 2023, 10:21 p.m. UTC | #4 
On 2023-09-22 18:11, Romain Geissler wrote:
> This patch fixes a very recently added leak in getaddrinfo (which was
> backported on release branches too).
> 
> I didn't spend much more than 5 minutes on investigating the code to end
> up with this patch, so it may be wrong. Quickly testing it on my side,
> it seems to work for me, but it definitely needs review from people who
> actually know this part of the code ;)
> 

Nice catch, thank you for noticing.

> Running a stripped down version of the newly added test
> nss/nss_test_gai_hv2_canonname.c with valgrind results in exposure of
> the leak:
> 
>> cat test.c
> 
> int main()
> {
>      char aHostName[256];
>      gethostname(aHostName,255);
> 
>      struct addrinfo hints = {};
>      struct addrinfo *result = NULL;
> 
>      hints.ai_family = AF_INET6;
>      hints.ai_flags = AI_ALL | AI_V4MAPPED | AI_CANONNAME;
> 
>      int ret = getaddrinfo(aHostName, NULL, &hints, &result);
> 
>      if (ret != 0)
>          return 1;
>      freeaddrinfo(result);
>      return 0;
> }
> 
>> /opt/1A/toolchain/x86_64-v19/bin/gcc -g -o test test.c
>> /opt/1A/toolchain/x86_64-v19/build-pack/default/bin/valgrind --leak-check=full ./test
>     ... (snapped)
> ==68017== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
> ==68017==    at 0x4840745: malloc (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/build-pack/19.0.81.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==68017==    by 0x48E7CDA: strdup (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4936582: convert_hostent_to_gaih_addrtuple.isra.0 (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4936787: gethosts (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4938F37: getaddrinfo (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4011A5: main (test.c:17)
>     ... (snapped)
> ---
>   sysdeps/posix/getaddrinfo.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index b4e8ea3880a..5f5bc3fd51f 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -1199,6 +1199,7 @@ free_and_return:
>     if (res.free_at)
>       free (res.at);
>     free (res.canon);
> +  free (res.h_name);

Could you please consolidate all of this into a gaih_result_reset (&res) 
call?  There's an additional memset, but that should be negligible 
overhead for a cleaner abstraction.

Thanks,
Sid
Siddhesh Poyarekar Sept. 22, 2023, 10:28 p.m. UTC | #5 
On 2023-09-22 23:21, Siddhesh Poyarekar wrote:
>> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
>> index b4e8ea3880a..5f5bc3fd51f 100644
>> --- a/sysdeps/posix/getaddrinfo.c
>> +++ b/sysdeps/posix/getaddrinfo.c
>> @@ -1199,6 +1199,7 @@ free_and_return:
>>     if (res.free_at)
>>       free (res.at);
>>     free (res.canon);
>> +  free (res.h_name);
> 
> Could you please consolidate all of this into a gaih_result_reset (&res) 
> call?  There's an additional memset, but that should be negligible 
> overhead for a cleaner abstraction.
> 

Oh, and it would be fabulous if you could quote the original bug number 
(BZ #30843) in the v2 and add a leak check test case (see 
resolv/tst-leaks* for an example) but I won't block the fix on that.

Thanks!
Sid
diff mbox series

Patch

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index b4e8ea3880a..5f5bc3fd51f 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -1199,6 +1199,7 @@  free_and_return:
   if (res.free_at)
     free (res.at);
   free (res.canon);
+  free (res.h_name);
 
   return result;
 }