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

Message ID 874l5pl5gk.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584]
Related show

Commit Message

Florian Weimer May 20, 2019, 11:40 a.m.
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.

Comments

Andreas Schwab May 20, 2019, 12:26 p.m. | #1
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."
Florian Weimer May 20, 2019, 1:28 p.m. | #2
* 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);
 }
Andreas Schwab May 20, 2019, 4:11 p.m. | #3
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."
Florian Weimer May 20, 2019, 5:18 p.m. | #4
* 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
Andreas Schwab May 21, 2019, 7:15 a.m. | #5
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."
Florian Weimer May 21, 2019, 8:38 a.m. | #6
* 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");
 }
Andreas Schwab May 21, 2019, 9:04 a.m. | #7
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."

Patch

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