| Message ID | 874l5pl5gk.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584] | expand |
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, > + ©->towc->__counter); That's struct __gconv_step. Andreas.
* 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, >> + ©->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, + ©->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); }
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? Andreas.
* 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? Thanks, Florian
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. Andreas.
* 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. 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, + ©->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"); }
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.
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, + ©->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); }