diff mbox series

[06/13] string: Implement strerror in terms of strerror_l

Message ID 20200519180518.318733-7-adhemerval.zanella@linaro.org
State New
Headers show
Series Signal and error list refactoring | expand

Commit Message

Adhemerval Zanella May 19, 2020, 6:05 p.m. UTC
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

Florian Weimer May 28, 2020, 11:41 a.m. UTC | #1
> 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
Adhemerval Zanella June 1, 2020, 6:28 p.m. UTC | #2
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
>
Florian Weimer June 3, 2020, 8:24 a.m. UTC | #3
* 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
diff mbox series

Patch

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