From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.1 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 4EF871F462 for ; Tue, 21 May 2019 08:38:25 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; q=dns; s=default; b=wq5Uc xSbuNDXp7/MyPb9UVaeA7z/lxEQnM0psU9x2j+P8SCQD2AD+4uC1CNccGA2FKRkH ErlbG0LZ67EZsda73cpWXHGVqb7OK66dE7X2OuPAFCqKdt4JetFWfLj2yw4b8VC4 tdNf0+5XUmoKn4MxWP+Au9ZhgVLiKHoS4gsitc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:references:date:in-reply-to :message-id:mime-version:content-type; s=default; bh=bHu7TpLkdpz MpuQOvw1fhqd7HvY=; b=R0iQO04lZk9v8dfSZbN/hnRzqPkeVno7LLnLb5cyhvv bawuuvUJDt4Rn6k2/tFhzfjqtZEigd/gRF9jStd4LoPfj44CRbBnwokDSy2Cfeoo mXzo6FT3FUObQtqZUwhVH+uUVFGLY0R+xL4XzQAnDIT9MZ04e4dBv/+ojeQJRGCY = 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 Received: (qmail 15377 invoked by uid 89); 21 May 2019 08:38:22 -0000 Authentication-Results: sourceware.org; auth=none 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 Content-Type: text/plain * 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"); }