Message ID | 20200519180518.318733-7-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Signal and error list refactoring | expand |
> 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 > <https://www.gnu.org/licenses/>. */ > > -#include <libintl.h> > -#include <stdio.h> > #include <string.h> > -#include <errno.h> > - > -/* 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 <locale/localeinfo.h> > > 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)); > } Why is it okay to share the buffer between strerror and strerror_l? POSIX is a bit unclear about this. Practically speaking, it should be okay. But perhaps there should be a NEWS entry. Thanks, Florian
On 28/05/2020 08:41, Florian Weimer wrote: >> 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 >> <https://www.gnu.org/licenses/>. */ >> >> -#include <libintl.h> >> -#include <stdio.h> >> #include <string.h> >> -#include <errno.h> >> - >> -/* 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 <locale/localeinfo.h> >> >> 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)); >> } > > Why is it okay to share the buffer between strerror and strerror_l? > POSIX is a bit unclear about this. Practically speaking, it should be > okay. But perhaps there should be a NEWS entry. POSIX states the strerror buffer *might* be overwritten by a subsequent call to strerror *or* strerror_l (as an extension to the ISO C). So my understanding is an implementation has the freedom to implement strerror on top of strerror_l and share its internal buffer (bionic and musl seem to share this understanding was well). What about add this NEWS entry at deprecated and removed features: * 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 any symbol. > > Thanks, > Florian >
* Adhemerval Zanella: > POSIX states the strerror buffer *might* be overwritten by a subsequent > call to strerror *or* strerror_l (as an extension to the ISO C). So my > understanding is an implementation has the freedom to implement strerror > on top of strerror_l and share its internal buffer (bionic and musl seem > to share this understanding was well). My recollection is that the wording in POSIX is asymmetric (one invalidates the other, but not the other direction). I don't think that matters, it's probably just an unfortunate wording. > What about add this NEWS entry at deprecated and removed features: > > * 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 any symbol. s/any symbol/either function/? There are some weird spaces before the line break. Thanks, Florian
On 03/06/2020 05:24, Florian Weimer wrote: > * Adhemerval Zanella: > >> POSIX states the strerror buffer *might* be overwritten by a subsequent >> call to strerror *or* strerror_l (as an extension to the ISO C). So my >> understanding is an implementation has the freedom to implement strerror >> on top of strerror_l and share its internal buffer (bionic and musl seem >> to share this understanding was well). > > My recollection is that the wording in POSIX is asymmetric (one > invalidates the other, but not the other direction). I don't think that > matters, it's probably just an unfortunate wording. Ack. > >> What about add this NEWS entry at deprecated and removed features: >> >> * 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 any symbol. > > s/any symbol/either function/? Ack. > There are some weird spaces before the line break. Yeah, I have explicit added them (it is not intended for the NEWS file itself). > > Thanks, > Florian >
diff --git a/include/string.h b/include/string.h index 1d7ec08a57..8efbefd43e 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 <sys/types.h> +#include <locale.h> 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); + /* Called as part of the thread shutdown sequence. */ void __strerror_thread_freeres (void) attribute_hidden; void __strsignal_thread_freeres (void) attribute_hidden; @@ -114,6 +117,7 @@ libc_hidden_proto (memmem) extern __typeof (memmem) __memmem; libc_hidden_proto (__memmem) libc_hidden_proto (__ffs) +libc_hidden_proto (__strerror_l) #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 <https://www.gnu.org/licenses/>. */ -#include <libintl.h> -#include <stdio.h> #include <string.h> -#include <errno.h> - -/* 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 <locale/localeinfo.h> 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)); } diff --git a/string/strerror_l.c b/string/strerror_l.c index 40e7d0e896..cb656b5d54 100644 --- a/string/strerror_l.c +++ b/string/strerror_l.c @@ -20,8 +20,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/param.h> -#include <libc-symbols.h> +#include <errno.h> static __thread char *last_value; @@ -38,8 +37,11 @@ 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 *r; + if (__glibc_unlikely (errnum < 0 || errnum >= _sys_nerr_internal || _sys_errlist_internal[errnum] == NULL)) { @@ -48,11 +50,16 @@ strerror_l (int errnum, locale_t loc) translate ("Unknown error ", loc), errnum) == -1) last_value = NULL; - return last_value; + r = last_value; } + else + r = (char *) translate (_sys_errlist_internal[errnum], loc); - return (char *) translate (_sys_errlist_internal[errnum], loc); + __set_errno (saved_errno); + return r; } +weak_alias (__strerror_l, strerror_l) +libc_hidden_def (__strerror_l) 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) /* This is called when a thread is exiting to free the last_value string. */ void