* [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727]
2022-10-28 16:39 [PATCH v2 0/4] patches from the morello port Szabolcs Nagy
@ 2022-10-28 16:39 ` Szabolcs Nagy
2022-11-02 15:20 ` Andreas Schwab
2022-10-28 16:40 ` [PATCH v2 2/4] Fix off-by-one OOB write in iconv/tst-iconv-mt Szabolcs Nagy
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Szabolcs Nagy @ 2022-10-28 16:39 UTC (permalink / raw)
To: libc-alpha
__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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727]
2022-10-28 16:39 ` [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727] Szabolcs Nagy
@ 2022-11-02 15:20 ` Andreas Schwab
0 siblings, 0 replies; 8+ messages in thread
From: Andreas Schwab @ 2022-11-02 15:20 UTC (permalink / raw)
To: Szabolcs Nagy via Libc-alpha; +Cc: Szabolcs Nagy
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."
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] Fix off-by-one OOB write in iconv/tst-iconv-mt
2022-10-28 16:39 [PATCH v2 0/4] patches from the morello port Szabolcs Nagy
2022-10-28 16:39 ` [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727] Szabolcs Nagy
@ 2022-10-28 16:40 ` Szabolcs Nagy
2022-10-28 16:40 ` [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS Szabolcs Nagy
2022-10-28 16:40 ` [PATCH v2 4/4] Remove unused scratch_buffer_dupfree Szabolcs Nagy
3 siblings, 0 replies; 8+ messages in thread
From: Szabolcs Nagy @ 2022-10-28 16:40 UTC (permalink / raw)
To: libc-alpha
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS
2022-10-28 16:39 [PATCH v2 0/4] patches from the morello port Szabolcs Nagy
2022-10-28 16:39 ` [PATCH v2 1/4] Fix OOB read in stdlib thousand grouping parsing [BZ #29727] Szabolcs Nagy
2022-10-28 16:40 ` [PATCH v2 2/4] Fix off-by-one OOB write in iconv/tst-iconv-mt Szabolcs Nagy
@ 2022-10-28 16:40 ` Szabolcs Nagy
2022-10-28 17:04 ` Florian Weimer
2022-10-28 16:40 ` [PATCH v2 4/4] Remove unused scratch_buffer_dupfree Szabolcs Nagy
3 siblings, 1 reply; 8+ messages in thread
From: Szabolcs Nagy @ 2022-10-28 16:40 UTC (permalink / raw)
To: 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;
}
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS
2022-10-28 16:40 ` [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS Szabolcs Nagy
@ 2022-10-28 17:04 ` Florian Weimer
0 siblings, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2022-10-28 17:04 UTC (permalink / raw)
To: Szabolcs Nagy via Libc-alpha; +Cc: Szabolcs Nagy
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] Remove unused scratch_buffer_dupfree
2022-10-28 16:39 [PATCH v2 0/4] patches from the morello port Szabolcs Nagy
` (2 preceding siblings ...)
2022-10-28 16:40 ` [PATCH v2 3/4] Fix elf/tst-dlmopen-twice not to exhaust static TLS Szabolcs Nagy
@ 2022-10-28 16:40 ` Szabolcs Nagy
2022-10-28 17:06 ` Florian Weimer
3 siblings, 1 reply; 8+ messages in thread
From: Szabolcs Nagy @ 2022-10-28 16:40 UTC (permalink / raw)
To: 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
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] Remove unused scratch_buffer_dupfree
2022-10-28 16:40 ` [PATCH v2 4/4] Remove unused scratch_buffer_dupfree Szabolcs Nagy
@ 2022-10-28 17:06 ` Florian Weimer
0 siblings, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2022-10-28 17:06 UTC (permalink / raw)
To: Szabolcs Nagy via Libc-alpha; +Cc: Szabolcs Nagy
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread