[<prev] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260410173027.GT1827@brightrain.aerifal.cx>
Date: Fri, 10 Apr 2026 13:30:27 -0400
From: Rich Felker <dalias@...c.org>
To: Luca Kellermann <mailto.luca.kellermann@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] scandir: fix qsort usage
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.
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.
> int scandir(const char *path, struct dirent ***res,
> - int (*sel)(const struct dirent *),
> - int (*cmp)(const struct dirent **, const struct dirent **))
> + int (*sel)(const struct dirent *), cmpfun cmp)
> {
I would avoid changing the public signature to be defined in terms of
an implementation-internal typedef. It just makes it less obvious that
it's correct.
My preference would be to avoid making the typedef entirely, but I'm
fairly indifferent if you like it in wrapper_cmp.
> DIR *d;
> struct dirent *de, **names=0, **tmp;
> @@ -70,7 +76,7 @@ int scandir(const char *path, struct dirent ***res,
> /* cmp() and caller must not observe that errno was set to 0. */
> errno = old_errno;
>
> - if (cmp) qsort(names, cnt, sizeof *names, (int (*)(const void *, const void *))cmp);
> + if (cmp) __qsort_r(names, cnt, sizeof *names, wrapper_cmp, (void *)cmp);
> *res = names;
> return cnt;
> }
Use of the __ version probably isn't strictly needed here, but better
anyway.
Rich
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.
