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

[Florian Weimer <fweimer@redhat.com>]

* 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?

Thanks,
Florian