From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id 249C43851C01 for ; Thu, 2 Jul 2020 19:44:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 249C43851C01 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-29-i9q4Uj7xO4KBh99-cKe_eg-1; Thu, 02 Jul 2020 15:44:35 -0400 X-MC-Unique: i9q4Uj7xO4KBh99-cKe_eg-1 Received: by mail-qt1-f200.google.com with SMTP id h10so6564139qtc.4 for ; Thu, 02 Jul 2020 12:44:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=SlWDmpYIw1yM/KHgFK7BFh4vmiNkAM1pJ6z8pCYdn48=; b=rdNBPJ4hQLFaCd2/WrZuzglL03s7U7iHBeck7IBxoH6GZw6posiTcIA11re8rWDGRQ BKPg6qIpO9MyqwXsL5N/0CRYQO+dRiEJ3RnRRJgm/G1CbSib3IUN5Hgh0Zfo9hklI6eq J1G54xnF3dHTF8c0lneUXmE28Rjs5pFDzPRjsIkDP8LFTUyOXR7REpK80ABIYCaYDr3A C43WTmvUJO9/aZmx3mhn151zQiXee8YUTC7iW43sInQUaTLXhZUQ92V0vphfUSlQRjoR m9pCAvdDGmULhrFGL2E2IX+xhYML2KtjM2fOclOrEYEFgRpu7k+JHfJhXsZvUkU4kRdR 5m3A== X-Gm-Message-State: AOAM5307yL+oN2FO3Fx/Vx/a5TNWoX5IA1VSLvGnRJnLPjWEQGqDKeKH Y0ffWMa4AiqQovJ2tqbLs44Kv6Cou6pPf8qbYoZ0z4OuILyIRk6WIQEAJZ5st/G08zcL2Ab7SXB vmy2WhBaKYcO4/5+z1G/+ X-Received: by 2002:a05:620a:22ae:: with SMTP id p14mr31966223qkh.455.1593719074251; Thu, 02 Jul 2020 12:44:34 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyFfWJFGyPzEj088CF7ce6A0qHdS75AwiZmwfJbMHky52Xa5Op/J5N7ZnEnS64Muu8PlzUmqQ== X-Received: by 2002:a05:620a:22ae:: with SMTP id p14mr31966204qkh.455.1593719073954; Thu, 02 Jul 2020 12:44:33 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id q189sm9260804qkd.57.2020.07.02.12.44.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Jul 2020 12:44:33 -0700 (PDT) Subject: Re: [PATCH v5 06/13] string: Implement strerror in terms of strerror_l To: Adhemerval Zanella , libc-alpha@sourceware.org References: <20200619134352.297146-1-adhemerval.zanella@linaro.org> <20200619134352.297146-6-adhemerval.zanella@linaro.org> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Thu, 2 Jul 2020 15:44:32 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200619134352.297146-6-adhemerval.zanella@linaro.org> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jul 2020 19:44:38 -0000 On 6/19/20 9:43 AM, Adhemerval Zanella wrote: > Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu, > and s390x-linux-gnu. OK for master if you accept the NEWS changes suggested. Please make sure to follow up with the linux man pages project to update their strerror documentation, particularly now that strerror is now MT-Safe, which is a good step forward. Tested-by: Carlos O'Donell Reviewed-by: Carlos O'Donell > --- > NEWS | 4 ++++ > include/string.h | 4 ++++ > string/strerror.c | 22 ++-------------------- > string/strerror_l.c | 15 ++++++++++----- > sysdeps/mach/strerror_l.c | 4 +++- > 5 files changed, 23 insertions(+), 26 deletions(-) > > diff --git a/NEWS b/NEWS > index df03a34657..ec39b6e056 100644 > --- a/NEWS > +++ b/NEWS > @@ -63,6 +63,10 @@ Deprecated and removed features, and other changes affecting compatibility: > compatibility symbols to support old binaries. All programs should use > strerror or strerror_r instead. > > +* Both strerror and strerror_l now share the same internal buffer, meaning > + that the returned string pointer might be invalidated or contents might > + be overwritten in subsequent calls of either symbol. Suggest: * Both strerror and strerror_l now share the same internal buffer in the calling thread, meaning that the returned string pointer may be invalided or contents might be overwritten on subsequent calls in the same thread or if the thread is terminated. Notes: - If the thread is terminated then __libc_thread_freeres will free the storage via __glibc_tls_internal_free. - It is only within the calling thread that this matters. It makes strerror MT-safe. > + > Changes to build and runtime requirements: > > * powerpc64le requires GCC 7.4 or newer. This is required for supporting > diff --git a/include/string.h b/include/string.h > index 4d622f1c03..3a33327cc0 100644 > --- a/include/string.h > +++ b/include/string.h > @@ -4,6 +4,7 @@ > /* Some of these are defined as macros in the real string.h, so we must > prototype them before including it. */ > #include > +#include OK. > > extern void *__memccpy (void *__dest, const void *__src, > int __c, size_t __n); > @@ -50,6 +51,8 @@ extern int __ffs (int __i) __attribute__ ((const)); > > extern char *__strerror_r (int __errnum, char *__buf, size_t __buflen); > > +extern char *__strerror_l (int __errnum, locale_t __loc); > + OK > /* Called as part of the thread shutdown sequence. */ > void __strerror_thread_freeres (void) attribute_hidden; > > @@ -113,6 +116,7 @@ libc_hidden_proto (memmem) > extern __typeof (memmem) __memmem; > libc_hidden_proto (__memmem) > libc_hidden_proto (__ffs) > +libc_hidden_proto (__strerror_l) OK. > > #if IS_IN (libc) > /* Avoid hidden reference to IFUNC symbol __explicit_bzero_chk. */ > diff --git a/string/strerror.c b/string/strerror.c > index 283ab70f91..35c749016e 100644 > --- a/string/strerror.c > +++ b/string/strerror.c > @@ -15,29 +15,11 @@ > License along with the GNU C Library; if not, see > . */ > > -#include > -#include > #include > -#include > - > -/* Return a string describing the errno code in ERRNUM. > - The storage is good only until the next call to strerror. > - Writing to the storage causes undefined behavior. */ > -libc_freeres_ptr (static char *buf); > +#include > > char * > strerror (int errnum) > { > - char *ret = __strerror_r (errnum, NULL, 0); > - int saved_errno; > - > - if (__glibc_likely (ret != NULL)) > - return ret; > - saved_errno = errno; > - if (buf == NULL) > - buf = malloc (1024); > - __set_errno (saved_errno); > - if (buf == NULL) > - return _("Unknown error"); > - return __strerror_r (errnum, buf, 1024); > + return __strerror_l (errnum, __libc_tsd_get (locale_t, LOCALE)); OK. > } > diff --git a/string/strerror_l.c b/string/strerror_l.c > index 309f42e66b..017bd14b99 100644 > --- a/string/strerror_l.c > +++ b/string/strerror_l.c > @@ -20,8 +20,7 @@ > #include > #include > #include > -#include > -#include > +#include > > static __thread char *last_value; > > @@ -38,8 +37,9 @@ translate (const char *str, locale_t loc) > > /* Return a string describing the errno code in ERRNUM. */ > char * > -strerror_l (int errnum, locale_t loc) > +__strerror_l (int errnum, locale_t loc) > { > + int saved_errno = errno; > char *err = (char *) __get_errlist (errnum); > if (__glibc_unlikely (err == NULL)) > { > @@ -48,11 +48,16 @@ strerror_l (int errnum, locale_t loc) > translate ("Unknown error ", loc), errnum) == -1) > last_value = NULL; > > - return last_value; > + err = last_value; > } > + else > + err = (char *) translate (err, loc); > > - return (char *) translate (err, loc); > + __set_errno (saved_errno); > + return err; > } > +weak_alias (__strerror_l, strerror_l) > +libc_hidden_def (__strerror_l) OK. > > void > __strerror_thread_freeres (void) > diff --git a/sysdeps/mach/strerror_l.c b/sysdeps/mach/strerror_l.c > index f514af341e..b6c9fdbe80 100644 > --- a/sysdeps/mach/strerror_l.c > +++ b/sysdeps/mach/strerror_l.c > @@ -42,7 +42,7 @@ translate (const char *str, locale_t loc) > > /* Return a string describing the errno code in ERRNUM. */ > char * > -strerror_l (int errnum, locale_t loc) > +__strerror_l (int errnum, locale_t loc) > { > int system; > int sub; > @@ -86,6 +86,8 @@ strerror_l (int errnum, locale_t loc) > > return (char *) translate (es->subsystem[sub].codes[code], loc); > } > +weak_alias (__strerror_l, strerror_l) > +libc_hidden_def (__strerror_l) OK. > > /* This is called when a thread is exiting to free the last_value string. */ > void > -- Cheers, Carlos.