* [PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN @ 2021-11-10 18:58 Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-10 18:58 UTC (permalink / raw) To: libc-alpha; +Cc: leonardo.macchia Adhemerval Zanella (3): inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) inet: Remove strdupa from nrl_domainname() inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory inet/getnameinfo.c | 146 +++++++++++++++++++++++---------------------- 1 file changed, 76 insertions(+), 70 deletions(-) -- 2.32.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-10 18:58 [PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella @ 2021-11-10 18:58 ` Adhemerval Zanella 2021-11-11 8:16 ` Florian Weimer 2021-11-10 18:58 ` [PATCH 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella 2 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-10 18:58 UTC (permalink / raw) To: libc-alpha; +Cc: leonardo.macchia The 'not_first' is accessed on nrl_domainname() in a non atomically way, although it is only updated after the lock is taken. Instead of being too clever, just always take the lock (another option might to use an atomic load and only take the lock if lock might be taken, but I think it would be better to have this optimization on generic lll_lock instead of ad-hoc solution). Checked on x86_64-linux-gnu. --- inet/getnameinfo.c | 120 ++++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 61 deletions(-) diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c index 8380d85783..58ebbb1154 100644 --- a/inet/getnameinfo.c +++ b/inet/getnameinfo.c @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain); static char * nrl_domainname (void) { - static int not_first; + __libc_lock_define_initialized (static, lock); + __libc_lock_lock (lock); + static bool not_first = false; if (! not_first) { - __libc_lock_define_initialized (static, lock); - __libc_lock_lock (lock); - - if (! not_first) - { - char *c; - struct hostent *h, th; - int herror; - struct scratch_buffer tmpbuf; + char *c; + struct hostent *h, th; + int herror; + struct scratch_buffer tmpbuf; - scratch_buffer_init (&tmpbuf); - not_first = 1; + scratch_buffer_init (&tmpbuf); - while (__gethostbyname_r ("localhost", &th, - tmpbuf.data, tmpbuf.length, - &h, &herror)) + while (__gethostbyname_r ("localhost", &th, + tmpbuf.data, tmpbuf.length, + &h, &herror)) + { + if (herror == NETDB_INTERNAL && errno == ERANGE) { - if (herror == NETDB_INTERNAL && errno == ERANGE) - { - if (!scratch_buffer_grow (&tmpbuf)) - goto done; - } - else - break; + if (!scratch_buffer_grow (&tmpbuf)) + goto done; } + else + break; + } - if (h && (c = strchr (h->h_name, '.'))) + if (h && (c = strchr (h->h_name, '.'))) + domain = __strdup (++c); + else + { + /* The name contains no domain information. Use the name + now to get more information. */ + while (__gethostname (tmpbuf.data, tmpbuf.length)) + if (!scratch_buffer_grow (&tmpbuf)) + goto done; + + if ((c = strchr (tmpbuf.data, '.'))) domain = __strdup (++c); else { - /* The name contains no domain information. Use the name - now to get more information. */ - while (__gethostname (tmpbuf.data, tmpbuf.length)) - if (!scratch_buffer_grow (&tmpbuf)) - goto done; + /* We need to preserve the hostname. */ + const char *hstname = strdupa (tmpbuf.data); + while (__gethostbyname_r (hstname, &th, + tmpbuf.data, + tmpbuf.length, + &h, &herror)) + { + if (herror == NETDB_INTERNAL && errno == ERANGE) + { + if (!scratch_buffer_grow_preserve (&tmpbuf)) + goto done; + } + else + break; + } - if ((c = strchr (tmpbuf.data, '.'))) + if (h && (c = strchr(h->h_name, '.'))) domain = __strdup (++c); else { - /* We need to preserve the hostname. */ - const char *hstname = strdupa (tmpbuf.data); + struct in_addr in_addr; + + in_addr.s_addr = htonl (INADDR_LOOPBACK); - while (__gethostbyname_r (hstname, &th, - tmpbuf.data, tmpbuf.length, + while (__gethostbyaddr_r ((const char *) &in_addr, + sizeof (struct in_addr), + AF_INET, &th, + tmpbuf.data, + tmpbuf.length, &h, &herror)) { if (herror == NETDB_INTERNAL && errno == ERANGE) @@ -146,41 +166,19 @@ nrl_domainname (void) break; } - if (h && (c = strchr(h->h_name, '.'))) + if (h && (c = strchr (h->h_name, '.'))) domain = __strdup (++c); - else - { - struct in_addr in_addr; - - in_addr.s_addr = htonl (INADDR_LOOPBACK); - - while (__gethostbyaddr_r ((const char *) &in_addr, - sizeof (struct in_addr), - AF_INET, &th, - tmpbuf.data, tmpbuf.length, - &h, &herror)) - { - if (herror == NETDB_INTERNAL && errno == ERANGE) - { - if (!scratch_buffer_grow (&tmpbuf)) - goto done; - } - else - break; - } - - if (h && (c = strchr (h->h_name, '.'))) - domain = __strdup (++c); - } } } - done: - scratch_buffer_free (&tmpbuf); } - __libc_lock_unlock (lock); + done: + scratch_buffer_free (&tmpbuf); + not_first = true; } + __libc_lock_unlock (lock); + return domain; }; -- 2.32.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-10 18:58 ` [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella @ 2021-11-11 8:16 ` Florian Weimer 2021-11-11 13:34 ` Adhemerval Zanella 0 siblings, 1 reply; 14+ messages in thread From: Florian Weimer @ 2021-11-11 8:16 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, leonardo.macchia * Adhemerval Zanella via Libc-alpha: > diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c > index 8380d85783..58ebbb1154 100644 > --- a/inet/getnameinfo.c > +++ b/inet/getnameinfo.c > @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain); > static char * > nrl_domainname (void) > { > + __libc_lock_define_initialized (static, lock); > + __libc_lock_lock (lock); > > + static bool not_first = false; > if (! not_first) > + done: > + scratch_buffer_free (&tmpbuf); > + not_first = true; This is missing the acquire/release pairing for the double-checked locking idiom. You can probably use the domain variable directly. > - if ((c = strchr (tmpbuf.data, '.'))) > + if (h && (c = strchr(h->h_name, '.'))) h != NULL? > + if (!scratch_buffer_grow_preserve (&tmpbuf)) I think the change to _preserve should be in the alloca elimination patch (but see my comment there). Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 8:16 ` Florian Weimer @ 2021-11-11 13:34 ` Adhemerval Zanella 2021-11-11 13:54 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-11 13:34 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha; +Cc: leonardo.macchia On 11/11/2021 05:16, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c >> index 8380d85783..58ebbb1154 100644 >> --- a/inet/getnameinfo.c >> +++ b/inet/getnameinfo.c >> @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain); >> static char * >> nrl_domainname (void) >> { >> + __libc_lock_define_initialized (static, lock); >> + __libc_lock_lock (lock); >> >> + static bool not_first = false; >> if (! not_first) > >> + done: >> + scratch_buffer_free (&tmpbuf); >> + not_first = true; > > This is missing the acquire/release pairing for the double-checked > locking idiom. You can probably use the domain variable directly. But it is done now within the lock, different than current implementation which does outside. I moved to be within the lock exactly to avoid the double-checked locking idiom. I think now that we might be moving to a more optimized lll_lock internally using a acquire-load+CAS instead of just CAS we can get it without need to code it explicitly. > >> - if ((c = strchr (tmpbuf.data, '.'))) >> + if (h && (c = strchr(h->h_name, '.'))) > > h != NULL? I tried to make the code as-is, since is essentially removing a tab; but I think it makes sense. > >> + if (!scratch_buffer_grow_preserve (&tmpbuf)) > > > I think the change to _preserve should be in the alloca elimination > patch (but see my comment there). Yeah, this is in fact something I did saw on my rebase, but I am not sure why it is still in the patch submission. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 13:34 ` Adhemerval Zanella @ 2021-11-11 13:54 ` Florian Weimer 2021-11-11 14:09 ` Adhemerval Zanella 0 siblings, 1 reply; 14+ messages in thread From: Florian Weimer @ 2021-11-11 13:54 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia * Adhemerval Zanella: > On 11/11/2021 05:16, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c >>> index 8380d85783..58ebbb1154 100644 >>> --- a/inet/getnameinfo.c >>> +++ b/inet/getnameinfo.c >>> @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain); >>> static char * >>> nrl_domainname (void) >>> { >>> + __libc_lock_define_initialized (static, lock); >>> + __libc_lock_lock (lock); >>> >>> + static bool not_first = false; >>> if (! not_first) >> >>> + done: >>> + scratch_buffer_free (&tmpbuf); >>> + not_first = true; >> >> This is missing the acquire/release pairing for the double-checked >> locking idiom. You can probably use the domain variable directly. > > But it is done now within the lock, different than current implementation > which does outside. I moved to be within the lock exactly to avoid the > double-checked locking idiom. Ah, sorry, I had missed that. > I think now that we might be moving to a more optimized lll_lock internally > using a acquire-load+CAS instead of just CAS we can get it without need > to code it explicitly. The double-checked locking idiom avoids the CAS after initialization. With the lll_lock change, an atomic read-modify-write operation still happens on the lock in all cases (prior to the eventual return to the caller). Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 13:54 ` Florian Weimer @ 2021-11-11 14:09 ` Adhemerval Zanella 2021-11-11 14:12 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-11 14:09 UTC (permalink / raw) To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia On 11/11/2021 10:54, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 11/11/2021 05:16, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c >>>> index 8380d85783..58ebbb1154 100644 >>>> --- a/inet/getnameinfo.c >>>> +++ b/inet/getnameinfo.c >>>> @@ -86,55 +86,75 @@ libc_freeres_ptr (static char *domain); >>>> static char * >>>> nrl_domainname (void) >>>> { >>>> + __libc_lock_define_initialized (static, lock); >>>> + __libc_lock_lock (lock); >>>> >>>> + static bool not_first = false; >>>> if (! not_first) >>> >>>> + done: >>>> + scratch_buffer_free (&tmpbuf); >>>> + not_first = true; >>> >>> This is missing the acquire/release pairing for the double-checked >>> locking idiom. You can probably use the domain variable directly. >> >> But it is done now within the lock, different than current implementation >> which does outside. I moved to be within the lock exactly to avoid the >> double-checked locking idiom. > > Ah, sorry, I had missed that. > >> I think now that we might be moving to a more optimized lll_lock internally >> using a acquire-load+CAS instead of just CAS we can get it without need >> to code it explicitly. > > The double-checked locking idiom avoids the CAS after initialization. > With the lll_lock change, an atomic read-modify-write operation still > happens on the lock in all cases (prior to the eventual return to the > caller). I meant H.J internal lock optimization [1], where the lll_lock int this case will be mostly a relaxed load instead of a CAS (since once 'domain' is properly initialized, there is no need to take the lock). [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 14:09 ` Adhemerval Zanella @ 2021-11-11 14:12 ` Florian Weimer 2021-11-11 14:17 ` Adhemerval Zanella 0 siblings, 1 reply; 14+ messages in thread From: Florian Weimer @ 2021-11-11 14:12 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia * Adhemerval Zanella: > I meant H.J internal lock optimization [1], where the lll_lock int this > case will be mostly a relaxed load instead of a CAS (since once 'domain' > is properly initialized, there is no need to take the lock). > > [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html It doesn't work this way. It still does a CAS. Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 14:12 ` Florian Weimer @ 2021-11-11 14:17 ` Adhemerval Zanella 2021-11-11 17:41 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-11 14:17 UTC (permalink / raw) To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia On 11/11/2021 11:12, Florian Weimer wrote: > * Adhemerval Zanella: > >> I meant H.J internal lock optimization [1], where the lll_lock int this >> case will be mostly a relaxed load instead of a CAS (since once 'domain' >> is properly initialized, there is no need to take the lock). >> >> [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html > > It doesn't work this way. It still does a CAS. Yeah, it helps only on contended case. Do you think we need to keep the double-check optimization here? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) 2021-11-11 14:17 ` Adhemerval Zanella @ 2021-11-11 17:41 ` Florian Weimer 0 siblings, 0 replies; 14+ messages in thread From: Florian Weimer @ 2021-11-11 17:41 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia * Adhemerval Zanella: > On 11/11/2021 11:12, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> I meant H.J internal lock optimization [1], where the lll_lock int this >>> case will be mostly a relaxed load instead of a CAS (since once 'domain' >>> is properly initialized, there is no need to take the lock). >>> >>> [1] https://sourceware.org/pipermail/libc-alpha/2021-November/132899.html >> >> It doesn't work this way. It still does a CAS. > > Yeah, it helps only on contended case. Do you think we need to keep the > double-check optimization here? I would say yes. It also helps with fork safety. After the first call, concurrent forks won't get a bad lock state. Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] inet: Remove strdupa from nrl_domainname() 2021-11-10 18:58 [PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella @ 2021-11-10 18:58 ` Adhemerval Zanella 2021-11-11 8:18 ` Florian Weimer 2021-11-10 18:58 ` [PATCH 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella 2 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-10 18:58 UTC (permalink / raw) To: libc-alpha; +Cc: leonardo.macchia We can use the already in place scratch_buffer. Checked on x86_64-linux-gnu. --- inet/getnameinfo.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c index 58ebbb1154..69a94604bd 100644 --- a/inet/getnameinfo.c +++ b/inet/getnameinfo.c @@ -127,10 +127,10 @@ nrl_domainname (void) else { /* We need to preserve the hostname. */ - const char *hstname = strdupa (tmpbuf.data); - while (__gethostbyname_r (hstname, &th, - tmpbuf.data, - tmpbuf.length, + size_t hstnamelen = strlen (tmpbuf.data) + 1; + while (__gethostbyname_r (tmpbuf.data, &th, + tmpbuf.data + hstnamelen, + tmpbuf.length - hstnamelen, &h, &herror)) { if (herror == NETDB_INTERNAL && errno == ERANGE) -- 2.32.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname() 2021-11-10 18:58 ` [PATCH 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella @ 2021-11-11 8:18 ` Florian Weimer 2021-11-11 13:36 ` Adhemerval Zanella 0 siblings, 1 reply; 14+ messages in thread From: Florian Weimer @ 2021-11-11 8:18 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, leonardo.macchia * Adhemerval Zanella via Libc-alpha: > We can use the already in place scratch_buffer. > > Checked on x86_64-linux-gnu. > --- > inet/getnameinfo.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c > index 58ebbb1154..69a94604bd 100644 > --- a/inet/getnameinfo.c > +++ b/inet/getnameinfo.c > @@ -127,10 +127,10 @@ nrl_domainname (void) > else > { > /* We need to preserve the hostname. */ > - const char *hstname = strdupa (tmpbuf.data); > - while (__gethostbyname_r (hstname, &th, > - tmpbuf.data, > - tmpbuf.length, > + size_t hstnamelen = strlen (tmpbuf.data) + 1; > + while (__gethostbyname_r (tmpbuf.data, &th, > + tmpbuf.data + hstnamelen, > + tmpbuf.length - hstnamelen, > &h, &herror)) > { > if (herror == NETDB_INTERNAL && errno == ERANGE) Can you use malloc instead? scratch_buffer_grow_preserve is a bit of an outlier in the interface. Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname() 2021-11-11 8:18 ` Florian Weimer @ 2021-11-11 13:36 ` Adhemerval Zanella 2021-11-11 14:00 ` Florian Weimer 0 siblings, 1 reply; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-11 13:36 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha; +Cc: leonardo.macchia On 11/11/2021 05:18, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> We can use the already in place scratch_buffer. >> >> Checked on x86_64-linux-gnu. >> --- >> inet/getnameinfo.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c >> index 58ebbb1154..69a94604bd 100644 >> --- a/inet/getnameinfo.c >> +++ b/inet/getnameinfo.c >> @@ -127,10 +127,10 @@ nrl_domainname (void) >> else >> { >> /* We need to preserve the hostname. */ >> - const char *hstname = strdupa (tmpbuf.data); >> - while (__gethostbyname_r (hstname, &th, >> - tmpbuf.data, >> - tmpbuf.length, >> + size_t hstnamelen = strlen (tmpbuf.data) + 1; >> + while (__gethostbyname_r (tmpbuf.data, &th, >> + tmpbuf.data + hstnamelen, >> + tmpbuf.length - hstnamelen, >> &h, &herror)) >> { >> if (herror == NETDB_INTERNAL && errno == ERANGE) > > Can you use malloc instead? scratch_buffer_grow_preserve is a bit of an > outlier in the interface. It would require to add more complexity, since now we have two allocations to handle (the hstname and the scratch_buffer) and on fast patch it would actually require a malloc call, where mostly likely it would be done in the stack fror scratch_buffer. I am not sure it is really an improvement here. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] inet: Remove strdupa from nrl_domainname() 2021-11-11 13:36 ` Adhemerval Zanella @ 2021-11-11 14:00 ` Florian Weimer 0 siblings, 0 replies; 14+ messages in thread From: Florian Weimer @ 2021-11-11 14:00 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha, leonardo.macchia * Adhemerval Zanella: > On 11/11/2021 05:18, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> We can use the already in place scratch_buffer. >>> >>> Checked on x86_64-linux-gnu. >>> --- >>> inet/getnameinfo.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c >>> index 58ebbb1154..69a94604bd 100644 >>> --- a/inet/getnameinfo.c >>> +++ b/inet/getnameinfo.c >>> @@ -127,10 +127,10 @@ nrl_domainname (void) >>> else >>> { >>> /* We need to preserve the hostname. */ >>> - const char *hstname = strdupa (tmpbuf.data); >>> - while (__gethostbyname_r (hstname, &th, >>> - tmpbuf.data, >>> - tmpbuf.length, >>> + size_t hstnamelen = strlen (tmpbuf.data) + 1; >>> + while (__gethostbyname_r (tmpbuf.data, &th, >>> + tmpbuf.data + hstnamelen, >>> + tmpbuf.length - hstnamelen, >>> &h, &herror)) >>> { >>> if (herror == NETDB_INTERNAL && errno == ERANGE) >> >> Can you use malloc instead? scratch_buffer_grow_preserve is a bit of an >> outlier in the interface. > > It would require to add more complexity, since now we have two allocations > to handle (the hstname and the scratch_buffer) and on fast patch it would > actually require a malloc call, where mostly likely it would be done in > the stack fror scratch_buffer. I am not sure it is really an improvement > here. Fair enough, let's keep the code simple. Thanks, Florian ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory 2021-11-10 18:58 [PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella @ 2021-11-10 18:58 ` Adhemerval Zanella 2 siblings, 0 replies; 14+ messages in thread From: Adhemerval Zanella @ 2021-11-10 18:58 UTC (permalink / raw) To: libc-alpha; +Cc: leonardo.macchia It aligns NI_NOFQDN with default behavior for getnameinfo(). Checked on x86_64-linux-gnu. --- inet/getnameinfo.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c index 69a94604bd..16d461428d 100644 --- a/inet/getnameinfo.c +++ b/inet/getnameinfo.c @@ -83,9 +83,11 @@ libc_freeres_ptr (static char *domain); now ignored. */ #define DEPRECATED_NI_IDN 192 -static char * +static bool nrl_domainname (void) { + int r; + __libc_lock_define_initialized (static, lock); __libc_lock_lock (lock); @@ -171,15 +173,17 @@ nrl_domainname (void) } } } + not_first = true; done: scratch_buffer_free (&tmpbuf); - not_first = true; } + r = not_first; + __libc_lock_unlock (lock); - return domain; + return r; }; /* Copy a string to a destination buffer with length checking. Return @@ -275,13 +279,17 @@ gni_host_inet_name (struct scratch_buffer *tmpbuf, if (h) { - char *c; - if ((flags & NI_NOFQDN) - && (c = nrl_domainname ()) - && (c = strstr (h->h_name, c)) - && (c != h->h_name) && (*(--c) == '.')) - /* Terminate the string after the prefix. */ - *c = '\0'; + if (flags & NI_NOFQDN) + { + if (!nrl_domainname ()) + return EAI_MEMORY; + + char *c = domain; + if (c != NULL && (c = strstr (h->h_name, c)) + && (c != h->h_name) && (*(--c) == '.')) + /* Terminate the string after the prefix. */ + *c = '\0'; + } /* If requested, convert from the IDN format. */ bool do_idn = flags & NI_IDN; -- 2.32.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-11 17:41 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-10 18:58 [PATCH 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella 2021-11-10 18:58 ` [PATCH 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella 2021-11-11 8:16 ` Florian Weimer 2021-11-11 13:34 ` Adhemerval Zanella 2021-11-11 13:54 ` Florian Weimer 2021-11-11 14:09 ` Adhemerval Zanella 2021-11-11 14:12 ` Florian Weimer 2021-11-11 14:17 ` Adhemerval Zanella 2021-11-11 17:41 ` Florian Weimer 2021-11-10 18:58 ` [PATCH 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella 2021-11-11 8:18 ` Florian Weimer 2021-11-11 13:36 ` Adhemerval Zanella 2021-11-11 14:00 ` Florian Weimer 2021-11-10 18:58 ` [PATCH 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).