[v2] qsort: Fix a typo causing unnecessary malloc/free (BZ 31276)

  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(-)
  

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.