From patchwork Tue May 21 08:38:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 32777 Received: (qmail 15387 invoked by alias); 21 May 2019 08:38:23 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15377 invoked by uid 89); 21 May 2019 08:38:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com From: Florian Weimer To: Andreas Schwab Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] wcsmbs: Fix data race in __wcsmbs_clone_conv [BZ #24584] References: <874l5pl5gk.fsf@oldenburg2.str.redhat.com> <87k1eljlwb.fsf@oldenburg2.str.redhat.com> <875zq5hwo9.fsf@oldenburg2.str.redhat.com> Date: Tue, 21 May 2019 10:38:09 +0200 In-Reply-To: (Andreas Schwab's message of "Tue, 21 May 2019 09:15:54 +0200") Message-ID: <87sgt8dwy6.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 * Andreas Schwab: > On Mai 20 2019, Florian Weimer wrote: > >> * Andreas Schwab: >> >>> On Mai 20 2019, Florian Weimer 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 >>>> #include >>>> #include >>>> +#include >>>> #include >>>> >>>> #include >>>> @@ -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 . 2019-05-21 Florian Weimer [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 #include #include +#include #include #include @@ -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"); }