wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584]
Commit Message
* 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,
>>>> + ©->towc->__counter);
>>>> if (copy->tomb->__shlib_handle != NULL)
>>>> - ++copy->tomb->__counter;
>>>> + overflow |= __builtin_add_overflow (copy->tomb->__counter, 1,
>>>> + ©->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.
Fair enough. Updated patch below.
Thanks,
Florian
wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584]
This also adds an overflow check and documents the synchronization
requirement in <gconv.h>.
2019-05-21 Florian Weimer <fweimer@redhat.com>
[BZ #24584]
* wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Acquire __gconv_lock
before updating __counter field and release it afterwards. Add
overflow check.
* iconv/gconv.h (struct __gconv_step): Mention synchronization
requirement for __counter member.
Comments
On Mai 21 2019, Florian Weimer <fweimer@redhat.com> wrote:
> [BZ #24584]
> * wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Acquire __gconv_lock
> before updating __counter field and release it afterwards. Add
> overflow check.
> * iconv/gconv.h (struct __gconv_step): Mention synchronization
> requirement for __counter member.
Ok.
Andreas.
@@ -86,6 +86,8 @@ struct __gconv_step
struct __gconv_loaded_object *__shlib_handle;
const char *__modname;
+ /* For internal use by glibc. (Accesses to this member must occur
+ when the internal __gconv_lock mutex is acquired). */
int __counter;
char *__from_name;
@@ -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,25 @@ __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,
+ ©->towc->__counter);
if (copy->tomb->__shlib_handle != NULL)
- ++copy->tomb->__counter;
+ overflow |= __builtin_add_overflow (copy->tomb->__counter, 1,
+ ©->tomb->__counter);
+
+ __libc_lock_unlock (__gconv_lock);
+
+ if (overflow)
+ __libc_fatal ("\
+Fatal glibc error: gconv module reference counter overflow\n");
}