[PATCH v2 0/4] patches from the morello port

[PATCH v2 0/4] patches from the morello port

[Szabolcs Nagy]

v2 of patches that got feedback.

Szabolcs Nagy (4):
  Fix OOB read in stdlib thousand grouping parsing [BZ #29727]
  Fix off-by-one OOB write in iconv/tst-iconv-mt
  Fix elf/tst-dlmopen-twice not to exhaust static TLS
  Remove unused scratch_buffer_dupfree

 elf/tst-dlmopen-twice.c         |  4 ++--
 iconv/tst-iconv-mt.c            | 15 ++++--------
 include/scratch_buffer.h        | 16 -------------
 malloc/Makefile                 |  1 -
 malloc/scratch_buffer_dupfree.c | 41 ---------------------------------
 malloc/tst-scratch_buffer.c     | 20 ----------------
 stdlib/grouping.c               | 16 ++++++-------
 7 files changed, 14 insertions(+), 99 deletions(-)
 delete mode 100644 malloc/scratch_buffer_dupfree.c

-- 
2.25.1

[PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727]

[Szabolcs Nagy]

__correctly_grouped_prefixmb only worked with thousands_len == 1,
otherwise it read past the end of cp or thousands.

This affects scanf formats like %'d, %'f and the internal but
exposed __strto{l,ul,f,d,..}_internal with grouping flag set
and an LC_NUMERIC locale where thousands_len > 1.

Avoid OOB access by considering thousands_len when initializing cp.
This fixes bug 29727.

Found by the morello port with strict bounds checking where

FAIL: stdlib/tst-strtod4
FAIL: stdlib/tst-strtod5i

crashed using a locale with thousands_len==3.

---
v2: - use const thousands_len for !USE_WIDE_CHAR.
---
 stdlib/grouping.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/stdlib/grouping.c b/stdlib/grouping.c
index be7922f5fd..06cbe7b9c7 100644
--- a/stdlib/grouping.c
+++ b/stdlib/grouping.c
@@ -52,21 +52,19 @@ __correctly_grouped_prefixmb (const STRING_TYPE *begin, const STRING_TYPE *end,
 #endif
 			      const char *grouping)
 {
-#ifndef USE_WIDE_CHAR
-  size_t thousands_len;
-  int cnt;
-#endif
-
   if (grouping == NULL)
     return end;
 
-#ifndef USE_WIDE_CHAR
-  thousands_len = strlen (thousands);
+#ifdef USE_WIDE_CHAR
+  size_t thousands_len = 1;
+#else
+  size_t thousands_len = strlen (thousands);
+  int cnt;
 #endif
 
-  while (end > begin)
+  while (end - begin >= thousands_len)
     {
-      const STRING_TYPE *cp = end - 1;
+      const STRING_TYPE *cp = end - thousands_len;
       const char *gp = grouping;
 
       /* Check first group.  */
-- 
2.25.1

Re: [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727]

[Andreas Schwab]

On Okt 28 2022, Szabolcs Nagy via Libc-alpha wrote:

> __correctly_grouped_prefixmb only worked with thousands_len == 1,
> otherwise it read past the end of cp or thousands.
>
> This affects scanf formats like %'d, %'f and the internal but
> exposed __strto{l,ul,f,d,..}_internal with grouping flag set
> and an LC_NUMERIC locale where thousands_len > 1.
>
> Avoid OOB access by considering thousands_len when initializing cp.
> This fixes bug 29727.
>
> Found by the morello port with strict bounds checking where
>
> FAIL: stdlib/tst-strtod4
> FAIL: stdlib/tst-strtod5i
>
> crashed using a locale with thousands_len==3.

Ok.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH v2 2/4] Fix off-by-one OOB write in iconv/tst-iconv-mt

[Szabolcs Nagy]

The iconv buffer sizes must not include the \0 string terminator.
And the output termination with *outbufpos = '\0' was OOB.

Consistently use non-null-terminated buffer sizes.

---
v2: dropped \0 and replaced strncmp with TEST_COMPARE_BLOB.
---
 iconv/tst-iconv-mt.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c
index daaebd273b..36afd8aed4 100644
--- a/iconv/tst-iconv-mt.c
+++ b/iconv/tst-iconv-mt.c
@@ -57,12 +57,13 @@ worker (void * arg)
   iconv_t cd;
 
   char ascii[] = CONV_INPUT;
+  size_t bytes = sizeof (CONV_INPUT) - 1;
   char *inbufpos = ascii;
-  size_t inbytesleft = sizeof (CONV_INPUT);
+  size_t inbytesleft = bytes;
 
-  char *utf8 = xcalloc (sizeof (CONV_INPUT), 1);
+  char *utf8 = xcalloc (bytes, 1);
   char *outbufpos = utf8;
-  size_t outbytesleft = sizeof (CONV_INPUT);
+  size_t outbytesleft = bytes;
 
   if (tidx < TCOUNT/2)
     /* The first half of the worker thread pool synchronize together here,
@@ -91,8 +92,6 @@ worker (void * arg)
                            &outbytesleft)
                     != (size_t) -1);
 
-  *outbufpos = '\0';
-
   xpthread_barrier_wait (&sync);
 
   TEST_VERIFY_EXIT (iconv_close (cd) == 0);
@@ -104,11 +103,7 @@ worker (void * arg)
   if (tidx < TCOUNT/2)
     xpthread_barrier_wait (&sync);
 
-  if (strncmp (utf8, CONV_INPUT, sizeof CONV_INPUT))
-    {
-      printf ("FAIL: thread %lx: invalid conversion output from iconv\n", tidx);
-      pthread_exit ((void *) (long int) 1);
-    }
+  TEST_COMPARE_BLOB (utf8, bytes, CONV_INPUT, bytes);
 
   pthread_exit (NULL);
 }
-- 
2.25.1

[PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS

[Szabolcs Nagy]

By default glibc only allocates enough static TLS for 4 link namespaces
including the initial one. So only use 3 dlmopens in the test.

---
v2: lowered the dlmopen count from 10 to 3.
---
 elf/tst-dlmopen-twice.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
index 70c71fe19c..a8d7e5e081 100644
--- a/elf/tst-dlmopen-twice.c
+++ b/elf/tst-dlmopen-twice.c
@@ -46,8 +46,8 @@ do_test (void)
   recurse (1);
 
   /* Then with nesting.  The constant needs to be less than the
-     internal DL_NNS namespace constant.  */
-  recurse (10);
+     glibc.rtld.nns tunable (which is 4 by default).  */
+  recurse (3);
   return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> By default glibc only allocates enough static TLS for 4 link namespaces
> including the initial one. So only use 3 dlmopens in the test.
>
> ---
> v2: lowered the dlmopen count from 10 to 3.
> ---
>  elf/tst-dlmopen-twice.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
> index 70c71fe19c..a8d7e5e081 100644
> --- a/elf/tst-dlmopen-twice.c
> +++ b/elf/tst-dlmopen-twice.c
> @@ -46,8 +46,8 @@ do_test (void)
>    recurse (1);
>  
>    /* Then with nesting.  The constant needs to be less than the
> -     internal DL_NNS namespace constant.  */
> -  recurse (10);
> +     glibc.rtld.nns tunable (which is 4 by default).  */
> +  recurse (3);
>    return 0;
>  }

Okay.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian

[PATCH v2 4/4] Remove unused scratch_buffer_dupfree

[Szabolcs Nagy]

Turns out scratch_buffer_dupfree internal API was unused since

commit ef0700004bf0dccf493a5e8e21f71d9e7972ea9f
stdlib: Simplify buffer management in canonicalize

And the related test in malloc/tst-scratch_buffer had issues
so it's better to remove it completely.

---
v2: remove all reference to scratch_buffer_dupfree
---
 include/scratch_buffer.h        | 16 -------------
 malloc/Makefile                 |  1 -
 malloc/scratch_buffer_dupfree.c | 41 ---------------------------------
 malloc/tst-scratch_buffer.c     | 20 ----------------
 4 files changed, 78 deletions(-)
 delete mode 100644 malloc/scratch_buffer_dupfree.c

diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h
index e4c5c8a85d..a9bdcadec2 100644
--- a/include/scratch_buffer.h
+++ b/include/scratch_buffer.h
@@ -132,20 +132,4 @@ scratch_buffer_set_array_size (struct scratch_buffer *buffer,
 			 (buffer, nelem, size));
 }
 
