Message ID | 20230922171157.65-1-romain.geissler@amadeus.com |
---|---|
State | New |
Headers | show |
Series | Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806. | expand |
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.
> 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 ?
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>
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
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 --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; }