[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aeAJdgZzaP92bJkC@mail.gmail.com>
Date: Wed, 15 Apr 2026 23:56:06 +0200
From: Luca Kellermann <mailto.luca.kellermann@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] scandir: fix qsort usage
On Sun, Apr 12, 2026 at 11:24:45AM -0400, Rich Felker wrote:
> On Sun, Apr 12, 2026 at 11:17:22AM -0400, Rich Felker wrote:
> > On Sun, Apr 12, 2026 at 01:43:08PM +0200, Luca Kellermann wrote:
> > > On Fri, Apr 10, 2026 at 01:30:27PM -0400, Rich Felker wrote:
> > > > On Fri, Feb 27, 2026 at 09:17:15PM +0100, Luca Kellermann wrote:
> > > > > calling qsort() with a pointer to a function whose type is not
> > > > > compatible with int(const void *, const void *) results in UB because
> > > > > qsort() would call this function with an incompatible type.
> > > > >
> > > > > avoid this by using qsort_r(). this is similar to how qsort() is
> > > > > implemented on top of qsort_r().
> > > > >
> > > > > the casts to void * in wrapper_cmp() are required because scandir()
> > > > > takes int (*)(const struct dirent **, const struct dirent **) and not
> > > > > int (*)(const struct dirent *const *, const struct dirent *const *).
> > > > > ---
> > > > > Since you haven't posted a patch yet, I went ahead and fixed this. The
> > > > > patch is based on the scandir() patch series I submitted in July.
> > > > >
> > > > > src/dirent/scandir.c | 12 +++++++++---
> > > > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
> > > > > index 9e4e623f..993e0c8b 100644
> > > > > --- a/src/dirent/scandir.c
> > > > > +++ b/src/dirent/scandir.c
> > > > > @@ -7,9 +7,15 @@
> > > > > #include <errno.h>
> > > > > #include <stddef.h>
> > > > >
> > > > > +typedef int (*cmpfun)(const struct dirent **, const struct dirent **);
> > > > > +
> > > > > +static int wrapper_cmp(const void *de1, const void *de2, void *cmp)
> > > > > +{
> > > > > + return ((cmpfun)cmp)((void *)de1, (void *)de2);
> > > > > +}
> > > > > +
> > > >
> > > > I was going to ask you what the (void *) casts are for, but now I see
> > > > it. The scandir comparison function was specified wrong and takes as
> > > > const T ** rather than a T *const * like it should.
> > >
> > > I anticipated that this might raise some questions. That's why I tried
> > > to explain it in the commit message :)
> > >
> > > > Unfortunately this also reveals an aliasing violation elsewhere. The
> > > > lvalue type the caller's comparison function will be using to access
> > > > elements of the names[] array are of type const struct dirent *, but
> > > > the actual elements of the names[] array have type struct dirent *. I
> > > > can prepare a separate patch to address this.
> > >
> > > I'm curious what fix you have in mind for this. The only thing I can
> > > think of is to change the type of the names array to const struct
> > > dirent ** and after sorting copy it back to a new array of type struct
> > > dirent ** that can then be stored in *res.
> >
> > Uhg, I missed that the array returned in *res mismatches the type of
> > the array to be sorted; I was thinking we could just adjust it to
> > const. Indeed copying would be gratuitously awful. Maybe this is
> > fixable just with some union punning. Or.. wrapper_cmp could just copy
> > the pointer values into local vars and pass the addresses of those
> > local vars to the cmp function. This is probably the most correct.
>
> As in (untested but compiles without warnings):
>
> static int wrapper_cmp(const void *de1, const void *de2, void *cmp)
> {
> const struct dirent *cd1 = *(struct dirent *const *)de1;
> const struct dirent *cd2 = *(struct dirent *const *)de2;
> return ((int (*)(const struct dirent **, const struct dirent **))cmp)(&cd1, &cd2);
> }
>
> This is fully const-correct and lacks aliasing violations.
That's a nice workaround. I've adopted it but used compound literals
instead of casts to highlight that no const is being cast away.
v3 attached with some code reorderings that reduce the number of
modified lines in the rest of the scandir() patch series and an
additional include cleanup.
Luca
View attachment "v3-0001-scandir-hide-that-errno-is-set-to-0.patch" of type "text/x-diff" (2226 bytes)
View attachment "v3-0002-scandir-don-t-examine-errno-after-closedir.patch" of type "text/x-diff" (2424 bytes)
View attachment "v3-0003-scandir-disable-cancellation-around-cancellation-.patch" of type "text/x-diff" (4658 bytes)
View attachment "v3-0004-scandir-report-ENOMEM-and-EOVERFLOW.patch" of type "text/x-diff" (1645 bytes)
View attachment "v3-0005-scandir-fix-qsort-usage.patch" of type "text/x-diff" (2205 bytes)
View attachment "v3-0006-scandir-remove-unused-header.patch" of type "text/x-diff" (764 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
