[PATCH 0/3] Reentrancy

[PATCH 0/3] Reentrancy

[Pekka Seppänen]

Hi,

The following patch series provides small (one line) fixes for 
newlib/libc
reentrancy when using --enable-newlib-reent-thread-local and uses the 
existing
infrastructure to do that:

   - There were a few locations on which _errno member was directly 
accessed.
As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
accesses to errno shall be done using the corresponding _REENT_ERRNO() 
macro.
Should be self-explanatory.

   - __getreent() did not check that struct _reent and _impure_ptr were
available.  Both <sys/reent.h> and impure.c, that declare and define
_impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is 
used
the allocated objects are not related to each other.  Therefore 
__getreent()
does not exist even at a concept level, hence should not be provided at 
all.

   - As _Thread_local might not be available (as in that particular 
keyword),
<sys/reent.h> shall include <sys/cdefs.h> that will provide a correct 
mapping,
should the compiler actually support thread-local storage.  A prime 
example is
libstdc++ configure, which uses #include <math.h> in some of the 
generated
checks.  Essentially any C++ target is affected and possibly C targets 
in
near future as C23 uses thread_local, not _Thread_local.

It should be noted that libgloss is affected by similar reentrancy 
issues, but
I decided not to touch those.

-- Pekka

Re: [PATCH 0/3] Reentrancy

[Corinna Vinschen]

Hi Pekka,

your patches don't apply cleanly because your MUA added unwanted
line breaks.

Can you please tell your MUA not to do that and send the patches again?
If that's not possible for some reason, add the patches as plaintext
attachments to a single mail.


Thanks,
Corinna


On Aug 30 12:16, Pekka Seppänen wrote:
> Hi,
> 
> The following patch series provides small (one line) fixes for newlib/libc
> reentrancy when using --enable-newlib-reent-thread-local and uses the
> existing
> infrastructure to do that:
> 
>   - There were a few locations on which _errno member was directly accessed.
> As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
> accesses to errno shall be done using the corresponding _REENT_ERRNO()
> macro.
> Should be self-explanatory.
> 
>   - __getreent() did not check that struct _reent and _impure_ptr were
> available.  Both <sys/reent.h> and impure.c, that declare and define
> _impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is
> used
> the allocated objects are not related to each other.  Therefore __getreent()
> does not exist even at a concept level, hence should not be provided at all.
> 
>   - As _Thread_local might not be available (as in that particular keyword),
> <sys/reent.h> shall include <sys/cdefs.h> that will provide a correct
> mapping,
> should the compiler actually support thread-local storage.  A prime example
> is
> libstdc++ configure, which uses #include <math.h> in some of the generated
> checks.  Essentially any C++ target is affected and possibly C targets in
> near future as C23 uses thread_local, not _Thread_local.
> 
> It should be noted that libgloss is affected by similar reentrancy issues,
> but
> I decided not to touch those.
> 
> -- Pekka

Re: [PATCH 0/3] Reentrancy

[Pekka Seppänen]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On 31.8.2023 12:23, Corinna Vinschen wrote:
> Hi Pekka,
> 
> your patches don't apply cleanly because your MUA added unwanted
> line breaks.
> 
> Can you please tell your MUA not to do that and send the patches again?
> If that's not possible for some reason, add the patches as plaintext
> attachments to a single mail.
> 

My apologies, appears not to be possible until the next distro stable 
version rolls out.  As that might take a while, here are the patches as 
plain text attachments.

-- Pekka

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-reent-errno.patch --]
[-- Type: text/x-diff; name=0001-reent-errno.patch, Size: 4854 bytes --]

Use _REENT_ERRNO() macro to access errno.  This encapsulation is
required, as errno might be either _errno member of struct _reent,
_tls_errno or any such implementation detail.
---
 newlib/libc/locale/locale.c       | 18 +++++++++---------
 newlib/libc/locale/newlocale.c    |  6 +++---
 newlib/libc/stdlib/_mallocr.c     |  2 +-
 newlib/libc/stdlib/nano-mallocr.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/newlib/libc/locale/locale.c b/newlib/libc/locale/locale.c
