[v2,1/3] inet: Fix getnameinfo (NI_NOFQDN) race condition (BZ#28566)
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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(-)
Comments
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;
> };
>
@@ -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;
};