Message ID | 20220202163902.3596109-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | getaddrinfo: Fix leak with AI_ALL [BZ #28852] | expand |
* 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
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
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; }
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(-)