[06/13] string: Implement strerror in terms of strerror_l
Commit Message
Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
and s390x-linux-gnu.
---
include/string.h | 4 ++++
string/strerror.c | 22 ++--------------------
string/strerror_l.c | 17 ++++++++++++-----
sysdeps/mach/strerror_l.c | 4 +++-
4 files changed, 21 insertions(+), 26 deletions(-)
Comments
> 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
>
@@ -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. */
@@ -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));
}
@@ -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)
@@ -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