[PATCH] wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584]

[Andreas Schwab]

On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:
>
>> On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
>>> index 5494d0a23e..e33a9c1312 100644
>>> --- a/wcsmbs/wcsmbsload.c
>>> +++ b/wcsmbs/wcsmbsload.c
>>> @@ -20,6 +20,7 @@
>>>  #include <langinfo.h>
>>>  #include <limits.h>
>>>  #include <stdlib.h>
>>> +#include <stdio.h>
>>>  #include <string.h>
>>>  
>>>  #include <locale/localeinfo.h>
>>> @@ -223,12 +224,24 @@ __wcsmbs_clone_conv (struct gconv_fcts *copy)
>>>    /* Copy the data.  */
>>>    *copy = *orig;
>>>  
>>> -  /* Now increment the usage counters.
>>> -     Note: This assumes copy->*_nsteps == 1.  */
>>> +  /* Now increment the usage counters.  Note: This assumes
>>> +     copy->*_nsteps == 1.  The current locale holds a reference, so it
>>> +     is still there after acquiring the lock.  */
>>> +
>>> +  __libc_lock_lock (__gconv_lock);
>>> +
>>> +  bool overflow = false;
>>>    if (copy->towc->__shlib_handle != NULL)
>>> -    ++copy->towc->__counter;
>>> +    overflow |= __builtin_add_overflow (copy->towc->__counter, 1,
>>> +					&copy->towc->__counter);
>>>    if (copy->tomb->__shlib_handle != NULL)
>>> -    ++copy->tomb->__counter;
>>> +    overflow |= __builtin_add_overflow (copy->tomb->__counter, 1,
>>> +					&copy->tomb->__counter);
>>> +  if (overflow)
>>> +    __libc_fatal ("\
>>> +Fatal glibc error: gconv module reference counter overflow\n");
>>> +
>>> +  __libc_lock_unlock (__gconv_lock);
>>
>> Should the lock be dropped before __libc_fatal?
>
> I think this is purely a matter of style because __libc_fatal does not
> return.  Do you have a preference?

I think it would be a nice to avoid leaving internal locks locked when
calling abort, in case a SIGABRT handler does something stupid.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."