[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <adwM9hI8mkgtMNVw@mail.gmail.com>
Date: Sun, 12 Apr 2026 23:21:58 +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 12:17:43PM -0400, Rich Felker wrote:
> 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.
>
> OK, digging into this more (thanks Haelwenn on Mastodon for help
> finding this stuff):
>
> Historically, glibc and the BSDs did not have these weird comparson
> and function types with double-pointers. They did the right thing,
> having the comparison function just take a pointer to the dirent,
> which was rightly const-qualified to signify that the comparison
> function is not to modify the data being sorted.
>
> This was changed by glibc in commit
> eee6b1432794967d4272394dfed1e2b5cca4be39 to align with the newly
> redesigned-by-committee POSIX-2008 signature, which has the const
> qualification in the wrong place. My only guess is that they made this
> change in order to have it be "possible to implement with qsort by
> casting the function pointer", which is gross UB.
At least for glibc, I think this is not entirely true. Before the
mentioned commit, the declaration in the public header was equivalent
to:
int scandir(const char *restrict, struct dirent ***restrict,
int (*)(const struct dirent *),
int (*)(const void *, const void *));
(note that POSIX never specified those restrict qualifiers but glibc
and the Linux man page still have them today)
The comparison function was directly passed to qsort() to sort an
array of type struct dirent **. Because the comparison function for
scandir() had the same type as qsort(), this was totally fine and free
of UB. The comparison function still had to deal with double-pointers
though. It also had to take care of the type conversion itself (which
glibc's alphasort() got wrong).
> Everything we're doing to fix this now is working around gratuitous
> breakage the committee introduced to avoid just standardizing the
> historical practice that was right but that required a qsort_r or
> similar to implement.
Assuming they wanted to standardize the actual existing practice I
described above, they only got the placement of const wrong.
Luca
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.
