[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.
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(-)
Comments
* 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
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.
* 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
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
* 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
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?
* 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
@@ -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;
};