[2/2] string: strerror, strsignal cannot use buffer after dlmopen (bug 32026)

Secondary namespaces have a different malloc.  Allocating the
buffer in one namespace and freeing it another results in
heap corruption.  Fix this by using a static string (potentially
translated) in secondary namespaces.  It would also be possible
to use the malloc from the initial namespace to manage the
buffer, but these functions would still not be safe to use in
auditors etc. because a call to strerror could still free a
buffer while it is used by the application.  Another approach
could use proper initial-exec TLS, duplicated in secondary
namespaces, but that would need a callback interface for freeing
libc resources in namespaces on thread exit, which does not exist
today.

Tested on x86_64-linux-gnu.

---
 string/strerror_l.c | 35 ++++++++++++++++++++++++-----------
 string/strsignal.c  | 34 ++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 25 deletions(-)

On 27/07/24 06:13, Florian Weimer wrote:
> Secondary namespaces have a different malloc.  Allocating the
> buffer in one namespace and freeing it another results in
> heap corruption.  Fix this by using a static string (potentially
> translated) in secondary namespaces.  It would also be possible
> to use the malloc from the initial namespace to manage the
> buffer, but these functions would still not be safe to use in
> auditors etc. because a call to strerror could still free a
> buffer while it is used by the application.  Another approach
> could use proper initial-exec TLS, duplicated in secondary
> namespaces, but that would need a callback interface for freeing
> libc resources in namespaces on thread exit, which does not exist
> today.
> 
> Tested on x86_64-linux-gnu.

Looks okay, although it also means that all interfaces that potentially
use malloc/free might be broken on auditors.

I also think it needs a regression check.

> 
> ---
>  string/strerror_l.c | 35 ++++++++++++++++++++++++-----------
>  string/strsignal.c  | 34 ++++++++++++++++++++--------------
>  2 files changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/string/strerror_l.c b/string/strerror_l.c
> index 15cce261e6..70456e5bb4 100644
> --- a/string/strerror_l.c
> +++ b/string/strerror_l.c
> @@ -20,7 +20,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <tls-internal.h>
> -
> +#include <libc-internal.h>

Spurious line removal?

>  
>  static const char *
>  translate (const char *str, locale_t loc)
> @@ -31,6 +31,12 @@ translate (const char *str, locale_t loc)
>    return res;
>  }
>  
> +static char *
> +unknown_error (locale_t loc)
> +{
> +  return (char *) translate ("Unknown error", loc);
> +}
> +
>  
>  /* Return a string describing the errno code in ERRNUM.  */
>  char *
> @@ -40,18 +46,25 @@ __strerror_l (int errnum, locale_t loc)
>    char *err = (char *) __get_errlist (errnum);
>    if (__glibc_unlikely (err == NULL))
>      {
> -      struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> -      free (tls_internal->strerror_l_buf);
> -      if (__asprintf (&tls_internal->strerror_l_buf, "%s%d",
> -		      translate ("Unknown error ", loc), errnum) > 0)
> -	err = tls_internal->strerror_l_buf;
> -      else
> +      if (__libc_initial)
>  	{
> -	  /* The memory was freed above.  */
> -	  tls_internal->strerror_l_buf = NULL;
> -	  /* Provide a fallback translation.  */
> -	  err = (char *) translate ("Unknown error", loc);
> +	  struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> +	  free (tls_internal->strerror_l_buf);
> +	  if (__asprintf (&tls_internal->strerror_l_buf, "%s%d",
> +			  translate ("Unknown error ", loc), errnum) > 0)
> +	    err = tls_internal->strerror_l_buf;
> +	  else
> +	    {
> +	      /* The memory was freed above.  */
> +	      tls_internal->strerror_l_buf = NULL;
> +	      /* Provide a fallback translation.  */
> +	      err = unknown_error (loc);
> +	    }
>  	}
> +      else
> +	/* Secondary namespaces use a different malloc, so cannot
> +	   participate in the buffer management.  */
> +	err = unknown_error (loc);
>      }
>    else
>      err = (char *) translate (err, loc);
> diff --git a/string/strsignal.c b/string/strsignal.c
> index 3114601564..808e474e15 100644
> --- a/string/strsignal.c
> +++ b/string/strsignal.c
> @@ -21,6 +21,7 @@
>  #include <string.h>
>  #include <libintl.h>
>  #include <tls-internal.h>
> +#include <libc-internal.h>
>  
>  /* Return a string describing the meaning of the signal number SIGNUM.  */
>  char *
> @@ -30,21 +31,26 @@ strsignal (int signum)
>    if (desc != NULL)
>      return _(desc);
>  
> -  struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> -  free (tls_internal->strsignal_buf);
> +  if (__libc_initial)
> +    {
> +      struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> +      free (tls_internal->strsignal_buf);
>  
> -  int r;
> +      int r;
>  #ifdef SIGRTMIN
> -  if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> -    r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
> -		    signum - SIGRTMIN);
> -  else
> +      if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> +	r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
> +			signum - SIGRTMIN);
> +      else
>  #endif
> -    r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
> -		    signum);
> -
> -  if (r == -1)
> -    tls_internal->strsignal_buf = NULL;
> -
> -  return tls_internal->strsignal_buf;
> +	r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
> +			signum);
> +
> +      if (r >= 0)
> +	return tls_internal->strsignal_buf;
> +    }
> +  /* Fall through on asprintf error, and for !__libc_initial:
> +     secondary namespaces use a different malloc and cannot
> +     participate in the buffer management.  */
> +  return _("Unknown signal");
>  }

