[PATCH 00/20] patches from the morello port

[PATCH 00/20] patches from the morello port

[Szabolcs Nagy]

These are patches from the current arm/morello/main branch that i think
can be included into master.

Carlos Eduardo Seo (1):
  malloc: Use uintptr_t for pointer alignment

Szabolcs Nagy (19):
  Fix OOB read in stdlib thousand grouping parsing [BZ #29727]
  scripts: Use bool in tunables initializer
  aarch64: Don't build wordcopy
  aarch64: Fix the extension header write in getcontext and swapcontext
  Fix invalid pointer dereference in wcscpy_chk
  Fix invalid pointer dereference in wcpcpy_chk
  Use uintptr_t in fts for pointer alignment
  malloc: Use uintptr_t in alloc_buffer
  malloc: Fix alignment logic in obstack
  elf: Fix alloca size in _dl_debug_vdprintf
  Fix the symbolic link of multilib dirs
  Use uintptr_t in string/tester for pointer alignment
  Fix off-by-one OOB write in iconv/tst-iconv-mt
  Fix off-by-one OOB read in elf/tst-tls20
  Fix malloc/tst-scratch_buffer OOB access
  Fix missing NUL terminator in stdio-common/scanf13 test
  Fix elf/tst-dlmopen-twice to support enough link namespaces
  Fix resource/bug-ulimit1 test
  Fix stdlib/test-dlclose-exit-race to not hang

 Makerules                                     |  2 +-
 debug/wcpcpy_chk.c                            |  3 +-
 debug/wcscpy_chk.c                            | 34 ++++---------------
 elf/Makefile                                  |  1 +
 elf/dl-printf.c                               |  7 ++--
 elf/tst-dlmopen-twice.c                       |  2 +-
 elf/tst-tls20.c                               |  4 +--
 iconv/tst-iconv-mt.c                          |  4 +--
 include/alloc_buffer.h                        | 10 +++---
 io/fts.c                                      |  3 +-
 malloc/alloc_buffer_alloc_array.c             |  6 ++--
 malloc/arena.c                                |  6 ++--
 malloc/obstack.h                              | 19 ++---------
 malloc/tst-scratch_buffer.c                   | 22 ++++--------
 resource/bug-ulimit1.c                        |  2 +-
 scripts/gen-tunables.awk                      |  2 +-
 stdio-common/scanf13.c                        |  1 +
 stdlib/grouping.c                             |  8 +++++
 stdlib/test-dlclose-exit-race.c               |  4 ++-
 string/tester.c                               | 13 +++----
 sysdeps/aarch64/wordcopy.c                    |  0
 sysdeps/unix/sysv/linux/aarch64/getcontext.S  |  4 +--
 sysdeps/unix/sysv/linux/aarch64/swapcontext.S |  4 +--
 23 files changed, 68 insertions(+), 93 deletions(-)
 create mode 100644 sysdeps/aarch64/wordcopy.c

-- 
2.25.1

[PATCH 01/20] 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.
---
 stdlib/grouping.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/stdlib/grouping.c b/stdlib/grouping.c
index be7922f5fd..4622897488 100644
--- a/stdlib/grouping.c
+++ b/stdlib/grouping.c
@@ -64,9 +64,17 @@ __correctly_grouped_prefixmb (const STRING_TYPE *begin, const STRING_TYPE *end,
   thousands_len = strlen (thousands);
 #endif
 
+#ifdef USE_WIDE_CHAR
   while (end > begin)
+#else
+  while (end - begin >= thousands_len)
+#endif
     {
+#ifdef USE_WIDE_CHAR
       const STRING_TYPE *cp = end - 1;
+#else
+      const STRING_TYPE *cp = end - thousands_len;
+#endif
       const char *gp = grouping;
 
       /* Check first group.  */
-- 
2.25.1

[PATCH 02/20] scripts: Use bool in tunables initializer

[Szabolcs Nagy]

The initializer for a tunable_t set the bool initialized flag to NULL.
This causes a build failure when pointer to bool conversion warns.
---
 scripts/gen-tunables.awk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index fa63e86d1a..d6de100df0 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -177,7 +177,7 @@ END {
     n = indices[2];
     m = indices[3];
     printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
-    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
+    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
 	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
 	    default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
   }
-- 
2.25.1

[PATCH 03/20] aarch64: Don't build wordcopy

[Szabolcs Nagy]

Use an empty wordcopy.c to avoid building the generic one.
It does not seem to be used anywhere.
---
 sysdeps/aarch64/wordcopy.c | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 sysdeps/aarch64/wordcopy.c

diff --git a/sysdeps/aarch64/wordcopy.c b/sysdeps/aarch64/wordcopy.c
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.25.1

[PATCH 04/20] aarch64: Fix the extension header write in getcontext and swapcontext

[Szabolcs Nagy]

The extension header is two 32bit words and in the last header both
should be 0. There is plenty space in the __reserved area, but it's
better not to write more than we mean to.
---
 sysdeps/unix/sysv/linux/aarch64/getcontext.S  | 4 ++--
 sysdeps/unix/sysv/linux/aarch64/swapcontext.S | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/aarch64/getcontext.S b/sysdeps/unix/sysv/linux/aarch64/getcontext.S
index 15fbd2d1a0..3109cbd99a 100644
--- a/sysdeps/unix/sysv/linux/aarch64/getcontext.S
+++ b/sysdeps/unix/sysv/linux/aarch64/getcontext.S
@@ -86,8 +86,8 @@ ENTRY(__getcontext)
 	/* Write the termination context extension header.  */
 	add	x2, x2, #FPSIMD_CONTEXT_SIZE
 
-	str	xzr, [x2, #oHEAD + oMAGIC]
-	str	xzr, [x2, #oHEAD + oSIZE]
+	str	wzr, [x2, #oHEAD + oMAGIC]
+	str	wzr, [x2, #oHEAD + oSIZE]
 
 	/* Grab the signal mask */
 	/* rt_sigprocmask (SIG_BLOCK, NULL, &ucp->uc_sigmask, _NSIG8) */
diff --git a/sysdeps/unix/sysv/linux/aarch64/swapcontext.S b/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
index 1ee2e40272..e3ec9da35d 100644
--- a/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
+++ b/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
@@ -75,8 +75,8 @@ ENTRY(__swapcontext)
 	/* Write the termination context extension header.  */
 	add	x2, x2, #FPSIMD_CONTEXT_SIZE
 
-	str	xzr, [x2, #oHEAD + oMAGIC]
-	str	xzr, [x2, #oHEAD + oSIZE]
+	str	wzr, [x2, #oHEAD + oMAGIC]
+	str	wzr, [x2, #oHEAD + oSIZE]
 
 	/* Preserve ucp.  */
 	mov	x21, x1
-- 
2.25.1

[PATCH 05/20] Fix invalid pointer dereference in wcscpy_chk

[Szabolcs Nagy]

The src pointer is const and points to a different object, so accessing
dest via src is invalid.
---
 debug/wcscpy_chk.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/debug/wcscpy_chk.c b/debug/wcscpy_chk.c
index 8ef03f81e4..d2dc769181 100644
--- a/debug/wcscpy_chk.c
+++ b/debug/wcscpy_chk.c
@@ -24,36 +24,16 @@ wchar_t *
 __wcscpy_chk (wchar_t *dest, const wchar_t *src, size_t n)
 {
   wint_t c;
-  wchar_t *wcp;
+  wchar_t *wcp = dest;
 
-  if (__alignof__ (wchar_t) >= sizeof (wchar_t))
+  do
     {
-      const ptrdiff_t off = dest - src - 1;
-
-      wcp = (wchar_t *) src;
-
-      do
-	{
-	  if (__glibc_unlikely (n-- == 0))
-	    __chk_fail ();
-	  c = *wcp++;
-	  wcp[off] = c;
-	}
-      while (c != L'\0');
-    }
-  else
-    {
-      wcp = dest;
-
-      do
-	{
-	  if (__glibc_unlikely (n-- == 0))
-	    __chk_fail ();
-	  c = *src++;
-	  *wcp++ = c;
-	}
-      while (c != L'\0');
+      if (__glibc_unlikely (n-- == 0))
+        __chk_fail ();
+      c = *src++;
+      *wcp++ = c;
     }
+  while (c != L'\0');
 
   return dest;
 }
-- 
2.25.1

[PATCH 06/20] Fix invalid pointer dereference in wcpcpy_chk

[Szabolcs Nagy]

The src pointer is const and points to a different object, so accessing
dest via src is invalid.
---
 debug/wcpcpy_chk.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/debug/wcpcpy_chk.c b/debug/wcpcpy_chk.c
index bc2be43c3e..d44fb479d0 100644
--- a/debug/wcpcpy_chk.c
+++ b/debug/wcpcpy_chk.c
@@ -28,13 +28,12 @@ __wcpcpy_chk (wchar_t *dest, const wchar_t *src, size_t destlen)
 {
   wchar_t *wcp = (wchar_t *) dest - 1;
   wint_t c;
-  const ptrdiff_t off = src - dest + 1;
 
   do
     {
       if (__glibc_unlikely (destlen-- == 0))
 	__chk_fail ();
-      c = wcp[off];
+      c = *src++;
       *++wcp = c;
     }
   while (c != L'\0');
-- 
2.25.1

[PATCH 07/20] Use uintptr_t in fts for pointer alignment

[Szabolcs Nagy]

The code assumed unsigned long can represent pointers.
---
 io/fts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/io/fts.c b/io/fts.c
index 283cf1791a..5a0a47a360 100644
--- a/io/fts.c
+++ b/io/fts.c
@@ -55,6 +55,7 @@ static char sccsid[] = "@(#)fts.c	8.6 (Berkeley) 8/14/94";
 #include <dirent.h>
 #include <errno.h>
 #include <fts.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -67,7 +68,7 @@ static char sccsid[] = "@(#)fts.c	8.6 (Berkeley) 8/14/94";
 #endif
 /* Align P to that size.  */
 #ifndef ALIGN
-#define	ALIGN(p)	(((unsigned long int) (p) + ALIGNBYTES) & ~ALIGNBYTES)
+#define	ALIGN(p)	(((uintptr_t) (p) + ALIGNBYTES) & ~ALIGNBYTES)
 #endif
 
 
-- 
2.25.1

[PATCH 08/20] malloc: Use uintptr_t for pointer alignment

[Szabolcs Nagy]

From: Carlos Eduardo Seo <carlos.seo@arm.com>

Avoid integer casts that assume unsigned long can represent pointers.
---
 malloc/arena.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/malloc/arena.c b/malloc/arena.c
index 074ecbc09f..f381f18371 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -439,7 +439,7 @@ dump_heap (heap_info *heap)
   fprintf (stderr, "Heap %p, size %10lx:\n", heap, (long) heap->size);
   ptr = (heap->ar_ptr != (mstate) (heap + 1)) ?
         (char *) (heap + 1) : (char *) (heap + 1) + sizeof (struct malloc_state);
-  p = (mchunkptr) (((unsigned long) ptr + MALLOC_ALIGN_MASK) &
+  p = (mchunkptr) (((uintptr_t) ptr + MALLOC_ALIGN_MASK) &
                    ~MALLOC_ALIGN_MASK);
   for (;; )
     {
@@ -513,7 +513,7 @@ alloc_new_heap  (size_t size, size_t top_pad, size_t pagesize,
       p1 = (char *) MMAP (0, max_size << 1, PROT_NONE, mmap_flags);
       if (p1 != MAP_FAILED)
         {
-          p2 = (char *) (((unsigned long) p1 + (max_size - 1))
+          p2 = (char *) (((uintptr_t) p1 + (max_size - 1))
                          & ~(max_size - 1));
           ul = p2 - p1;
           if (ul)
@@ -752,7 +752,7 @@ _int_new_arena (size_t size)
 
   /* Set up the top chunk, with proper alignment. */
   ptr = (char *) (a + 1);
-  misalign = (unsigned long) chunk2mem (ptr) & MALLOC_ALIGN_MASK;
+  misalign = (uintptr_t) chunk2mem (ptr) & MALLOC_ALIGN_MASK;
   if (misalign > 0)
     ptr += MALLOC_ALIGNMENT - misalign;
   top (a) = (mchunkptr) ptr;
-- 
2.25.1

[PATCH 09/20] malloc: Use uintptr_t in alloc_buffer

[Szabolcs Nagy]

The values represnt pointers and not sizes. The members of struct
alloc_buffer are already uintptr_t.
---
 include/alloc_buffer.h            | 10 +++++-----
 malloc/alloc_buffer_alloc_array.c |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/alloc_buffer.h b/include/alloc_buffer.h
index be33e8b68c..1c1dbe0a46 100644
--- a/include/alloc_buffer.h
+++ b/include/alloc_buffer.h
@@ -248,9 +248,9 @@ __alloc_buffer_alloc (struct alloc_buffer *buf, size_t size, size_t align)
   if (size == 1 && align == 1)
     return alloc_buffer_alloc_bytes (buf, size);
 
-  size_t current = buf->__alloc_buffer_current;
-  size_t aligned = roundup (current, align);
-  size_t new_current = aligned + size;
+  uintptr_t current = buf->__alloc_buffer_current;
+  uintptr_t aligned = roundup (current, align);
+  uintptr_t new_current = aligned + size;
   if (aligned >= current        /* No overflow in align step.  */
       && new_current >= size    /* No overflow in size computation.  */
       && new_current <= buf->__alloc_buffer_end) /* Room in buffer.  */
@@ -282,8 +282,8 @@ __alloc_buffer_next (struct alloc_buffer *buf, size_t align)
   if (align == 1)
     return (const void *) buf->__alloc_buffer_current;
 
-  size_t current = buf->__alloc_buffer_current;
-  size_t aligned = roundup (current, align);
+  uintptr_t current = buf->__alloc_buffer_current;
+  uintptr_t aligned = roundup (current, align);
   if (aligned >= current        /* No overflow in align step.  */
       && aligned <= buf->__alloc_buffer_end) /* Room in buffer.  */
     {
diff --git a/malloc/alloc_buffer_alloc_array.c b/malloc/alloc_buffer_alloc_array.c
index d8c08d03ea..b5f32bb630 100644
--- a/malloc/alloc_buffer_alloc_array.c
+++ b/malloc/alloc_buffer_alloc_array.c
@@ -23,12 +23,12 @@ void *
 __libc_alloc_buffer_alloc_array (struct alloc_buffer *buf, size_t element_size,
                                  size_t align, size_t count)
 {
-  size_t current = buf->__alloc_buffer_current;
+  uintptr_t current = buf->__alloc_buffer_current;
   /* The caller asserts that align is a power of two.  */
-  size_t aligned = ALIGN_UP (current, align);
+  uintptr_t aligned = ALIGN_UP (current, align);
   size_t size;
   bool overflow = __builtin_mul_overflow (element_size, count, &size);
-  size_t new_current = aligned + size;
+  uintptr_t new_current = aligned + size;
   if (!overflow                /* Multiplication did not overflow.  */
       && aligned >= current    /* No overflow in align step.  */
       && new_current >= size   /* No overflow in size computation.  */
-- 
2.25.1

[PATCH 10/20] malloc: Fix alignment logic in obstack

[Szabolcs Nagy]

If sizeof(ptrdiff_t) < sizeof(void*) the alignment logic was wrong:
incorrectly assumed that base was already sufficiently aligned.

Use more robust alignment logic: this one should work on any target.
Note: this is an installed header so it must be namespace clean and
portable hence it uses unsigned long for the alignment offset.
---
 malloc/obstack.h | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/malloc/obstack.h b/malloc/obstack.h
index 4b01cdfe4d..1cf18e5464 100644
--- a/malloc/obstack.h
+++ b/malloc/obstack.h
@@ -116,22 +116,9 @@
 # define PTR_INT_TYPE ptrdiff_t
 #endif
 
-/* If B is the base of an object addressed by P, return the result of
-   aligning P to the next multiple of A + 1.  B and P must be of type
-   char *.  A + 1 must be a power of 2.  */
-
-#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
-
-/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
-   where pointers can be converted to integers, aligned as integers,
-   and converted back again.  If PTR_INT_TYPE is narrower than a
-   pointer (e.g., the AS/400), play it safe and compute the alignment
-   relative to B.  Otherwise, use the faster strategy of computing the
-   alignment relative to 0.  */
-
-#define __PTR_ALIGN(B, P, A)						      \
-  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
-		P, A)
+/* Align P to the next multiple of A + 1, where A + 1 is a power of 2,
+   A fits into unsigned long and P has type char *.  */
+#define __PTR_ALIGN(B, P, A) ((P) + (-(unsigned long)(P) & (A)))
 
 #include <string.h>
 
-- 
2.25.1

[PATCH 11/20] elf: Fix alloca size in _dl_debug_vdprintf

[Szabolcs Nagy]

The alloca size did not consider the optional width parameter for
padding which could cause buffer underflow. The width is currently used
e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
can be larger than the alloca buffer size on targets where
sizeof(void *) >= 2 * sizeof(unsigned long).

Even if large width is not used on existing targets it is better to fix
the formatting code to avoid surprises.
---
 elf/dl-printf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/elf/dl-printf.c b/elf/dl-printf.c
index 429d2e80c2..00c114002c 100644
--- a/elf/dl-printf.c
+++ b/elf/dl-printf.c
@@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 		/* We use alloca() to allocate the buffer with the most
 		   pessimistic guess for the size.  Using alloca() allows
 		   having more than one integer formatting in a call.  */
-		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
-		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
+		int size = 1 + 3 * sizeof (unsigned long int);
+		if (width + 1 > size)
+		  size = width + 1;
+		char *buf = (char *) alloca (size);
+		char *endp = &buf[size];
 		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
 
 		/* Pad to the width the user specified.  */
-- 
2.25.1

[PATCH 12/20] Fix the symbolic link of multilib dirs

[Szabolcs Nagy]

If dir contains several / then "ln -s . $dir" does not link it to the
current directory. Use the existing rellns.sh script to compute the
correct relative path to .
---
 Makerules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makerules b/Makerules
index 09c0cf8357..e3ab84b63c 100644
--- a/Makerules
+++ b/Makerules
@@ -1002,7 +1002,7 @@ endef
 define make-link-multidir
 $(patsubst %/,cd %,$(objpfx)); \
   $(addprefix $(abspath $(..)scripts/mkinstalldirs) ,$(dir $(multidir))); \
-  $(LN_S) . $(multidir) 2> /dev/null; \
+  $(SHELL) $(abspath $(..)scripts/rellns-sh) . $(multidir) 2> /dev/null; \
   test -L $(multidir)
 endef
 else
-- 
2.25.1

[PATCH 13/20] Use uintptr_t in string/tester for pointer alignment

[Szabolcs Nagy]

The code assumed unsigned long can represent pointers.
---
 string/tester.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/string/tester.c b/string/tester.c
index eed76239f5..ba948c5723 100644
--- a/string/tester.c
+++ b/string/tester.c
@@ -27,6 +27,7 @@
 #endif
 
 #include <errno.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -529,7 +530,7 @@ test_strlen (void)
     char *p;
     for (i=0; i < 0x100; i++)
       {
-	p = (char *) ((unsigned long int)(buf + 0xff) & ~0xff) + i;
+	p = (char *) ((uintptr_t)(buf + 0xff) & ~0xff) + i;
 	strcpy (p, "OK");
 	strcpy (p+3, "BAD/WRONG");
 	check (strlen (p) == 2, 4+i);
@@ -554,7 +555,7 @@ test_strnlen (void)
   char buf[4096];
   for (int i = 0; i < 0x100; ++i)
     {
-      char *p = (char *) ((unsigned long int)(buf + 0xff) & ~0xff) + i;
+      char *p = (char *) ((uintptr_t)(buf + 0xff) & ~0xff) + i;
       strcpy (p, "OK");
       strcpy (p + 3, "BAD/WRONG");
       check (strnlen (p, 100) == 2, 10 + i);
@@ -582,7 +583,7 @@ test_strchr (void)
     char *p;
     for (i=0; i < 0x100; i++)
       {
-	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
+	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
 	strcpy (p, "OK");
 	strcpy (p+3, "BAD/WRONG");
 	check (strchr (p, '/') == NULL, 9+i);
@@ -614,7 +615,7 @@ test_strchrnul (void)
     char *p;
     for (i=0; i < 0x100; i++)
       {
-	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
+	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
 	strcpy (p, "OK");
 	strcpy (p+3, "BAD/WRONG");
 	cp = strchrnul (p, '/');
@@ -643,7 +644,7 @@ test_rawmemchr (void)
     char *p;
     for (i=0; i < 0x100; i++)
       {
-	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
+	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
 	strcpy (p, "OK");
 	strcpy (p+3, "BAD/WRONG");
 	check (rawmemchr (p, 'R') == p+8, 6+i);
@@ -689,7 +690,7 @@ test_strrchr (void)
     char *p;
     for (i=0; i < 0x100; i++)
       {
-	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
+	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
 	strcpy (p, "OK");
 	strcpy (p+3, "BAD/WRONG");
 	check (strrchr (p, '/') == NULL, 9+i);
-- 
2.25.1

[PATCH 14/20] Fix off-by-one OOB write in iconv/tst-iconv-mt

[Szabolcs Nagy]

The iconv buffer sizes must not include the \0 string terminator.

When \0 cannot be part of a valid character encoding glibc iconv
would copy it to the output as expected, but then later the explicit
output termination with *outbufpos = '\0' is out of bounds.
---
 iconv/tst-iconv-mt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c
index daaebd273b..0320885c06 100644
--- a/iconv/tst-iconv-mt.c
+++ b/iconv/tst-iconv-mt.c
@@ -58,11 +58,11 @@ worker (void * arg)
 
   char ascii[] = CONV_INPUT;
   char *inbufpos = ascii;
-  size_t inbytesleft = sizeof (CONV_INPUT);
+  size_t inbytesleft = sizeof (CONV_INPUT) - 1;
 
   char *utf8 = xcalloc (sizeof (CONV_INPUT), 1);
   char *outbufpos = utf8;
-  size_t outbytesleft = sizeof (CONV_INPUT);
+  size_t outbytesleft = sizeof (CONV_INPUT) - 1;
 
   if (tidx < TCOUNT/2)
     /* The first half of the worker thread pool synchronize together here,
-- 
2.25.1

[PATCH 15/20] Fix off-by-one OOB read in elf/tst-tls20

[Szabolcs Nagy]

The int mods[nmods] array on the stack was overread by one.
---
 elf/tst-tls20.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c
index ce4635eeb1..9cebe22a40 100644
--- a/elf/tst-tls20.c
+++ b/elf/tst-tls20.c
@@ -264,7 +264,7 @@ do_test_dependency (void)
 	  xdlclose (moddep);
 	}
 
-      for (int n = 1; n <= nmods; n++)
+      for (int n = 1; n < nmods; n++)
 	if (mods[n] != 0)
 	  unload_mod (n);
     }
@@ -342,7 +342,7 @@ do_test_invalid_dependency (bool bind_now)
 	    xdlclose (moddep);
 	}
 
-      for (int n = 1; n <= nmods; n++)
+      for (int n = 1; n < nmods; n++)
 	if (mods[n] != 0)
 	  unload_mod (n);
     }
-- 
2.25.1

[PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Szabolcs Nagy]

The test used scratch_buffer_dupfree incorrectly:

- The passed in size must be <= buf.length.
- Must be called at most once on a buf object since it frees it.
- After it is called buf.data and buf.length must not be accessed.

All of these were violated, the test happened to work because the
buffer was on the stack, which meant the test copied out-of-bounds
bytes from the stack into a new buffer and then compared those bytes.

Run one test and avoid the issues above.
---
 malloc/tst-scratch_buffer.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
index 9fcb11ba2c..60a513ccc6 100644
--- a/malloc/tst-scratch_buffer.c
+++ b/malloc/tst-scratch_buffer.c
@@ -155,21 +155,13 @@ do_test (void)
     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);
+    size_t l = 16 <= buf.length ? 16 : buf.length;
+    void *r = scratch_buffer_dupfree (&buf, l);
+    void *c = xmalloc (l);
+    memset (c, '@', l);
+    TEST_COMPARE_BLOB (r, l, c, l);
+    free (r);
+    free (c);
   }
   return 0;
 }
-- 
2.25.1

[PATCH 17/20] Fix missing NUL terminator in stdio-common/scanf13 test

[Szabolcs Nagy]

sscanf is only defined on nul terminated string input, but '\0' was
missing in this test which caused _IO_str_init_static_internal to
read OOB on the stack when computing the bounds of the string.
---
 stdio-common/scanf13.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/stdio-common/scanf13.c b/stdio-common/scanf13.c
index 720224aa05..60aa62a26f 100644
--- a/stdio-common/scanf13.c
+++ b/stdio-common/scanf13.c
@@ -67,6 +67,7 @@ main (void)
   buf[2049] = 0x84;
   buf[2058] = '\t';
   buf[2059] = 'a';
+  buf[sizeof (buf) - 1] = '\0';
   if (sscanf (buf, "%ms%mc", &sp1, &sp2) != 2)
     FAIL ();
   else
-- 
2.25.1

[PATCH 18/20] Fix elf/tst-dlmopen-twice to support enough link namespaces

[Szabolcs Nagy]

The test dlmopens 10 namespaces recursively, which requires a glibc
tunable setting, otherwise it may run out of static TLS.
---
 elf/Makefile            | 1 +
 elf/tst-dlmopen-twice.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/elf/Makefile b/elf/Makefile
index 7b50ccc07a..ace15dc11b 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -2969,6 +2969,7 @@ tst-audit-tlsdesc-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
 $(objpfx)tst-audit-tlsdesc-dlopen.out: $(objpfx)tst-auditmod-tlsdesc.so
 tst-audit-tlsdesc-dlopen-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
 
+tst-dlmopen-twice-ENV = GLIBC_TUNABLES=glibc.rtld.nns=10
 $(objpfx)tst-dlmopen-twice.out: \
   $(objpfx)tst-dlmopen-twice-mod1.so \
   $(objpfx)tst-dlmopen-twice-mod2.so
diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
index 70c71fe19c..dfa58b1505 100644
--- a/elf/tst-dlmopen-twice.c
+++ b/elf/tst-dlmopen-twice.c
@@ -46,7 +46,7 @@ do_test (void)
   recurse (1);
 
   /* Then with nesting.  The constant needs to be less than the
-     internal DL_NNS namespace constant.  */
+     glibc.rtld.nns tunable (which is between 1 and DL_NNS).  */
   recurse (10);
   return 0;
 }
-- 
2.25.1

[PATCH 19/20] Fix resource/bug-ulimit1 test

[Szabolcs Nagy]

ulimit is a variadic function and the second argument must have type
long (or unsigned long).
---
 resource/bug-ulimit1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/resource/bug-ulimit1.c b/resource/bug-ulimit1.c
index 334d7fff04..8dd3ddf493 100644
--- a/resource/bug-ulimit1.c
+++ b/resource/bug-ulimit1.c
@@ -7,7 +7,7 @@ main (void)
   int retval = 0;
   long int res;
 
-  res = ulimit (UL_SETFSIZE, 10000);
+  res = ulimit (UL_SETFSIZE, 10000L);
   printf ("Result of ulimit (UL_SETFSIZE, 10000): %ld\n", res);
   if (res != 10000)
     retval = 1;
-- 
2.25.1

[PATCH 20/20] Fix stdlib/test-dlclose-exit-race to not hang

[Szabolcs Nagy]

Use the standard wrapper that kills the test after a timeout.
---
 stdlib/test-dlclose-exit-race.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/stdlib/test-dlclose-exit-race.c b/stdlib/test-dlclose-exit-race.c
index 91613116bb..260f7c9e05 100644
--- a/stdlib/test-dlclose-exit-race.c
+++ b/stdlib/test-dlclose-exit-race.c
@@ -63,7 +63,7 @@ last (void)
 }
 
 int
-main (void)
+do_test (void)
 {
   int value;
   void *dso;
@@ -90,3 +90,5 @@ main (void)
 
   FAIL_EXIT1 ("Did not terminate via exit(0) in exit_thread() as expected.");
 }
+
+#include <support/test-driver.c>
-- 
2.25.1

Re: [PATCH 01/20] Fix OOB read in stdlib thousand grouping parsing [BZ #29727]

[Andreas Schwab]

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

> diff --git a/stdlib/grouping.c b/stdlib/grouping.c
> index be7922f5fd..4622897488 100644
> --- a/stdlib/grouping.c
> +++ b/stdlib/grouping.c
> @@ -64,9 +64,17 @@ __correctly_grouped_prefixmb (const STRING_TYPE *begin, const STRING_TYPE *end,
>    thousands_len = strlen (thousands);
>  #endif
>  
> +#ifdef USE_WIDE_CHAR
>    while (end > begin)
> +#else
> +  while (end - begin >= thousands_len)
> +#endif
>      {
> +#ifdef USE_WIDE_CHAR
>        const STRING_TYPE *cp = end - 1;
> +#else
> +      const STRING_TYPE *cp = end - thousands_len;
> +#endif

This could be simplified by defining a constant thousands_len for the
USE_WIDE_CHAR case.

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

Re: [PATCH 09/20] malloc: Use uintptr_t in alloc_buffer

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The values represnt pointers and not sizes. The members of struct
> alloc_buffer are already uintptr_t.
> ---
>  include/alloc_buffer.h            | 10 +++++-----
>  malloc/alloc_buffer_alloc_array.c |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)

Looks okay.

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

Thanks,
Florian

Re: [PATCH 20/20] Fix stdlib/test-dlclose-exit-race to not hang

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> Use the standard wrapper that kills the test after a timeout.
> ---
>  stdlib/test-dlclose-exit-race.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/stdlib/test-dlclose-exit-race.c b/stdlib/test-dlclose-exit-race.c
> index 91613116bb..260f7c9e05 100644
> --- a/stdlib/test-dlclose-exit-race.c
> +++ b/stdlib/test-dlclose-exit-race.c
> @@ -63,7 +63,7 @@ last (void)
>  }
>  
>  int
> -main (void)
> +do_test (void)
>  {
>    int value;
>    void *dso;
> @@ -90,3 +90,5 @@ main (void)
>  
>    FAIL_EXIT1 ("Did not terminate via exit(0) in exit_thread() as expected.");
>  }
> +
> +#include <support/test-driver.c>

I'm wondering if this interferes with the text objective.  Unfortunately
this wasn't discussed during the original patch review.

Thanks,
Florian

Re: [PATCH 18/20] Fix elf/tst-dlmopen-twice to support enough link namespaces

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The test dlmopens 10 namespaces recursively, which requires a glibc
> tunable setting, otherwise it may run out of static TLS.
> ---
>  elf/Makefile            | 1 +
>  elf/tst-dlmopen-twice.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 7b50ccc07a..ace15dc11b 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -2969,6 +2969,7 @@ tst-audit-tlsdesc-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>  $(objpfx)tst-audit-tlsdesc-dlopen.out: $(objpfx)tst-auditmod-tlsdesc.so
>  tst-audit-tlsdesc-dlopen-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>  
> +tst-dlmopen-twice-ENV = GLIBC_TUNABLES=glibc.rtld.nns=10
>  $(objpfx)tst-dlmopen-twice.out: \
>    $(objpfx)tst-dlmopen-twice-mod1.so \
>    $(objpfx)tst-dlmopen-twice-mod2.so
> diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
> index 70c71fe19c..dfa58b1505 100644
> --- a/elf/tst-dlmopen-twice.c
> +++ b/elf/tst-dlmopen-twice.c
> @@ -46,7 +46,7 @@ do_test (void)
>    recurse (1);
>  
>    /* Then with nesting.  The constant needs to be less than the
> -     internal DL_NNS namespace constant.  */
> +     glibc.rtld.nns tunable (which is between 1 and DL_NNS).  */
>    recurse (10);
>    return 0;
>  }

This doesn't work if configured with --disable-tunables.

I suspect this is merely a symptom.  The static TLS sizing defaults
should allow creating DL_NNS namespaces?

Thanks,
Florian

Re: [PATCH 02/20] scripts: Use bool in tunables initializer

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The initializer for a tunable_t set the bool initialized flag to NULL.
> This causes a build failure when pointer to bool conversion warns.
> ---
>  scripts/gen-tunables.awk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index fa63e86d1a..d6de100df0 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -177,7 +177,7 @@ END {
>      n = indices[2];
>      m = indices[3];
>      printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> -    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
> +    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
>  	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
>  	    default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
>    }

Looks okay.

elf/dl-tunable-types.h has:

| /* A tunable.  */
| struct _tunable
| {
|   const char name[TUNABLE_NAME_MAX];    /* Internal name of the tunable.  */
|   tunable_type_t type;                  /* Data type of the tunable.  */
|   tunable_val_t val;                    /* The value.  */
|   bool initialized;                     /* Flag to indicate that the tunable is
|                                            initialized.  */

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

Thanks,
Florian

Re: [PATCH 18/20] Fix elf/tst-dlmopen-twice to support enough link namespaces

[Szabolcs Nagy]

The 10/27/2022 18:24, Florian Weimer wrote:
> * Szabolcs Nagy via Libc-alpha:
> 
> > The test dlmopens 10 namespaces recursively, which requires a glibc
> > tunable setting, otherwise it may run out of static TLS.
> > ---
> >  elf/Makefile            | 1 +
> >  elf/tst-dlmopen-twice.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/elf/Makefile b/elf/Makefile
> > index 7b50ccc07a..ace15dc11b 100644
> > --- a/elf/Makefile
> > +++ b/elf/Makefile
> > @@ -2969,6 +2969,7 @@ tst-audit-tlsdesc-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
> >  $(objpfx)tst-audit-tlsdesc-dlopen.out: $(objpfx)tst-auditmod-tlsdesc.so
> >  tst-audit-tlsdesc-dlopen-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
> >  
> > +tst-dlmopen-twice-ENV = GLIBC_TUNABLES=glibc.rtld.nns=10
> >  $(objpfx)tst-dlmopen-twice.out: \
> >    $(objpfx)tst-dlmopen-twice-mod1.so \
> >    $(objpfx)tst-dlmopen-twice-mod2.so
> > diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
> > index 70c71fe19c..dfa58b1505 100644
> > --- a/elf/tst-dlmopen-twice.c
> > +++ b/elf/tst-dlmopen-twice.c
> > @@ -46,7 +46,7 @@ do_test (void)
> >    recurse (1);
> >  
> >    /* Then with nesting.  The constant needs to be less than the
> > -     internal DL_NNS namespace constant.  */
> > +     glibc.rtld.nns tunable (which is between 1 and DL_NNS).  */
> >    recurse (10);
> >    return 0;
> >  }
> 
> This doesn't work if configured with --disable-tunables.
> 
> I suspect this is merely a symptom.  The static TLS sizing defaults
> should allow creating DL_NNS namespaces?

the tunable defaults to 4, DL_NNS is 16 and that's the max value
of the tunable (since that's the static namespace array size)

Re: [PATCH 18/20] Fix elf/tst-dlmopen-twice to support enough link namespaces

[Adhemerval Zanella Netto]

On 27/10/22 13:24, Florian Weimer via Libc-alpha wrote:
> * Szabolcs Nagy via Libc-alpha:
> 
>> The test dlmopens 10 namespaces recursively, which requires a glibc
>> tunable setting, otherwise it may run out of static TLS.
>> ---
>>  elf/Makefile            | 1 +
>>  elf/tst-dlmopen-twice.c | 2 +-
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 7b50ccc07a..ace15dc11b 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -2969,6 +2969,7 @@ tst-audit-tlsdesc-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>>  $(objpfx)tst-audit-tlsdesc-dlopen.out: $(objpfx)tst-auditmod-tlsdesc.so
>>  tst-audit-tlsdesc-dlopen-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>>  
>> +tst-dlmopen-twice-ENV = GLIBC_TUNABLES=glibc.rtld.nns=10
>>  $(objpfx)tst-dlmopen-twice.out: \
>>    $(objpfx)tst-dlmopen-twice-mod1.so \
>>    $(objpfx)tst-dlmopen-twice-mod2.so
>> diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
>> index 70c71fe19c..dfa58b1505 100644
>> --- a/elf/tst-dlmopen-twice.c
>> +++ b/elf/tst-dlmopen-twice.c
>> @@ -46,7 +46,7 @@ do_test (void)
>>    recurse (1);
>>  
>>    /* Then with nesting.  The constant needs to be less than the
>> -     internal DL_NNS namespace constant.  */
>> +     glibc.rtld.nns tunable (which is between 1 and DL_NNS).  */
>>    recurse (10);
>>    return 0;
>>  }
> 
> This doesn't work if configured with --disable-tunables.
> 
> I suspect this is merely a symptom.  The static TLS sizing defaults
> should allow creating DL_NNS namespaces?

Another question would be whether if we should keep --disable-tunables option.

Re: [PATCH 19/20] Fix resource/bug-ulimit1 test

[Adhemerval Zanella Netto]

On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> ulimit is a variadic function and the second argument must have type
> long (or unsigned long).


LGTM, thanks.

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

> ---
>  resource/bug-ulimit1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/resource/bug-ulimit1.c b/resource/bug-ulimit1.c
> index 334d7fff04..8dd3ddf493 100644
> --- a/resource/bug-ulimit1.c
> +++ b/resource/bug-ulimit1.c
> @@ -7,7 +7,7 @@ main (void)
>    int retval = 0;
>    long int res;
>  
> -  res = ulimit (UL_SETFSIZE, 10000);
> +  res = ulimit (UL_SETFSIZE, 10000L);
>    printf ("Result of ulimit (UL_SETFSIZE, 10000): %ld\n", res);
>    if (res != 10000)
>      retval = 1;

Re: [PATCH 18/20] Fix elf/tst-dlmopen-twice to support enough link namespaces

[Florian Weimer]

* Szabolcs Nagy:

> The 10/27/2022 18:24, Florian Weimer wrote:
>> * Szabolcs Nagy via Libc-alpha:
>> 
>> > The test dlmopens 10 namespaces recursively, which requires a glibc
>> > tunable setting, otherwise it may run out of static TLS.
>> > ---
>> >  elf/Makefile            | 1 +
>> >  elf/tst-dlmopen-twice.c | 2 +-
>> >  2 files changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/elf/Makefile b/elf/Makefile
>> > index 7b50ccc07a..ace15dc11b 100644
>> > --- a/elf/Makefile
>> > +++ b/elf/Makefile
>> > @@ -2969,6 +2969,7 @@ tst-audit-tlsdesc-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>> >  $(objpfx)tst-audit-tlsdesc-dlopen.out: $(objpfx)tst-auditmod-tlsdesc.so
>> >  tst-audit-tlsdesc-dlopen-ENV = LD_AUDIT=$(objpfx)tst-auditmod-tlsdesc.so
>> >  
>> > +tst-dlmopen-twice-ENV = GLIBC_TUNABLES=glibc.rtld.nns=10
>> >  $(objpfx)tst-dlmopen-twice.out: \
>> >    $(objpfx)tst-dlmopen-twice-mod1.so \
>> >    $(objpfx)tst-dlmopen-twice-mod2.so
>> > diff --git a/elf/tst-dlmopen-twice.c b/elf/tst-dlmopen-twice.c
>> > index 70c71fe19c..dfa58b1505 100644
>> > --- a/elf/tst-dlmopen-twice.c
>> > +++ b/elf/tst-dlmopen-twice.c
>> > @@ -46,7 +46,7 @@ do_test (void)
>> >    recurse (1);
>> >  
>> >    /* Then with nesting.  The constant needs to be less than the
>> > -     internal DL_NNS namespace constant.  */
>> > +     glibc.rtld.nns tunable (which is between 1 and DL_NNS).  */
>> >    recurse (10);
>> >    return 0;
>> >  }
>> 
>> This doesn't work if configured with --disable-tunables.
>> 
>> I suspect this is merely a symptom.  The static TLS sizing defaults
>> should allow creating DL_NNS namespaces?
>
> the tunable defaults to 4, DL_NNS is 16 and that's the max value
> of the tunable (since that's the static namespace array size)

Maybe we should lower the constant from 10 to 3 then?  I mistakenly
assumed the namespace count was higher than it actually is.  3 should
not yet invalidate the test.

Thanks,
Florian

Re: [PATCH 03/20] aarch64: Don't build wordcopy

[Adhemerval Zanella Netto]

On 27/10/22 12:32, Szabolcs Nagy via Libc-alpha wrote:
> Use an empty wordcopy.c to avoid building the generic one.
> It does not seem to be used anywhere.

LGTM, thanks.  I would be good if we could refactor to avoid each target that
reimplement memcpy/memmove with assembly routine to need to add this empty
file.

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

> ---
>  sysdeps/aarch64/wordcopy.c | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 sysdeps/aarch64/wordcopy.c
> 
> diff --git a/sysdeps/aarch64/wordcopy.c b/sysdeps/aarch64/wordcopy.c
> new file mode 100644
> index 0000000000..e69de29bb2

Re: [PATCH 11/20] elf: Fix alloca size in _dl_debug_vdprintf

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The alloca size did not consider the optional width parameter for
> padding which could cause buffer underflow. The width is currently used
> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> can be larger than the alloca buffer size on targets where
> sizeof(void *) >= 2 * sizeof(unsigned long).
>
> Even if large width is not used on existing targets it is better to fix
> the formatting code to avoid surprises.
> ---
>  elf/dl-printf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> index 429d2e80c2..00c114002c 100644
> --- a/elf/dl-printf.c
> +++ b/elf/dl-printf.c
> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>  		/* We use alloca() to allocate the buffer with the most
>  		   pessimistic guess for the size.  Using alloca() allows
>  		   having more than one integer formatting in a call.  */
> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> +		int size = 1 + 3 * sizeof (unsigned long int);
> +		if (width + 1 > size)
> +		  size = width + 1;
> +		char *buf = (char *) alloca (size);
> +		char *endp = &buf[size];
>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>  
>  		/* Pad to the width the user specified.  */

This looks okay.

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

Thanks,
Florian

Re: [PATCH 05/20] Fix invalid pointer dereference in wcscpy_chk

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The src pointer is const and points to a different object, so accessing
> dest via src is invalid.
> ---
>  debug/wcscpy_chk.c | 34 +++++++---------------------------
>  1 file changed, 7 insertions(+), 27 deletions(-)
>
> diff --git a/debug/wcscpy_chk.c b/debug/wcscpy_chk.c
> index 8ef03f81e4..d2dc769181 100644
> --- a/debug/wcscpy_chk.c
> +++ b/debug/wcscpy_chk.c
> @@ -24,36 +24,16 @@ wchar_t *
>  __wcscpy_chk (wchar_t *dest, const wchar_t *src, size_t n)
>  {
>    wint_t c;
> +  wchar_t *wcp = dest;
>  
> +  do
>      {
> +      if (__glibc_unlikely (n-- == 0))
> +        __chk_fail ();
> +      c = *src++;
> +      *wcp++ = c;
>      }
> +  while (c != L'\0');
>  
>    return dest;
>  }

Seems fine.

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

Thanks,
Florian

Re: [PATCH 15/20] Fix off-by-one OOB read in elf/tst-tls20

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The int mods[nmods] array on the stack was overread by one.
> ---
>  elf/tst-tls20.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/elf/tst-tls20.c b/elf/tst-tls20.c
> index ce4635eeb1..9cebe22a40 100644
> --- a/elf/tst-tls20.c
> +++ b/elf/tst-tls20.c
> @@ -264,7 +264,7 @@ do_test_dependency (void)
>  	  xdlclose (moddep);
>  	}
>  
> -      for (int n = 1; n <= nmods; n++)
> +      for (int n = 1; n < nmods; n++)
>  	if (mods[n] != 0)
>  	  unload_mod (n);
>      }
> @@ -342,7 +342,7 @@ do_test_invalid_dependency (bool bind_now)
>  	    xdlclose (moddep);
>  	}
>  
> -      for (int n = 1; n <= nmods; n++)
> +      for (int n = 1; n < nmods; n++)
>  	if (mods[n] != 0)
>  	  unload_mod (n);
>      }

Looks good.

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

Thanks,
Florian

Re: [PATCH 14/20] Fix off-by-one OOB write in iconv/tst-iconv-mt

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The iconv buffer sizes must not include the \0 string terminator.
>
> When \0 cannot be part of a valid character encoding glibc iconv
> would copy it to the output as expected, but then later the explicit
> output termination with *outbufpos = '\0' is out of bounds.
> ---
>  iconv/tst-iconv-mt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/iconv/tst-iconv-mt.c b/iconv/tst-iconv-mt.c
> index daaebd273b..0320885c06 100644
> --- a/iconv/tst-iconv-mt.c
> +++ b/iconv/tst-iconv-mt.c
> @@ -58,11 +58,11 @@ worker (void * arg)
>  
>    char ascii[] = CONV_INPUT;
>    char *inbufpos = ascii;
> -  size_t inbytesleft = sizeof (CONV_INPUT);
> +  size_t inbytesleft = sizeof (CONV_INPUT) - 1;
>  
>    char *utf8 = xcalloc (sizeof (CONV_INPUT), 1);
>    char *outbufpos = utf8;
> -  size_t outbytesleft = sizeof (CONV_INPUT);
> +  size_t outbytesleft = sizeof (CONV_INPUT) - 1;
>  
>    if (tidx < TCOUNT/2)
>      /* The first half of the worker thread pool synchronize together here,

I would prefer to remove the null terminator and replace strncmp with
TEST_COMPARE_BLOB.

Thanks,
Florian

Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

 test used scratch_buffer_dupfree incorrectly:
>
> - The passed in size must be <= buf.length.
> - Must be called at most once on a buf object since it frees it.
> - After it is called buf.data and buf.length must not be accessed.
>
> All of these were violated, the test happened to work because the
> buffer was on the stack, which meant the test copied out-of-bounds
> bytes from the stack into a new buffer and then compared those bytes.
>
> Run one test and avoid the issues above.
> ---
>  malloc/tst-scratch_buffer.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
> index 9fcb11ba2c..60a513ccc6 100644
> --- a/malloc/tst-scratch_buffer.c
> +++ b/malloc/tst-scratch_buffer.c
> @@ -155,21 +155,13 @@ do_test (void)
>      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);
> +    size_t l = 16 <= buf.length ? 16 : buf.length;
> +    void *r = scratch_buffer_dupfree (&buf, l);
> +    void *c = xmalloc (l);
> +    memset (c, '@', l);
> +    TEST_COMPARE_BLOB (r, l, c, l);
> +    free (r);
> +    free (c);
>    }
>    return 0;
>  }

I think we should keep the test loop, but create a new scratch buffer on
each iteration.

Thanks,
Florian

Re: [PATCH 17/20] Fix missing NUL terminator in stdio-common/scanf13 test

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> sscanf is only defined on nul terminated string input, but '\0' was
> missing in this test which caused _IO_str_init_static_internal to
> read OOB on the stack when computing the bounds of the string.
> ---
>  stdio-common/scanf13.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/stdio-common/scanf13.c b/stdio-common/scanf13.c
> index 720224aa05..60aa62a26f 100644
> --- a/stdio-common/scanf13.c
> +++ b/stdio-common/scanf13.c
> @@ -67,6 +67,7 @@ main (void)
>    buf[2049] = 0x84;
>    buf[2058] = '\t';
>    buf[2059] = 'a';
> +  buf[sizeof (buf) - 1] = '\0';
>    if (sscanf (buf, "%ms%mc", &sp1, &sp2) != 2)
>      FAIL ();
>    else

We want this test to be correct (see bug 17577), but for now this is the
right change.

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

Thanks,
Florian

Re: [PATCH 06/20] Fix invalid pointer dereference in wcpcpy_chk

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The src pointer is const and points to a different object, so accessing
> dest via src is invalid.
> ---
>  debug/wcpcpy_chk.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/debug/wcpcpy_chk.c b/debug/wcpcpy_chk.c
> index bc2be43c3e..d44fb479d0 100644
> --- a/debug/wcpcpy_chk.c
> +++ b/debug/wcpcpy_chk.c
> @@ -28,13 +28,12 @@ __wcpcpy_chk (wchar_t *dest, const wchar_t *src, size_t destlen)
>  {
>    wchar_t *wcp = (wchar_t *) dest - 1;
>    wint_t c;
> -  const ptrdiff_t off = src - dest + 1;
>  
>    do
>      {
>        if (__glibc_unlikely (destlen-- == 0))
>  	__chk_fail ();
> -      c = wcp[off];
> +      c = *src++;
>        *++wcp = c;
>      }
>    while (c != L'\0');

Looks reasonable.

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

Thanks,
Florian

Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Szabolcs Nagy]

The 10/28/2022 07:41, Florian Weimer wrote:
> * Szabolcs Nagy via Libc-alpha:
> 
>  test used scratch_buffer_dupfree incorrectly:
> >
> > - The passed in size must be <= buf.length.
> > - Must be called at most once on a buf object since it frees it.
> > - After it is called buf.data and buf.length must not be accessed.
> >
> > All of these were violated, the test happened to work because the
> > buffer was on the stack, which meant the test copied out-of-bounds
> > bytes from the stack into a new buffer and then compared those bytes.
> >
> > Run one test and avoid the issues above.
> > ---
> >  malloc/tst-scratch_buffer.c | 22 +++++++---------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/malloc/tst-scratch_buffer.c b/malloc/tst-scratch_buffer.c
> > index 9fcb11ba2c..60a513ccc6 100644
> > --- a/malloc/tst-scratch_buffer.c
> > +++ b/malloc/tst-scratch_buffer.c
> > @@ -155,21 +155,13 @@ do_test (void)
> >      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);
> > +    size_t l = 16 <= buf.length ? 16 : buf.length;
> > +    void *r = scratch_buffer_dupfree (&buf, l);
> > +    void *c = xmalloc (l);
> > +    memset (c, '@', l);
> > +    TEST_COMPARE_BLOB (r, l, c, l);
> > +    free (r);
> > +    free (c);
> >    }
> >    return 0;
> >  }
> 
> I think we should keep the test loop, but create a new scratch buffer on
> each iteration.

given the documentation of scratch_buffer_dupfree
i don't see how the test supposed to work with
sizes > buf.length or what's the point of this loop.

Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Florian Weimer]

* Szabolcs Nagy:

>> I think we should keep the test loop, but create a new scratch buffer on
>> each iteration.
>
> given the documentation of scratch_buffer_dupfree
> i don't see how the test supposed to work with
> sizes > buf.length or what's the point of this loop.

Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
patch, or do you want to do it?

Thanks,
Florian

Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Szabolcs Nagy]

The 10/28/2022 13:30, Florian Weimer via Libc-alpha wrote:
> * Szabolcs Nagy:
> 
> >> I think we should keep the test loop, but create a new scratch buffer on
> >> each iteration.
> >
> > given the documentation of scratch_buffer_dupfree
> > i don't see how the test supposed to work with
> > sizes > buf.length or what's the point of this loop.
> 
> Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
> patch, or do you want to do it?

i think my original patch makes sense that at least has
one scratch_buffer_dupfree test.

or do you prefer to remove this bit completely?

Re: [PATCH 16/20] Fix malloc/tst-scratch_buffer OOB access

[Florian Weimer]

* Szabolcs Nagy:

> The 10/28/2022 13:30, Florian Weimer via Libc-alpha wrote:
>> * Szabolcs Nagy:
>> 
>> >> I think we should keep the test loop, but create a new scratch buffer on
>> >> each iteration.
>> >
>> > given the documentation of scratch_buffer_dupfree
>> > i don't see how the test supposed to work with
>> > sizes > buf.length or what's the point of this loop.
>> 
>> Hmph.  Let's just remove it.  It's unused anyway.  Should I send a
>> patch, or do you want to do it?
>
> i think my original patch makes sense that at least has
> one scratch_buffer_dupfree test.
>
> or do you prefer to remove this bit completely?

Sorry I meant we should remove scratch_buffer_dupfree along with its
test because it's unused after commit
ef0700004bf0dccf493a5e8e21f71d9e7972ea9f ("stdlib: Sync canonicalize
with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]").

Thanks,
Florian

Re: [PATCH 11/20] elf: Fix alloca size in _dl_debug_vdprintf

[Adhemerval Zanella Netto]

On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> The alloca size did not consider the optional width parameter for
> padding which could cause buffer underflow. The width is currently used
> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> can be larger than the alloca buffer size on targets where
> sizeof(void *) >= 2 * sizeof(unsigned long).
> 
> Even if large width is not used on existing targets it is better to fix
> the formatting code to avoid surprises.
> ---
>  elf/dl-printf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> index 429d2e80c2..00c114002c 100644
> --- a/elf/dl-printf.c
> +++ b/elf/dl-printf.c
> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>  		/* We use alloca() to allocate the buffer with the most
>  		   pessimistic guess for the size.  Using alloca() allows
>  		   having more than one integer formatting in a call.  */
> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> +		int size = 1 + 3 * sizeof (unsigned long int);
> +		if (width + 1 > size)
> +		  size = width + 1;
> +		char *buf = (char *) alloca (size);
> +		char *endp = &buf[size];
>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>  
>  		/* Pad to the width the user specified.  */


Would be better to just limit a maximum width and use a fixed-size buffer instead
(and assert if size is larger)? 

Re: [PATCH 04/20] aarch64: Fix the extension header write in getcontext and swapcontext

[Adhemerval Zanella Netto]

On 27/10/22 12:32, Szabolcs Nagy via Libc-alpha wrote:
> The extension header is two 32bit words and in the last header both
> should be 0. There is plenty space in the __reserved area, but it's
> better not to write more than we mean to.

LGTM, thanks.

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

> ---
>  sysdeps/unix/sysv/linux/aarch64/getcontext.S  | 4 ++--
>  sysdeps/unix/sysv/linux/aarch64/swapcontext.S | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/getcontext.S b/sysdeps/unix/sysv/linux/aarch64/getcontext.S
> index 15fbd2d1a0..3109cbd99a 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/getcontext.S
> +++ b/sysdeps/unix/sysv/linux/aarch64/getcontext.S
> @@ -86,8 +86,8 @@ ENTRY(__getcontext)
>  	/* Write the termination context extension header.  */
>  	add	x2, x2, #FPSIMD_CONTEXT_SIZE
>  
> -	str	xzr, [x2, #oHEAD + oMAGIC]
> -	str	xzr, [x2, #oHEAD + oSIZE]
> +	str	wzr, [x2, #oHEAD + oMAGIC]
> +	str	wzr, [x2, #oHEAD + oSIZE]
>  
>  	/* Grab the signal mask */
>  	/* rt_sigprocmask (SIG_BLOCK, NULL, &ucp->uc_sigmask, _NSIG8) */
> diff --git a/sysdeps/unix/sysv/linux/aarch64/swapcontext.S b/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
> index 1ee2e40272..e3ec9da35d 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
> +++ b/sysdeps/unix/sysv/linux/aarch64/swapcontext.S
> @@ -75,8 +75,8 @@ ENTRY(__swapcontext)
>  	/* Write the termination context extension header.  */
>  	add	x2, x2, #FPSIMD_CONTEXT_SIZE
>  
> -	str	xzr, [x2, #oHEAD + oMAGIC]
> -	str	xzr, [x2, #oHEAD + oSIZE]
> +	str	wzr, [x2, #oHEAD + oMAGIC]
> +	str	wzr, [x2, #oHEAD + oSIZE]
>  
>  	/* Preserve ucp.  */
>  	mov	x21, x1

Re: [PATCH 13/20] Use uintptr_t in string/tester for pointer alignment

[Adhemerval Zanella Netto]

On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> The code assumed unsigned long can represent pointers.

LGTM, thanks.

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

> ---
>  string/tester.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/string/tester.c b/string/tester.c
> index eed76239f5..ba948c5723 100644
> --- a/string/tester.c
> +++ b/string/tester.c
> @@ -27,6 +27,7 @@
>  #endif
>  
>  #include <errno.h>
> +#include <stdint.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -529,7 +530,7 @@ test_strlen (void)
>      char *p;
>      for (i=0; i < 0x100; i++)
>        {
> -	p = (char *) ((unsigned long int)(buf + 0xff) & ~0xff) + i;
> +	p = (char *) ((uintptr_t)(buf + 0xff) & ~0xff) + i;
>  	strcpy (p, "OK");
>  	strcpy (p+3, "BAD/WRONG");
>  	check (strlen (p) == 2, 4+i);
> @@ -554,7 +555,7 @@ test_strnlen (void)
>    char buf[4096];
>    for (int i = 0; i < 0x100; ++i)
>      {
> -      char *p = (char *) ((unsigned long int)(buf + 0xff) & ~0xff) + i;
> +      char *p = (char *) ((uintptr_t)(buf + 0xff) & ~0xff) + i;
>        strcpy (p, "OK");
>        strcpy (p + 3, "BAD/WRONG");
>        check (strnlen (p, 100) == 2, 10 + i);
> @@ -582,7 +583,7 @@ test_strchr (void)
>      char *p;
>      for (i=0; i < 0x100; i++)
>        {
> -	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
> +	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
>  	strcpy (p, "OK");
>  	strcpy (p+3, "BAD/WRONG");
>  	check (strchr (p, '/') == NULL, 9+i);
> @@ -614,7 +615,7 @@ test_strchrnul (void)
>      char *p;
>      for (i=0; i < 0x100; i++)
>        {
> -	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
> +	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
>  	strcpy (p, "OK");
>  	strcpy (p+3, "BAD/WRONG");
>  	cp = strchrnul (p, '/');
> @@ -643,7 +644,7 @@ test_rawmemchr (void)
>      char *p;
>      for (i=0; i < 0x100; i++)
>        {
> -	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
> +	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
>  	strcpy (p, "OK");
>  	strcpy (p+3, "BAD/WRONG");
>  	check (rawmemchr (p, 'R') == p+8, 6+i);
> @@ -689,7 +690,7 @@ test_strrchr (void)
>      char *p;
>      for (i=0; i < 0x100; i++)
>        {
> -	p = (char *) ((unsigned long int) (buf + 0xff) & ~0xff) + i;
> +	p = (char *) ((uintptr_t) (buf + 0xff) & ~0xff) + i;
>  	strcpy (p, "OK");
>  	strcpy (p+3, "BAD/WRONG");
>  	check (strrchr (p, '/') == NULL, 9+i);

Re: [PATCH 11/20] elf: Fix alloca size in _dl_debug_vdprintf

[Szabolcs Nagy]

The 10/28/2022 10:56, Adhemerval Zanella Netto wrote:
> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> > The alloca size did not consider the optional width parameter for
> > padding which could cause buffer underflow. The width is currently used
> > e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
> > can be larger than the alloca buffer size on targets where
> > sizeof(void *) >= 2 * sizeof(unsigned long).
> > 
> > Even if large width is not used on existing targets it is better to fix
> > the formatting code to avoid surprises.
> > ---
> >  elf/dl-printf.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/elf/dl-printf.c b/elf/dl-printf.c
> > index 429d2e80c2..00c114002c 100644
> > --- a/elf/dl-printf.c
> > +++ b/elf/dl-printf.c
> > @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
> >  		/* We use alloca() to allocate the buffer with the most
> >  		   pessimistic guess for the size.  Using alloca() allows
> >  		   having more than one integer formatting in a call.  */
> > -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
> > -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
> > +		int size = 1 + 3 * sizeof (unsigned long int);
> > +		if (width + 1 > size)
> > +		  size = width + 1;
> > +		char *buf = (char *) alloca (size);
> > +		char *endp = &buf[size];
> >  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
> >  
> >  		/* Pad to the width the user specified.  */
> 
> 
> Would be better to just limit a maximum width and use a fixed-size buffer instead
> (and assert if size is larger)? 

i already committed this. i think it's safe:
it's internal api and using huge paddings is unlikely.

Re: [PATCH 11/20] elf: Fix alloca size in _dl_debug_vdprintf

[Adhemerval Zanella Netto]

On 28/10/22 11:43, Szabolcs Nagy wrote:
> The 10/28/2022 10:56, Adhemerval Zanella Netto wrote:
>> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
>>> The alloca size did not consider the optional width parameter for
>>> padding which could cause buffer underflow. The width is currently used
>>> e.g. by _dl_map_object_from_fd which passes 2 * sizeof(void *) which
>>> can be larger than the alloca buffer size on targets where
>>> sizeof(void *) >= 2 * sizeof(unsigned long).
>>>
>>> Even if large width is not used on existing targets it is better to fix
>>> the formatting code to avoid surprises.
>>> ---
>>>  elf/dl-printf.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/elf/dl-printf.c b/elf/dl-printf.c
>>> index 429d2e80c2..00c114002c 100644
>>> --- a/elf/dl-printf.c
>>> +++ b/elf/dl-printf.c
>>> @@ -163,8 +163,11 @@ _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
>>>  		/* We use alloca() to allocate the buffer with the most
>>>  		   pessimistic guess for the size.  Using alloca() allows
>>>  		   having more than one integer formatting in a call.  */
>>> -		char *buf = (char *) alloca (1 + 3 * sizeof (unsigned long int));
>>> -		char *endp = &buf[1 + 3 * sizeof (unsigned long int)];
>>> +		int size = 1 + 3 * sizeof (unsigned long int);
>>> +		if (width + 1 > size)
>>> +		  size = width + 1;
>>> +		char *buf = (char *) alloca (size);
>>> +		char *endp = &buf[size];
>>>  		char *cp = _itoa (num, endp, *fmt == 'x' ? 16 : 10, 0);
>>>  
>>>  		/* Pad to the width the user specified.  */
>>
>>
>> Would be better to just limit a maximum width and use a fixed-size buffer instead
>> (and assert if size is larger)? 
> 
> i already committed this. i think it's safe:
> it's internal api and using huge paddings is unlikely.

The idea is to remove all internal alloca usage where possible,
and I think this is a good spot to continue the work.

Re: [PATCH 07/20] Use uintptr_t in fts for pointer alignment

[Adhemerval Zanella Netto]

On 27/10/22 12:32, Szabolcs Nagy via Libc-alpha wrote:
> The code assumed unsigned long can represent pointers.

LGTM, thanks.

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

> ---
>  io/fts.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/io/fts.c b/io/fts.c
> index 283cf1791a..5a0a47a360 100644
> --- a/io/fts.c
> +++ b/io/fts.c
> @@ -55,6 +55,7 @@ static char sccsid[] = "@(#)fts.c	8.6 (Berkeley) 8/14/94";
>  #include <dirent.h>
>  #include <errno.h>
>  #include <fts.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> @@ -67,7 +68,7 @@ static char sccsid[] = "@(#)fts.c	8.6 (Berkeley) 8/14/94";
>  #endif
>  /* Align P to that size.  */
>  #ifndef ALIGN
> -#define	ALIGN(p)	(((unsigned long int) (p) + ALIGNBYTES) & ~ALIGNBYTES)
> +#define	ALIGN(p)	(((uintptr_t) (p) + ALIGNBYTES) & ~ALIGNBYTES)
>  #endif
>  
>  

Re: [PATCH 08/20] malloc: Use uintptr_t for pointer alignment

[Adhemerval Zanella Netto]

On 27/10/22 12:32, Szabolcs Nagy via Libc-alpha wrote:
> From: Carlos Eduardo Seo <carlos.seo@arm.com>
> 
> Avoid integer casts that assume unsigned long can represent pointers.

LGTM, thanks.

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


> ---
>  malloc/arena.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 074ecbc09f..f381f18371 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -439,7 +439,7 @@ dump_heap (heap_info *heap)
>    fprintf (stderr, "Heap %p, size %10lx:\n", heap, (long) heap->size);
>    ptr = (heap->ar_ptr != (mstate) (heap + 1)) ?
>          (char *) (heap + 1) : (char *) (heap + 1) + sizeof (struct malloc_state);
> -  p = (mchunkptr) (((unsigned long) ptr + MALLOC_ALIGN_MASK) &
> +  p = (mchunkptr) (((uintptr_t) ptr + MALLOC_ALIGN_MASK) &
>                     ~MALLOC_ALIGN_MASK);
>    for (;; )
>      {
> @@ -513,7 +513,7 @@ alloc_new_heap  (size_t size, size_t top_pad, size_t pagesize,
>        p1 = (char *) MMAP (0, max_size << 1, PROT_NONE, mmap_flags);
>        if (p1 != MAP_FAILED)
>          {
> -          p2 = (char *) (((unsigned long) p1 + (max_size - 1))
> +          p2 = (char *) (((uintptr_t) p1 + (max_size - 1))
>                           & ~(max_size - 1));
>            ul = p2 - p1;
>            if (ul)
> @@ -752,7 +752,7 @@ _int_new_arena (size_t size)
>  
>    /* Set up the top chunk, with proper alignment. */
>    ptr = (char *) (a + 1);
> -  misalign = (unsigned long) chunk2mem (ptr) & MALLOC_ALIGN_MASK;
> +  misalign = (uintptr_t) chunk2mem (ptr) & MALLOC_ALIGN_MASK;
>    if (misalign > 0)
>      ptr += MALLOC_ALIGNMENT - misalign;
>    top (a) = (mchunkptr) ptr;

Re: [PATCH 10/20] malloc: Fix alignment logic in obstack

[Adhemerval Zanella Netto]

On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> If sizeof(ptrdiff_t) < sizeof(void*) the alignment logic was wrong:
> incorrectly assumed that base was already sufficiently aligned.
> 
> Use more robust alignment logic: this one should work on any target.
> Note: this is an installed header so it must be namespace clean and
> portable hence it uses unsigned long for the alignment offset.
> ---
>  malloc/obstack.h | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/malloc/obstack.h b/malloc/obstack.h
> index 4b01cdfe4d..1cf18e5464 100644
> --- a/malloc/obstack.h
> +++ b/malloc/obstack.h
> @@ -116,22 +116,9 @@
>  # define PTR_INT_TYPE ptrdiff_t
>  #endif
>  
> -/* If B is the base of an object addressed by P, return the result of
> -   aligning P to the next multiple of A + 1.  B and P must be of type
> -   char *.  A + 1 must be a power of 2.  */
> -
> -#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
> -
> -/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
> -   where pointers can be converted to integers, aligned as integers,
> -   and converted back again.  If PTR_INT_TYPE is narrower than a
> -   pointer (e.g., the AS/400), play it safe and compute the alignment
> -   relative to B.  Otherwise, use the faster strategy of computing the
> -   alignment relative to 0.  */
> -
> -#define __PTR_ALIGN(B, P, A)						      \
> -  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
> -		P, A)
> +/* Align P to the next multiple of A + 1, where A + 1 is a power of 2,
> +   A fits into unsigned long and P has type char *.  */
> +#define __PTR_ALIGN(B, P, A) ((P) + (-(unsigned long)(P) & (A)))

Shouldn't you use uintptr_t here to be consistent with your other changes
that exactly change using long to cast from pointers?

It would be good to check with gnulib as well, since this header is also
shared with it.

>  
>  #include <string.h>
>  

Re: [PATCH 10/20] malloc: Fix alignment logic in obstack

[Szabolcs Nagy]

The 10/31/2022 13:14, Adhemerval Zanella Netto wrote:
> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
> > If sizeof(ptrdiff_t) < sizeof(void*) the alignment logic was wrong:
> > incorrectly assumed that base was already sufficiently aligned.
> > 
> > Use more robust alignment logic: this one should work on any target.
> > Note: this is an installed header so it must be namespace clean and
> > portable hence it uses unsigned long for the alignment offset.
> > ---
> >  malloc/obstack.h | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> > 
> > diff --git a/malloc/obstack.h b/malloc/obstack.h
> > index 4b01cdfe4d..1cf18e5464 100644
> > --- a/malloc/obstack.h
> > +++ b/malloc/obstack.h
> > @@ -116,22 +116,9 @@
> >  # define PTR_INT_TYPE ptrdiff_t
> >  #endif
> >  
> > -/* If B is the base of an object addressed by P, return the result of
> > -   aligning P to the next multiple of A + 1.  B and P must be of type
> > -   char *.  A + 1 must be a power of 2.  */
> > -
> > -#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
> > -
> > -/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
> > -   where pointers can be converted to integers, aligned as integers,
> > -   and converted back again.  If PTR_INT_TYPE is narrower than a
> > -   pointer (e.g., the AS/400), play it safe and compute the alignment
> > -   relative to B.  Otherwise, use the faster strategy of computing the
> > -   alignment relative to 0.  */
> > -
> > -#define __PTR_ALIGN(B, P, A)						      \
> > -  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
> > -		P, A)
> > +/* Align P to the next multiple of A + 1, where A + 1 is a power of 2,
> > +   A fits into unsigned long and P has type char *.  */
> > +#define __PTR_ALIGN(B, P, A) ((P) + (-(unsigned long)(P) & (A)))
> 
> Shouldn't you use uintptr_t here to be consistent with your other changes
> that exactly change using long to cast from pointers?

here the offset part is unsigned long, but the pointer is kept
char *. in other patches the problem was that the pointer
was turned into long.

here unsigned int would be enough, since obstack->alignment_mask
is int, larger alignments are not supported.

the new formula may not be the fastest to compute, but if the
goal is portability then i think it's better than the current
code.

> 
> It would be good to check with gnulib as well, since this header is also
> shared with it.

i see. i haven't looked at gnulib.

> 
> >  
> >  #include <string.h>
> >  

Re: [PATCH 10/20] malloc: Fix alignment logic in obstack

[Adhemerval Zanella Netto]

On 01/11/22 06:43, Szabolcs Nagy wrote:
> The 10/31/2022 13:14, Adhemerval Zanella Netto wrote:
>> On 27/10/22 12:33, Szabolcs Nagy via Libc-alpha wrote:
>>> If sizeof(ptrdiff_t) < sizeof(void*) the alignment logic was wrong:
>>> incorrectly assumed that base was already sufficiently aligned.
>>>
>>> Use more robust alignment logic: this one should work on any target.
>>> Note: this is an installed header so it must be namespace clean and
>>> portable hence it uses unsigned long for the alignment offset.
>>> ---
>>>  malloc/obstack.h | 19 +++----------------
>>>  1 file changed, 3 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/malloc/obstack.h b/malloc/obstack.h
>>> index 4b01cdfe4d..1cf18e5464 100644
>>> --- a/malloc/obstack.h
>>> +++ b/malloc/obstack.h
>>> @@ -116,22 +116,9 @@
>>>  # define PTR_INT_TYPE ptrdiff_t
>>>  #endif
>>>  
>>> -/* If B is the base of an object addressed by P, return the result of
>>> -   aligning P to the next multiple of A + 1.  B and P must be of type
>>> -   char *.  A + 1 must be a power of 2.  */
>>> -
>>> -#define __BPTR_ALIGN(B, P, A) ((B) + (((P) - (B) + (A)) & ~(A)))
>>> -
>>> -/* Similar to _BPTR_ALIGN (B, P, A), except optimize the common case
>>> -   where pointers can be converted to integers, aligned as integers,
>>> -   and converted back again.  If PTR_INT_TYPE is narrower than a
>>> -   pointer (e.g., the AS/400), play it safe and compute the alignment
>>> -   relative to B.  Otherwise, use the faster strategy of computing the
>>> -   alignment relative to 0.  */
>>> -
>>> -#define __PTR_ALIGN(B, P, A)						      \
>>> -  __BPTR_ALIGN (sizeof (PTR_INT_TYPE) < sizeof (void *) ? (B) : (char *) 0, \
>>> -		P, A)
>>> +/* Align P to the next multiple of A + 1, where A + 1 is a power of 2,
>>> +   A fits into unsigned long and P has type char *.  */
>>> +#define __PTR_ALIGN(B, P, A) ((P) + (-(unsigned long)(P) & (A)))
>>
>> Shouldn't you use uintptr_t here to be consistent with your other changes
>> that exactly change using long to cast from pointers?
> 
> here the offset part is unsigned long, but the pointer is kept
> char *. in other patches the problem was that the pointer
> was turned into long.
> 
> here unsigned int would be enough, since obstack->alignment_mask
> is int, larger alignments are not supported.
> 
> the new formula may not be the fastest to compute, but if the
> goal is portability then i think it's better than the current
> code.

Alright, although I still why not use uintptr_t here for consistency (as we
do for all other pointer conversions).  And the code already include stddef.h.

> 
>>
>> It would be good to check with gnulib as well, since this header is also
>> shared with it.
> 
> i see. i haven't looked at gnulib