[v2] qsort: Fix a typo causing unnecessary malloc/free (BZ 31276)
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
In qsort_r we allocate a buffer sized QSORT_STACK_SIZE (1024) on stack
and we intend to use it if all elements can fit into it. But there is a
typo:
if (total_size < sizeof buf)
buf = tmp;
else
/* allocate a buffer on heap and use it ... */
Here "buf" is a pointer, thus sizeof buf is just 4 or 8, instead of
1024. There is also a minor issue that we should use "<=" instead of
"<".
This bug is detected debugging some strange heap corruption running the
Ruby-3.3.0 test suite (on an experimental Linux From Scratch build using
Binutils-2.41.90 and Glibc trunk, and also Fedora Rawhide [1]). It
seems Ruby is doing some wild "optimization" by jumping into somewhere
in qsort_r instead of calling it normally, resulting in a double free of
buf if we allocate it on heap. The issue can be reproduced
deterministically with:
LD_PRELOAD=/usr/lib/libc_malloc_debug.so MALLOC_CHECK_=3 \
LD_LIBRARY_PATH=. ./ruby test/runner.rb test/ruby/test_enum.rb
in Ruby-3.3.0 tree after building it. This change would hide the issue
for Ruby, but Ruby is likely still buggy (if using this "optimization"
sorting larger arrays).
[1]:https://kojipkgs.fedoraproject.org/work/tasks/9729/111889729/build.log
Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
v1 -> v2:
- Use `<=` instead of `<`.
- Add BZ number.
stdlib/qsort.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 22/01/24 17:29, Xi Ruoyao wrote:
> In qsort_r we allocate a buffer sized QSORT_STACK_SIZE (1024) on stack
> and we intend to use it if all elements can fit into it. But there is a
> typo:
>
> if (total_size < sizeof buf)
> buf = tmp;
> else
> /* allocate a buffer on heap and use it ... */
>
> Here "buf" is a pointer, thus sizeof buf is just 4 or 8, instead of
> 1024. There is also a minor issue that we should use "<=" instead of
> "<".
>
> This bug is detected debugging some strange heap corruption running the
> Ruby-3.3.0 test suite (on an experimental Linux From Scratch build using
> Binutils-2.41.90 and Glibc trunk, and also Fedora Rawhide [1]). It
> seems Ruby is doing some wild "optimization" by jumping into somewhere
> in qsort_r instead of calling it normally, resulting in a double free of
> buf if we allocate it on heap. The issue can be reproduced
> deterministically with:
>
> LD_PRELOAD=/usr/lib/libc_malloc_debug.so MALLOC_CHECK_=3 \
> LD_LIBRARY_PATH=. ./ruby test/runner.rb test/ruby/test_enum.rb
>
> in Ruby-3.3.0 tree after building it. This change would hide the issue
> for Ruby, but Ruby is likely still buggy (if using this "optimization"
> sorting larger arrays).
>
> [1]:https://kojipkgs.fedoraproject.org/work/tasks/9729/111889729/build.log
>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
LGTM, thanks for catching it.
> ---
>
> v1 -> v2:
> - Use `<=` instead of `<`.
> - Add BZ number.
>
> stdlib/qsort.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/stdlib/qsort.c b/stdlib/qsort.c
> index 7f5a00fb33..be47aebbe0 100644
> --- a/stdlib/qsort.c
> +++ b/stdlib/qsort.c
> @@ -353,7 +353,7 @@ __qsort_r (void *const pbase, size_t total_elems, size_t size,
> if (size > INDIRECT_SORT_SIZE_THRES)
> total_size = 2 * total_elems * sizeof (void *) + size;
>
> - if (total_size < sizeof buf)
> + if (total_size <= sizeof tmp)
> buf = tmp;
> else
> {
Am Montag, 22. Januar 2024, 21:31:18 CET schrieb Adhemerval Zanella Netto:
>
> On 22/01/24 17:29, Xi Ruoyao wrote:
> > In qsort_r we allocate a buffer sized QSORT_STACK_SIZE (1024) on stack
> > and we intend to use it if all elements can fit into it. But there is a
> > typo:
> >
> > if (total_size < sizeof buf)
> > buf = tmp;
> > else
> > /* allocate a buffer on heap and use it ... */
> >
> > Here "buf" is a pointer, thus sizeof buf is just 4 or 8, instead of
> > 1024. There is also a minor issue that we should use "<=" instead of
> > "<".
> >
> > This bug is detected debugging some strange heap corruption running the
> > Ruby-3.3.0 test suite (on an experimental Linux From Scratch build using
> > Binutils-2.41.90 and Glibc trunk, and also Fedora Rawhide [1]). It
> > seems Ruby is doing some wild "optimization" by jumping into somewhere
> > in qsort_r instead of calling it normally, resulting in a double free of
> > buf if we allocate it on heap. The issue can be reproduced
> > deterministically with:
> >
> > LD_PRELOAD=/usr/lib/libc_malloc_debug.so MALLOC_CHECK_=3 \
> > LD_LIBRARY_PATH=. ./ruby test/runner.rb test/ruby/test_enum.rb
> >
> > in Ruby-3.3.0 tree after building it. This change would hide the issue
> > for Ruby, but Ruby is likely still buggy (if using this "optimization"
> > sorting larger arrays).
> >
> > [1]:https://kojipkgs.fedoraproject.org/work/tasks/9729/111889729/build.log
> >
> > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
>
> LGTM, thanks for catching it.
>
Great, let's push it.
On Mon, 2024-01-22 at 21:50 +0100, Andreas K. Huettel wrote:
> Am Montag, 22. Januar 2024, 21:31:18 CET schrieb Adhemerval Zanella Netto:
> >
> > On 22/01/24 17:29, Xi Ruoyao wrote:
> > > In qsort_r we allocate a buffer sized QSORT_STACK_SIZE (1024) on stack
> > > and we intend to use it if all elements can fit into it. But there is a
> > > typo:
> > >
> > > if (total_size < sizeof buf)
> > > buf = tmp;
> > > else
> > > /* allocate a buffer on heap and use it ... */
> > >
> > > Here "buf" is a pointer, thus sizeof buf is just 4 or 8, instead of
> > > 1024. There is also a minor issue that we should use "<=" instead of
> > > "<".
> > >
> > > This bug is detected debugging some strange heap corruption running the
> > > Ruby-3.3.0 test suite (on an experimental Linux From Scratch build using
> > > Binutils-2.41.90 and Glibc trunk, and also Fedora Rawhide [1]). It
> > > seems Ruby is doing some wild "optimization" by jumping into somewhere
> > > in qsort_r instead of calling it normally, resulting in a double free of
> > > buf if we allocate it on heap. The issue can be reproduced
> > > deterministically with:
> > >
> > > LD_PRELOAD=/usr/lib/libc_malloc_debug.so MALLOC_CHECK_=3 \
> > > LD_LIBRARY_PATH=. ./ruby test/runner.rb test/ruby/test_enum.rb
> > >
> > > in Ruby-3.3.0 tree after building it. This change would hide the issue
> > > for Ruby, but Ruby is likely still buggy (if using this "optimization"
> > > sorting larger arrays).
> > >
> > > [1]:https://kojipkgs.fedoraproject.org/work/tasks/9729/111889729/build.log
> > >
> > > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> >
> > LGTM, thanks for catching it.
> >
> Great, let's push it.
Note that I don't have a write access, please push for master and 2.39.
On Tue, Jan 23, 2024 at 12:20 AM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Mon, 2024-01-22 at 21:50 +0100, Andreas K. Huettel wrote:
> > Am Montag, 22. Januar 2024, 21:31:18 CET schrieb Adhemerval Zanella Netto:
> > >
> > > On 22/01/24 17:29, Xi Ruoyao wrote:
> > > > In qsort_r we allocate a buffer sized QSORT_STACK_SIZE (1024) on stack
> > > > and we intend to use it if all elements can fit into it. But there is a
> > > > typo:
> > > >
> > > > if (total_size < sizeof buf)
> > > > buf = tmp;
> > > > else
> > > > /* allocate a buffer on heap and use it ... */
> > > >
> > > > Here "buf" is a pointer, thus sizeof buf is just 4 or 8, instead of
> > > > 1024. There is also a minor issue that we should use "<=" instead of
> > > > "<".
> > > >
> > > > This bug is detected debugging some strange heap corruption running the
> > > > Ruby-3.3.0 test suite (on an experimental Linux From Scratch build using
> > > > Binutils-2.41.90 and Glibc trunk, and also Fedora Rawhide [1]). It
> > > > seems Ruby is doing some wild "optimization" by jumping into somewhere
> > > > in qsort_r instead of calling it normally, resulting in a double free of
> > > > buf if we allocate it on heap. The issue can be reproduced
> > > > deterministically with:
> > > >
> > > > LD_PRELOAD=/usr/lib/libc_malloc_debug.so MALLOC_CHECK_=3 \
> > > > LD_LIBRARY_PATH=. ./ruby test/runner.rb test/ruby/test_enum.rb
> > > >
> > > > in Ruby-3.3.0 tree after building it. This change would hide the issue
> > > > for Ruby, but Ruby is likely still buggy (if using this "optimization"
> > > > sorting larger arrays).
> > > >
> > > > [1]:https://kojipkgs.fedoraproject.org/work/tasks/9729/111889729/build.log
> > > >
> > > > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > >
> > > LGTM, thanks for catching it.
> > >
> > Great, let's push it.
>
> Note that I don't have a write access, please push for master and 2.39.
>
Pushed onto master. 2.39 hasn't been branched yet.
Thanks.
@@ -353,7 +353,7 @@ __qsort_r (void *const pbase, size_t total_elems, size_t size,
if (size > INDIRECT_SORT_SIZE_THRES)
total_size = 2 * total_elems * sizeof (void *) + size;
- if (total_size < sizeof buf)
+ if (total_size <= sizeof tmp)
buf = tmp;
else
{