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

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

[Florian Weimer]

This also adds an overflow check and documents the synchronization
requirement in <iconv/gconv_int.h>.

2019-05-20  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_int.h (struct __gconv_loaded_object): Mention
	synchronization requirement for __counter member.

diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
index ea41d6feaa..9510102c07 100644
--- a/iconv/gconv_int.h
+++ b/iconv/gconv_int.h
@@ -45,7 +45,8 @@ struct __gconv_loaded_object
   const char *name;
 
   /* Reference counter for the db functionality.  If no conversion is
-     needed we unload the db library.  */
+     needed we unload the db library.  __gconv_lock is used to
+     synchronize updates to this field.  */
   int counter;
 
   /* The handle for the shared object.  */
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);
 }
 
 

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

[Andreas Schwab]

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

> diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
> index ea41d6feaa..9510102c07 100644
> --- a/iconv/gconv_int.h
> +++ b/iconv/gconv_int.h
> @@ -45,7 +45,8 @@ struct __gconv_loaded_object
>    const char *name;
>  
>    /* Reference counter for the db functionality.  If no conversion is
> -     needed we unload the db library.  */
> +     needed we unload the db library.  __gconv_lock is used to
> +     synchronize updates to this field.  */
>    int counter;

That's struct __gconv_loaded_object.

> @@ -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);

That's struct __gconv_step.

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."

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

[Florian Weimer]

* Andreas Schwab:

> On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:
>
>> diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
>> index ea41d6feaa..9510102c07 100644
>> --- a/iconv/gconv_int.h
>> +++ b/iconv/gconv_int.h
>> @@ -45,7 +45,8 @@ struct __gconv_loaded_object
>>    const char *name;
>>  
>>    /* Reference counter for the db functionality.  If no conversion is
>> -     needed we unload the db library.  */
>> +     needed we unload the db library.  __gconv_lock is used to
>> +     synchronize updates to this field.  */
>>    int counter;
>
> That's struct __gconv_loaded_object.
>
>> @@ -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);
>
> That's struct __gconv_step.

Thanks.  What about this patch?

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-20  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.

diff --git a/iconv/gconv.h b/iconv/gconv.h
index 5ad26c06ac..7ce79bcbf6 100644
--- a/iconv/gconv.h
+++ b/iconv/gconv.h
@@ -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;
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);
 }
 
 

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

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

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."

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

[Florian Weimer]

* 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

Re: [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."

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

[Florian Weimer]

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

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.

diff --git a/iconv/gconv.h b/iconv/gconv.h
index 5ad26c06ac..7ce79bcbf6 100644
--- a/iconv/gconv.h
+++ b/iconv/gconv.h
@@ -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;
diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
index 5494d0a23e..6648365d82 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,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,
+					&copy->towc->__counter);
   if (copy->tomb->__shlib_handle != NULL)
-    ++copy->tomb->__counter;
+    overflow |= __builtin_add_overflow (copy->tomb->__counter, 1,
+					&copy->tomb->__counter);
+
+  __libc_lock_unlock (__gconv_lock);
+
+  if (overflow)
+    __libc_fatal ("\
+Fatal glibc error: gconv module reference counter overflow\n");
 }
 
 

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

[Andreas Schwab]

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.

-- 
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."