* Adhemerval Zanella Netto:

> On 27/07/24 06:13, Florian Weimer wrote:
>> Secondary namespaces have a different malloc.  Allocating the
>> buffer in one namespace and freeing it another results in
>> heap corruption.  Fix this by using a static string (potentially
>> translated) in secondary namespaces.  It would also be possible
>> to use the malloc from the initial namespace to manage the
>> buffer, but these functions would still not be safe to use in
>> auditors etc. because a call to strerror could still free a
>> buffer while it is used by the application.  Another approach
>> could use proper initial-exec TLS, duplicated in secondary
>> namespaces, but that would need a callback interface for freeing
>> libc resources in namespaces on thread exit, which does not exist
>> today.
>> 
>> Tested on x86_64-linux-gnu.
>
> Looks okay, although it also means that all interfaces that potentially
> use malloc/free might be broken on auditors.

The issue here is that this kind of internal TLS isn't replicated along
with libc.  Most other uses are.  We did have a similar issue with
dlerror, which I fixed a while back, by making sure that the right
malloc and free were called.

> I also think it needs a regression check.

I tried to write a regression test, but it's difficult to get a reliable
crash because tcache, fastbins etc. tend to be interoperable between the
mallocs.  The test didn't look very valuable to me.

Should I posted a v2 with the whitespace removal glitch fixed?

Thanks,
Florian

On 02/08/24 10:41, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 27/07/24 06:13, Florian Weimer wrote:
>>> Secondary namespaces have a different malloc.  Allocating the
>>> buffer in one namespace and freeing it another results in
>>> heap corruption.  Fix this by using a static string (potentially
>>> translated) in secondary namespaces.  It would also be possible
>>> to use the malloc from the initial namespace to manage the
>>> buffer, but these functions would still not be safe to use in
>>> auditors etc. because a call to strerror could still free a
>>> buffer while it is used by the application.  Another approach
>>> could use proper initial-exec TLS, duplicated in secondary
>>> namespaces, but that would need a callback interface for freeing
>>> libc resources in namespaces on thread exit, which does not exist
>>> today.
>>>
>>> Tested on x86_64-linux-gnu.
>>
>> Looks okay, although it also means that all interfaces that potentially
>> use malloc/free might be broken on auditors.
> 
> The issue here is that this kind of internal TLS isn't replicated along
> with libc.  Most other uses are.  We did have a similar issue with
> dlerror, which I fixed a while back, by making sure that the right
> malloc and free were called.

Yeah, that's what I referring meaning that we will need extra care on
some interfaces to get them right for auditors.

> 
>> I also think it needs a regression check.
> 
> I tried to write a regression test, but it's difficult to get a reliable
> crash because tcache, fastbins etc. tend to be interoperable between the
> mallocs.  The test didn't look very valuable to me.
>

Fair enough.
 
> Should I posted a v2 with the whitespace removal glitch fixed?

No need.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

* Florian Weimer:

> diff --git a/string/strsignal.c b/string/strsignal.c
> index 3114601564..808e474e15 100644
> --- a/string/strsignal.c
> +++ b/string/strsignal.c
> @@ -21,6 +21,7 @@
>  #include <string.h>
>  #include <libintl.h>
>  #include <tls-internal.h>
> +#include <libc-internal.h>
>  
>  /* Return a string describing the meaning of the signal number SIGNUM.  */
>  char *
> @@ -30,21 +31,26 @@ strsignal (int signum)
>    if (desc != NULL)
>      return _(desc);
>  
> -  struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> -  free (tls_internal->strsignal_buf);
> +  if (__libc_initial)
> +    {
> +      struct tls_internal_t *tls_internal = __glibc_tls_internal ();
> +      free (tls_internal->strsignal_buf);
>  
> -  int r;
> +      int r;
>  #ifdef SIGRTMIN
> -  if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> -    r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
> -		    signum - SIGRTMIN);
> -  else
> +      if (signum >= SIGRTMIN && signum <= SIGRTMAX)
> +	r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
> +			signum - SIGRTMIN);
> +      else
>  #endif
> -    r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
> -		    signum);
> -
> -  if (r == -1)
> -    tls_internal->strsignal_buf = NULL;
> -
> -  return tls_internal->strsignal_buf;
> +	r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
> +			signum);
> +
> +      if (r >= 0)
> +	return tls_internal->strsignal_buf;
> +    }
> +  /* Fall through on asprintf error, and for !__libc_initial:
> +     secondary namespaces use a different malloc and cannot
> +     participate in the buffer management.  */
> +  return _("Unknown signal");
>  }

Sadly this introduces a double-free bug if asprintf fails.
The “if (r == -1) tls_internal->strsignal_buf = NULL;” part was
required.  Will repost a fixed version.

Florian