index 3772106e3..b16ec1511 100644
--- a/newlib/libc/locale/locale.c
+++ b/newlib/libc/locale/locale.c
@@ -317,7 +317,7 @@ _setlocale_r (struct _reent *p,
 
   if (category < LC_ALL || category >= _LC_LAST)
     {
-      p->_errno = EINVAL;
+      _REENT_ERRNO(p) = EINVAL;
       return NULL;
     }
 
@@ -343,7 +343,7 @@ _setlocale_r (struct _reent *p,
 	      env = __get_locale_env (p, i);
 	      if (strlen (env) > ENCODING_LEN)
 		{
-		  p->_errno = EINVAL;
+		  _REENT_ERRNO(p) = EINVAL;
 		  return NULL;
 		}
 	      strcpy (new_categories[i], env);
@@ -354,7 +354,7 @@ _setlocale_r (struct _reent *p,
 	  env = __get_locale_env (p, category);
 	  if (strlen (env) > ENCODING_LEN)
 	    {
-	      p->_errno = EINVAL;
+	      _REENT_ERRNO(p) = EINVAL;
 	      return NULL;
 	    }
 	  strcpy (new_categories[category], env);
@@ -364,7 +364,7 @@ _setlocale_r (struct _reent *p,
     {
       if (strlen (locale) > ENCODING_LEN)
 	{
-	  p->_errno = EINVAL;
+	  _REENT_ERRNO(p) = EINVAL;
 	  return NULL;
 	}
       strcpy (new_categories[category], locale);
@@ -375,7 +375,7 @@ _setlocale_r (struct _reent *p,
 	{
 	  if (strlen (locale) > ENCODING_LEN)
 	    {
-	      p->_errno = EINVAL;
+	      _REENT_ERRNO(p) = EINVAL;
 	      return NULL;
 	    }
 	  for (i = 1; i < _LC_LAST; ++i)
@@ -387,7 +387,7 @@ _setlocale_r (struct _reent *p,
 	    ;
 	  if (!r[1])
 	    {
-	      p->_errno = EINVAL;
+	      _REENT_ERRNO(p) = EINVAL;
 	      return NULL;  /* Hmm, just slashes... */
 	    }
 	  do
@@ -396,7 +396,7 @@ _setlocale_r (struct _reent *p,
 		break;  /* Too many slashes... */
 	      if ((len = r - locale) > ENCODING_LEN)
 		{
-		  p->_errno = EINVAL;
+		  _REENT_ERRNO(p) = EINVAL;
 		  return NULL;
 		}
 	      strlcpy (new_categories[i], locale, len + 1);
@@ -429,7 +429,7 @@ _setlocale_r (struct _reent *p,
       strcpy (saved_categories[i], __get_global_locale ()->categories[i]);
       if (__loadlocale (__get_global_locale (), i, new_categories[i]) == NULL)
 	{
-	  saverr = p->_errno;
+	  saverr = _REENT_ERRNO(p);
 	  for (j = 1; j < i; j++)
 	    {
 	      strcpy (new_categories[j], saved_categories[j]);
@@ -440,7 +440,7 @@ _setlocale_r (struct _reent *p,
 		  __loadlocale (__get_global_locale (), j, new_categories[j]);
 		}
 	    }
-	  p->_errno = saverr;
+	  _REENT_ERRNO(p) = saverr;
 	  return NULL;
 	}
     }
diff --git a/newlib/libc/locale/newlocale.c b/newlib/libc/locale/newlocale.c
index 278f78ed2..92f9e0a9c 100644
--- a/newlib/libc/locale/newlocale.c
+++ b/newlib/libc/locale/newlocale.c
@@ -103,7 +103,7 @@ _newlocale_r (struct _reent *p, int category_mask, const char *locale,
   /* Check for invalid mask values and valid locale ptr. */
   if ((category_mask & ~LC_VALID_MASK) || !locale)
     {
-      p->_errno = EINVAL;
+      _REENT_ERRNO(p) = EINVAL;
       return NULL;
     }
   /* If the new locale is supposed to be all default locale, just return
@@ -125,7 +125,7 @@ _newlocale_r (struct _reent *p, int category_mask, const char *locale,
 						: locale;
 	  if (strlen (cat) > ENCODING_LEN)
 	    {
-	      p->_errno = EINVAL;
+	      _REENT_ERRNO(p) = EINVAL;
 	      return NULL;
 	    }
 	  strcpy (new_categories[i], cat);
@@ -172,7 +172,7 @@ _newlocale_r (struct _reent *p, int category_mask, const char *locale,
 	  /* Otherwise load locale data. */
 	  else if (!__loadlocale (&tmp_locale, i, new_categories[i]))
 	    {
-	      p->_errno = ENOENT;
+	      _REENT_ERRNO(p) = ENOENT;
 	      goto error;
 	    }
 	}
diff --git a/newlib/libc/stdlib/_mallocr.c b/newlib/libc/stdlib/_mallocr.c
index 1997b6db1..c73982c18 100644
--- a/newlib/libc/stdlib/_mallocr.c
+++ b/newlib/libc/stdlib/_mallocr.c
@@ -346,7 +346,7 @@ extern void __malloc_unlock();
 #define RDECL struct _reent *reent_ptr;
 #endif
 
-#define RERRNO reent_ptr->_errno
+#define RERRNO _REENT_ERRNO(reent_ptr)
 #define RCALL reent_ptr,
 #define RONECALL reent_ptr
 
diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
index 41e69abb0..030be44ad 100644
--- a/newlib/libc/stdlib/nano-mallocr.c
+++ b/newlib/libc/stdlib/nano-mallocr.c
@@ -64,7 +64,7 @@
 #define MALLOC_LOCK __malloc_lock(reent_ptr)
 #define MALLOC_UNLOCK __malloc_unlock(reent_ptr)
 
-#define RERRNO reent_ptr->_errno
+#define RERRNO _REENT_ERRNO(reent_ptr)
 
 #define nano_malloc		_malloc_r
 #define nano_free		_free_r
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-reent-getreent.patch --]
[-- Type: text/x-diff; name=0002-reent-getreent.patch, Size: 749 bytes --]

Conditionally provide default __getreent() implementation only if
_REENT_THREAD_LOCAL is not defined.  If struct _reent is replaced by
dedicated thread-local objects neither the structure nor _impure_ptr is
available.
---
 newlib/libc/reent/getreent.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/newlib/libc/reent/getreent.c b/newlib/libc/reent/getreent.c
index 5fa98e96b..86bb04fb9 100644
--- a/newlib/libc/reent/getreent.c
+++ b/newlib/libc/reent/getreent.c
@@ -9,6 +9,8 @@ int _dummy_getreent;
 #include <_ansi.h>
 #include <reent.h>
 
+#ifndef _REENT_THREAD_LOCAL
+
 #ifdef __getreent
 #undef __getreent
 #endif
@@ -19,4 +21,6 @@ __getreent (void)
   return _impure_ptr;
 }
 
+#endif /* !_REENT_THREAD_LOCAL */
+
 #endif
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-reent-threadlocal.patch --]
[-- Type: text/x-diff; name=0003-reent-threadlocal.patch, Size: 599 bytes --]

Attempt to always provide _Thread_local in <sys/reent.h> by including
<sys/cdefs.h>.  The C specific keyword _Thread_local is not available
unless targetting a suitable C version.
---
 newlib/libc/include/sys/reent.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index a6c923f1c..a02e7c2bd 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -12,6 +12,7 @@ extern "C" {
 
 #include <_ansi.h>
 #include <stddef.h>
+#include <sys/cdefs.h>
 #include <sys/_types.h>
 
 #define _NULL 0
-- 
2.34.1

Re: [PATCH 0/3] Reentrancy

[Corinna Vinschen]

Hi Sebastian,

given you're the one pushing the thread_local change in the first
place, would you mind to review this, too?  Looks good to me, but
I'd like to have your input on this.

If you think the patches are ok, feel free to push them.


Thanks,
Corinna


On Aug 30 12:16, Pekka Seppänen wrote:
> Hi,
> 
> The following patch series provides small (one line) fixes for newlib/libc
> reentrancy when using --enable-newlib-reent-thread-local and uses the
> existing
> infrastructure to do that:
> 
>   - There were a few locations on which _errno member was directly accessed.
> As struct _reent is not available if _REENT_THREAD_LOCAL is defined, any
> accesses to errno shall be done using the corresponding _REENT_ERRNO()
> macro.
> Should be self-explanatory.
> 
>   - __getreent() did not check that struct _reent and _impure_ptr were
> available.  Both <sys/reent.h> and impure.c, that declare and define
> _impure_ptr, use similar #ifdef/ifndef gate.  If thread-local storage is
> used
> the allocated objects are not related to each other.  Therefore __getreent()
> does not exist even at a concept level, hence should not be provided at all.
> 
>   - As _Thread_local might not be available (as in that particular keyword),
> <sys/reent.h> shall include <sys/cdefs.h> that will provide a correct
> mapping,
> should the compiler actually support thread-local storage.  A prime example
> is
> libstdc++ configure, which uses #include <math.h> in some of the generated
> checks.  Essentially any C++ target is affected and possibly C targets in
> near future as C23 uses thread_local, not _Thread_local.
> 
> It should be noted that libgloss is affected by similar reentrancy issues,
> but
> I decided not to touch those.
> 
> -- Pekka

Re: [PATCH 0/3] Reentrancy

[Sebastian Huber]

Hello Pekka,

thanks for the fixes. I reviewed the changes and checked them in.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/