* [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN
@ 2021-11-12 14:21 Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2021-11-12 14:21 UTC (permalink / raw)
To: libc-alpha; +Cc: leonardo.macchia
Changes from v1:
* Keep the double-checked locking optimization.
* Also check for strdup return for EAI_MEMORY
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 | 181 +++++++++++++++++++++++++--------------------
1 file changed, 102 insertions(+), 79 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
2021-11-12 14:21 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
@ 2021-11-12 14:21 ` Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella
2 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2021-11-12 14:21 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.
This patch fix the double-checked locking by using acquire-release
atomic operation instead of plain load and by moving the 'not_first'
store only after 'domain' is actually set.
Checked on x86_64-linux-gnu.
---
inet/getnameinfo.c | 148 ++++++++++++++++++++++++---------------------
1 file changed, 78 insertions(+), 70 deletions(-)
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 8380d85783..5eee354200 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -83,104 +83,112 @@ libc_freeres_ptr (static char *domain);
now ignored. */
#define DEPRECATED_NI_IDN 192
-static char *
-nrl_domainname (void)
+static void
+nrl_domainname_core (struct scratch_buffer *tmpbuf)
{
- static int not_first;
+ char *c;
+ struct hostent *h, th;
+ int herror;
- if (! not_first)
+ while (__gethostbyname_r ("localhost", &th,
+ tmpbuf->data, tmpbuf->length,
+ &h, &herror))
{
- __libc_lock_define_initialized (static, lock);
- __libc_lock_lock (lock);
-
- if (! not_first)
+ if (herror == NETDB_INTERNAL && errno == ERANGE)
{
- char *c;
- struct hostent *h, th;
- int herror;
- struct scratch_buffer tmpbuf;
-
- scratch_buffer_init (&tmpbuf);
- not_first = 1;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
+ }
+ else
+ break;
+ }
- while (__gethostbyname_r ("localhost", &th,
- tmpbuf.data, tmpbuf.length,
+ if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
+ 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))
+ return;
+
+ if ((c = strchr (tmpbuf->data, '.')) != NULL)
+ domain = __strdup (++c);
+ else
+ {
+ /* 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 (&tmpbuf))
- goto done;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
}
else
break;
}
- if (h && (c = strchr (h->h_name, '.')))
+ if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
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;
+ struct in_addr in_addr;
- if ((c = strchr (tmpbuf.data, '.')))
- domain = __strdup (++c);
- else
- {
- /* We need to preserve the hostname. */
- const char *hstname = strdupa (tmpbuf.data);
+ in_addr.s_addr = htonl (INADDR_LOOPBACK);
- while (__gethostbyname_r (hstname, &th,
- tmpbuf.data, tmpbuf.length,
- &h, &herror))
+ 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 (herror == NETDB_INTERNAL && errno == ERANGE)
- {
- if (!scratch_buffer_grow (&tmpbuf))
- goto done;
- }
- else
- break;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
}
-
- 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);
- }
+ break;
}
+
+ if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
+ domain = __strdup (++c);
}
- done:
- scratch_buffer_free (&tmpbuf);
}
+ }
+}
- __libc_lock_unlock (lock);
+static char *
+nrl_domainname (void)
+{
+ static int not_first;
+
+ if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
+ return domain;
+
+ __libc_lock_define_initialized (static, lock);
+ __libc_lock_lock (lock);
+
+ if (atomic_load_relaxed (¬_first) == 0)
+ {
+ struct scratch_buffer tmpbuf;
+ scratch_buffer_init (&tmpbuf);
+
+ nrl_domainname_core (&tmpbuf);
+
+ scratch_buffer_free (&tmpbuf);
+
+ atomic_store_release (¬_first, 1);
}
+ __libc_lock_unlock (lock);
+
return domain;
};
--
2.32.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] inet: Remove strdupa from nrl_domainname()
2021-11-12 14:21 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
@ 2021-11-12 14:21 ` Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella
2 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2021-11-12 14:21 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 | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 5eee354200..2d2397e7dc 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -118,15 +118,15 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
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)
{
- if (!scratch_buffer_grow (tmpbuf))
+ if (!scratch_buffer_grow_preserve (tmpbuf))
return;
}
else
--
2.32.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory
2021-11-12 14:21 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella
@ 2021-11-12 14:21 ` Adhemerval Zanella
2021-11-12 15:44 ` Andreas Schwab
2 siblings, 1 reply; 9+ messages in thread
From: Adhemerval Zanella @ 2021-11-12 14:21 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 | 55 +++++++++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 2d2397e7dc..2bec3263fc 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -83,7 +83,7 @@ libc_freeres_ptr (static char *domain);
now ignored. */
#define DEPRECATED_NI_IDN 192
-static void
+static bool
nrl_domainname_core (struct scratch_buffer *tmpbuf)
{
char *c;
@@ -97,7 +97,7 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
if (herror == NETDB_INTERNAL && errno == ERANGE)
{
if (!scratch_buffer_grow (tmpbuf))
- return;
+ return false;
}
else
break;
@@ -111,10 +111,13 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
now to get more information. */
while (__gethostname (tmpbuf->data, tmpbuf->length))
if (!scratch_buffer_grow (tmpbuf))
- return;
+ return false;
if ((c = strchr (tmpbuf->data, '.')) != NULL)
- domain = __strdup (++c);
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
else
{
/* We need to preserve the hostname. */
@@ -127,14 +130,17 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
if (herror == NETDB_INTERNAL && errno == ERANGE)
{
if (!scratch_buffer_grow_preserve (tmpbuf))
- return;
+ return false;
}
else
break;
}
if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
- domain = __strdup (++c);
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
else
{
struct in_addr in_addr;
@@ -151,20 +157,24 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
if (herror == NETDB_INTERNAL && errno == ERANGE)
{
if (!scratch_buffer_grow (tmpbuf))
- return;
+ return false;
}
else
break;
}
if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
- domain = __strdup (++c);
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
}
}
}
+ return true;
}
-static char *
+static bool
nrl_domainname (void)
{
static int not_first;
@@ -172,6 +182,8 @@ nrl_domainname (void)
if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
return domain;
+ int r = true;
+
__libc_lock_define_initialized (static, lock);
__libc_lock_lock (lock);
@@ -180,16 +192,15 @@ nrl_domainname (void)
struct scratch_buffer tmpbuf;
scratch_buffer_init (&tmpbuf);
- nrl_domainname_core (&tmpbuf);
+ if ((r = nrl_domainname_core (&tmpbuf)))
+ atomic_store_release (¬_first, 1);
scratch_buffer_free (&tmpbuf);
-
- atomic_store_release (¬_first, 1);
}
__libc_lock_unlock (lock);
- return domain;
+ return r;
};
/* Copy a string to a destination buffer with length checking. Return
@@ -285,13 +296,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] 9+ messages in thread
* Re: [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory
2021-11-12 14:21 ` [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella
@ 2021-11-12 15:44 ` Andreas Schwab
2021-11-25 16:38 ` [PATCH v3] " Adhemerval Zanella
0 siblings, 1 reply; 9+ messages in thread
From: Andreas Schwab @ 2021-11-12 15:44 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, leonardo.macchia
On Nov 12 2021, Adhemerval Zanella via Libc-alpha wrote:
> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
> index 2d2397e7dc..2bec3263fc 100644
> --- a/inet/getnameinfo.c
> +++ b/inet/getnameinfo.c
> @@ -83,7 +83,7 @@ libc_freeres_ptr (static char *domain);
> now ignored. */
> #define DEPRECATED_NI_IDN 192
>
> -static void
> +static bool
> nrl_domainname_core (struct scratch_buffer *tmpbuf)
> {
> char *c;
> @@ -97,7 +97,7 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
> if (herror == NETDB_INTERNAL && errno == ERANGE)
> {
> if (!scratch_buffer_grow (tmpbuf))
> - return;
> + return false;
> }
> else
> break;
> @@ -111,10 +111,13 @@ nrl_domainname_core (struct scratch_buffer *tmpbuf)
> now to get more information. */
> while (__gethostname (tmpbuf->data, tmpbuf->length))
> if (!scratch_buffer_grow (tmpbuf))
> - return;
> + return false;
>
> if ((c = strchr (tmpbuf->data, '.')) != NULL)
> - domain = __strdup (++c);
> + {
> + domain = __strdup (++c);
> + return domain != NULL;
> + }
> else
You can remove `else' here and save a level of indentation.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory
2021-11-12 15:44 ` Andreas Schwab
@ 2021-11-25 16:38 ` Adhemerval Zanella
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2021-11-25 16:38 UTC (permalink / raw)
To: libc-alpha, Andreas Schwab
It aligns NI_NOFQDN with default behavior for getnameinfo().
Checked on x86_64-linux-gnu.
---
Changes from v2:
* Simplify nrl_domainname_core.
---
inet/getnameinfo.c | 140 +++++++++++++++++++++++----------------------
1 file changed, 72 insertions(+), 68 deletions(-)
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 2d2397e7dc..acf1538a37 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -83,94 +83,95 @@ libc_freeres_ptr (static char *domain);
now ignored. */
#define DEPRECATED_NI_IDN 192
-static void
+static bool
nrl_domainname_core (struct scratch_buffer *tmpbuf)
{
char *c;
struct hostent *h, th;
int herror;
- while (__gethostbyname_r ("localhost", &th,
- tmpbuf->data, tmpbuf->length,
+ while (__gethostbyname_r ("localhost", &th, tmpbuf->data, tmpbuf->length,
&h, &herror))
{
if (herror == NETDB_INTERNAL && errno == ERANGE)
{
if (!scratch_buffer_grow (tmpbuf))
- return;
+ return false;
}
else
break;
}
if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
- 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))
- return;
-
- if ((c = strchr (tmpbuf->data, '.')) != NULL)
- domain = __strdup (++c);
- else
- {
- /* We need to preserve the hostname. */
- 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)
- {
- if (!scratch_buffer_grow_preserve (tmpbuf))
- return;
- }
- else
- break;
- }
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
- if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
- domain = __strdup (++c);
- else
- {
- struct in_addr in_addr;
+ /* 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))
+ return false;
- in_addr.s_addr = htonl (INADDR_LOOPBACK);
+ if ((c = strchr (tmpbuf->data, '.')) != NULL)
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
- 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))
- return;
- }
- else
- break;
- }
+ /* We need to preserve the hostname. */
+ 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)
+ {
+ if (!scratch_buffer_grow_preserve (tmpbuf))
+ return false;
+ }
+ else
+ break;
+ }
- if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
- domain = __strdup (++c);
- }
+ if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
+ }
+
+ struct 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))
+ return false;
}
+ else
+ break;
+ }
+
+ if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
+ {
+ domain = __strdup (++c);
+ return domain != NULL;
}
+ return true;
}
-static char *
+static bool
nrl_domainname (void)
{
static int not_first;
if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
- return domain;
+ return true;
+
+ int r = true;
__libc_lock_define_initialized (static, lock);
__libc_lock_lock (lock);
@@ -180,16 +181,15 @@ nrl_domainname (void)
struct scratch_buffer tmpbuf;
scratch_buffer_init (&tmpbuf);
- nrl_domainname_core (&tmpbuf);
+ if ((r = nrl_domainname_core (&tmpbuf)))
+ atomic_store_release (¬_first, 1);
scratch_buffer_free (&tmpbuf);
-
- atomic_store_release (¬_first, 1);
}
__libc_lock_unlock (lock);
- return domain;
+ return r;
};
/* Copy a string to a destination buffer with length checking. Return
@@ -285,13 +285,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] 9+ messages in thread
* Re: [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
2021-12-10 11:07 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
2022-02-03 20:52 ` Adhemerval Zanella
@ 2022-03-08 3:44 ` DJ Delorie
1 sibling, 0 replies; 9+ messages in thread
From: DJ Delorie @ 2022-03-08 3:44 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
Reviewing the resulting text intead of the patch, because... easier?
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
> static void
> nrl_domainname_core (struct scratch_buffer *tmpbuf)
> {
> char *c;
> struct hostent *h, th;
> int herror;
>
> while (__gethostbyname_r ("localhost", &th,
> tmpbuf->data, tmpbuf->length,
> &h, &herror))
> {
> if (herror == NETDB_INTERNAL && errno == ERANGE)
> {
> if (!scratch_buffer_grow (tmpbuf))
> return;
> }
> else
> break;
> }
Ok.
> if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
> domain = __strdup (++c);
> else
Ok.
> {
> /* 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))
> return;
Ok.
> if ((c = strchr (tmpbuf->data, '.')) != NULL)
> domain = __strdup (++c);
> else
> {
> /* 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 (tmpbuf))
> return;
> }
> else
> break;
> }
Ok.
> if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
> 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))
> return;
> }
> else
> break;
> }
Ok.
> if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
> domain = __strdup (++c);
> }
> }
> }
> }
Ok.
> static char *
> nrl_domainname (void)
> {
> static int not_first;
>
> if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
> return domain;
Multiple threads may have gotten to this point the "first time" (esp if
they get called while the above function is running)...
> __libc_lock_define_initialized (static, lock);
> __libc_lock_lock (lock);
>
> if (atomic_load_relaxed (¬_first) == 0)
> {
... so we test again inside the lock, only one will get the lock *and*
see not_first still zero; ok.
> struct scratch_buffer tmpbuf;
> scratch_buffer_init (&tmpbuf);
>
> nrl_domainname_core (&tmpbuf);
>
> scratch_buffer_free (&tmpbuf);
Calculate the static "domain", ok
> atomic_store_release (¬_first, 1);
Set not_first before releasing lock; Ok.
> }
>
> __libc_lock_unlock (lock);
This is the only exit path, so OK.
> return domain;
> };
Ok.
LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
2021-12-10 11:07 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
@ 2022-02-03 20:52 ` Adhemerval Zanella
2022-03-08 3:44 ` DJ Delorie
1 sibling, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2022-02-03 20:52 UTC (permalink / raw)
To: libc-alpha
Ping.
On 10/12/2021 08:07, Adhemerval Zanella wrote:
> The 'not_first' is accessed on nrl_domainname() in a non atomically
> way, although it is only updated after the lock is taken.
>
> This patch fix the double-checked locking by using acquire-release
> atomic operation instead of plain load and by moving the 'not_first'
> store only after 'domain' is actually set.
>
> Checked on x86_64-linux-gnu.
> ---
> inet/getnameinfo.c | 148 ++++++++++++++++++++++++---------------------
> 1 file changed, 78 insertions(+), 70 deletions(-)
>
> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
> index 8380d85783..5eee354200 100644
> --- a/inet/getnameinfo.c
> +++ b/inet/getnameinfo.c
> @@ -83,104 +83,112 @@ libc_freeres_ptr (static char *domain);
> now ignored. */
> #define DEPRECATED_NI_IDN 192
>
> -static char *
> -nrl_domainname (void)
> +static void
> +nrl_domainname_core (struct scratch_buffer *tmpbuf)
> {
> - static int not_first;
> + char *c;
> + struct hostent *h, th;
> + int herror;
>
> - if (! not_first)
> + while (__gethostbyname_r ("localhost", &th,
> + tmpbuf->data, tmpbuf->length,
> + &h, &herror))
> {
> - __libc_lock_define_initialized (static, lock);
> - __libc_lock_lock (lock);
> -
> - if (! not_first)
> + if (herror == NETDB_INTERNAL && errno == ERANGE)
> {
> - char *c;
> - struct hostent *h, th;
> - int herror;
> - struct scratch_buffer tmpbuf;
> -
> - scratch_buffer_init (&tmpbuf);
> - not_first = 1;
> + if (!scratch_buffer_grow (tmpbuf))
> + return;
> + }
> + else
> + break;
> + }
>
> - while (__gethostbyname_r ("localhost", &th,
> - tmpbuf.data, tmpbuf.length,
> + if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
> + 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))
> + return;
> +
> + if ((c = strchr (tmpbuf->data, '.')) != NULL)
> + domain = __strdup (++c);
> + else
> + {
> + /* 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 (&tmpbuf))
> - goto done;
> + if (!scratch_buffer_grow (tmpbuf))
> + return;
> }
> else
> break;
> }
>
> - if (h && (c = strchr (h->h_name, '.')))
> + if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
> 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;
> + struct in_addr in_addr;
>
> - if ((c = strchr (tmpbuf.data, '.')))
> - domain = __strdup (++c);
> - else
> - {
> - /* We need to preserve the hostname. */
> - const char *hstname = strdupa (tmpbuf.data);
> + in_addr.s_addr = htonl (INADDR_LOOPBACK);
>
> - while (__gethostbyname_r (hstname, &th,
> - tmpbuf.data, tmpbuf.length,
> - &h, &herror))
> + 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 (herror == NETDB_INTERNAL && errno == ERANGE)
> - {
> - if (!scratch_buffer_grow (&tmpbuf))
> - goto done;
> - }
> - else
> - break;
> + if (!scratch_buffer_grow (tmpbuf))
> + return;
> }
> -
> - 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);
> - }
> + break;
> }
> +
> + if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
> + domain = __strdup (++c);
> }
> - done:
> - scratch_buffer_free (&tmpbuf);
> }
> + }
> +}
>
> - __libc_lock_unlock (lock);
> +static char *
> +nrl_domainname (void)
> +{
> + static int not_first;
> +
> + if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
> + return domain;
> +
> + __libc_lock_define_initialized (static, lock);
> + __libc_lock_lock (lock);
> +
> + if (atomic_load_relaxed (¬_first) == 0)
> + {
> + struct scratch_buffer tmpbuf;
> + scratch_buffer_init (&tmpbuf);
> +
> + nrl_domainname_core (&tmpbuf);
> +
> + scratch_buffer_free (&tmpbuf);
> +
> + atomic_store_release (¬_first, 1);
> }
>
> + __libc_lock_unlock (lock);
> +
> return domain;
> };
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
2021-12-10 11:07 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
@ 2021-12-10 11:07 ` Adhemerval Zanella
2022-02-03 20:52 ` Adhemerval Zanella
2022-03-08 3:44 ` DJ Delorie
0 siblings, 2 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2021-12-10 11:07 UTC (permalink / raw)
To: libc-alpha
The 'not_first' is accessed on nrl_domainname() in a non atomically
way, although it is only updated after the lock is taken.
This patch fix the double-checked locking by using acquire-release
atomic operation instead of plain load and by moving the 'not_first'
store only after 'domain' is actually set.
Checked on x86_64-linux-gnu.
---
inet/getnameinfo.c | 148 ++++++++++++++++++++++++---------------------
1 file changed, 78 insertions(+), 70 deletions(-)
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 8380d85783..5eee354200 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -83,104 +83,112 @@ libc_freeres_ptr (static char *domain);
now ignored. */
#define DEPRECATED_NI_IDN 192
-static char *
-nrl_domainname (void)
+static void
+nrl_domainname_core (struct scratch_buffer *tmpbuf)
{
- static int not_first;
+ char *c;
+ struct hostent *h, th;
+ int herror;
- if (! not_first)
+ while (__gethostbyname_r ("localhost", &th,
+ tmpbuf->data, tmpbuf->length,
+ &h, &herror))
{
- __libc_lock_define_initialized (static, lock);
- __libc_lock_lock (lock);
-
- if (! not_first)
+ if (herror == NETDB_INTERNAL && errno == ERANGE)
{
- char *c;
- struct hostent *h, th;
- int herror;
- struct scratch_buffer tmpbuf;
-
- scratch_buffer_init (&tmpbuf);
- not_first = 1;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
+ }
+ else
+ break;
+ }
- while (__gethostbyname_r ("localhost", &th,
- tmpbuf.data, tmpbuf.length,
+ if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
+ 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))
+ return;
+
+ if ((c = strchr (tmpbuf->data, '.')) != NULL)
+ domain = __strdup (++c);
+ else
+ {
+ /* 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 (&tmpbuf))
- goto done;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
}
else
break;
}
- if (h && (c = strchr (h->h_name, '.')))
+ if (h != NULL && (c = strchr(h->h_name, '.')) != NULL)
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;
+ struct in_addr in_addr;
- if ((c = strchr (tmpbuf.data, '.')))
- domain = __strdup (++c);
- else
- {
- /* We need to preserve the hostname. */
- const char *hstname = strdupa (tmpbuf.data);
+ in_addr.s_addr = htonl (INADDR_LOOPBACK);
- while (__gethostbyname_r (hstname, &th,
- tmpbuf.data, tmpbuf.length,
- &h, &herror))
+ 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 (herror == NETDB_INTERNAL && errno == ERANGE)
- {
- if (!scratch_buffer_grow (&tmpbuf))
- goto done;
- }
- else
- break;
+ if (!scratch_buffer_grow (tmpbuf))
+ return;
}
-
- 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);
- }
+ break;
}
+
+ if (h != NULL && (c = strchr (h->h_name, '.')) != NULL)
+ domain = __strdup (++c);
}
- done:
- scratch_buffer_free (&tmpbuf);
}
+ }
+}
- __libc_lock_unlock (lock);
+static char *
+nrl_domainname (void)
+{
+ static int not_first;
+
+ if (__glibc_likely (atomic_load_acquire (¬_first) != 0))
+ return domain;
+
+ __libc_lock_define_initialized (static, lock);
+ __libc_lock_lock (lock);
+
+ if (atomic_load_relaxed (¬_first) == 0)
+ {
+ struct scratch_buffer tmpbuf;
+ scratch_buffer_init (&tmpbuf);
+
+ nrl_domainname_core (&tmpbuf);
+
+ scratch_buffer_free (&tmpbuf);
+
+ atomic_store_release (¬_first, 1);
}
+ __libc_lock_unlock (lock);
+
return domain;
};
--
2.32.0
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-08 3:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-12 14:21 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 2/3] inet: Remove strdupa from nrl_domainname() Adhemerval Zanella
2021-11-12 14:21 ` [PATCH v2 3/3] inet: Return EAI_MEMORY when nrl_domainname() fails to allocate memory Adhemerval Zanella
2021-11-12 15:44 ` Andreas Schwab
2021-11-25 16:38 ` [PATCH v3] " Adhemerval Zanella
2021-12-10 11:07 [PATCH v2 0/3] Fixes for getnameinfo() with NI_NOFQDN Adhemerval Zanella
2021-12-10 11:07 ` [PATCH v2 1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566) Adhemerval Zanella
2022-02-03 20:52 ` Adhemerval Zanella
2022-03-08 3:44 ` DJ Delorie
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).