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

  Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
and s390x-linux-gnu.
---
 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(-)
  

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 <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  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 <sys/types.h>
> +#include <locale.h>

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
>     <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));

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 <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,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
>
  

On 02/07/2020 16:44, Carlos O'Donell wrote:
> 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.

Ack.

> 
> Tested-by: Carlos O'Donell <carlos@redhat.com>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
>> ---
>>  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.

Ack, I just adjusted the line wrap to fit on 78 columns and add
the note it makes the strerror MT-safe:

* 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.  It makes strerror MT-safe.

> 
> 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.

I have add this implementation detail notes on commit message.

> 
>> +
>>  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 <sys/types.h>
>> +#include <locale.h>
> 
> 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
>>     <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));
> 
> 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 <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,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
>>
> 
>