-/* Return a copy of *BUFFER's first SIZE bytes as a heap-allocated block,
-   deallocating *BUFFER if it was heap-allocated.  SIZE must be at
-   most *BUFFER's size.  Return NULL (setting errno) on memory
-   exhaustion.  */
-void *__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer,
-                                     size_t size);
-libc_hidden_proto (__libc_scratch_buffer_dupfree)
-
-/* Alias for __libc_scratch_dupfree.  */
-static __always_inline void *
-scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
-{
-  void *r = __libc_scratch_buffer_dupfree (buffer, size);
-  return __glibc_likely (r != NULL) ? r : NULL;
-}
-
 #endif /* _SCRATCH_BUFFER_H */
diff --git a/malloc/Makefile b/malloc/Makefile
index 4e32de2a0b..211be75e36 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -123,7 +123,6 @@ tests-mcheck = $(filter-out $(tests-exclude-mcheck) $(tests-static), $(tests))
 endif
 
 routines = malloc mcheck mtrace obstack reallocarray \
-  scratch_buffer_dupfree \
   scratch_buffer_grow scratch_buffer_grow_preserve \
   scratch_buffer_set_array_size \
   dynarray_at_failure \
diff --git a/malloc/scratch_buffer_dupfree.c b/malloc/scratch_buffer_dupfree.c
deleted file mode 100644
index eb3b95c1b1..0000000000
--- a/malloc/scratch_buffer_dupfree.c
+++ /dev/null
@@ -1,41 +0,0 @@
-/* Variable-sized buffer with on-stack default allocation.
-   Copyright (C) 2020-2022 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#ifndef _LIBC
-# include <libc-config.h>
-#endif
-
-#include <scratch_buffer.h>
-#include <string.h>
-
-void *
-__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
-{
-  void *data = buffer->data;
-  if (data == buffer->__space.__c)
-    {
-      void *copy = malloc (size);
-      return copy != NULL ? memcpy (copy, data, size) : NULL;
-    }
-  else
-    {
-      void *copy = realloc (data, size);
-      return copy != NULL ? copy : data;
-    }
-}
-libc_hidden_def (__libc_scratch_buffer_dupfree)
diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
index 9fcb11ba2c..1f1e770486 100644
--- a/malloc/tst-scratch_buffer.c
+++ b/malloc/tst-scratch_buffer.c
@@ -151,26 +151,6 @@ do_test (void)
 	  && array_size_must_fail (4, ((size_t)-1) / 4)))
 	return 1;
   }
-  {
-    struct scratch_buffer buf;
-    scratch_buffer_init (&buf);
-    memset (buf.data, '@', buf.length);
-
-    size_t sizes[] = { 16, buf.length, buf.length + 16 };
-    for (int i = 0; i < array_length (sizes); i++)
-      {
-        /* The extra size is unitialized through realloc.  */
-        size_t l = sizes[i] > buf.length ? sizes[i] : buf.length;
-        void *r = scratch_buffer_dupfree (&buf, l);
-        void *c = xmalloc (l);
-        memset (c, '@', l);
-        TEST_COMPARE_BLOB (r, l, buf.data, l);
-        free (r);
-        free (c);
-      }
-
-    scratch_buffer_free (&buf);
-  }
   return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 4/4] Remove unused scratch_buffer_dupfree

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> Turns out scratch_buffer_dupfree internal API was unused since
>
> commit ef0700004bf0dccf493a5e8e21f71d9e7972ea9f
> stdlib: Simplify buffer management in canonicalize
>
> And the related test in malloc/tst-scratch_buffer had issues
> so it's better to remove it completely.
>
> ---
> v2: remove all reference to scratch_buffer_dupfree
> ---
>  include/scratch_buffer.h        | 16 -------------
>  malloc/Makefile                 |  1 -
>  malloc/scratch_buffer_dupfree.c | 41 ---------------------------------
>  malloc/tst-scratch_buffer.c     | 20 ----------------
>  4 files changed, 78 deletions(-)
>  delete mode 100644 malloc/scratch_buffer_dupfree.c

Please also update malloc/Versions.  Rest looks okay.  No need to send
a new version.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian