[PATCH 1/4] scandir: hide that errno is set to 0

[PATCH 1/4] scandir: hide that errno is set to 0

[Luca Kellermann]

POSIX.1-2024 requires that standard functions don't set errno to 0.
commit dae17a1aaf25d8333e729173d86659066607d87d ensured that cmp() and
the caller of scandir() cannot observe that errno is set to 0
internally. however, this was not yet the case for the sel() callback.

the following program demonstrates the issue:

#include <assert.h>
#include <dirent.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
static int sel(const struct dirent *e)
{
	/* XSH errno / XSH 2.3 Error Numbers: "No function in this
	 * volume of POSIX.1-2024 shall set errno to [0 / zero]." */
	assert(errno != 0);
	return 1;
}
int main(void)
{
	/* XSH errno: "The value of errno in the initial thread shall
	 * be zero at program startup [...]" */
	assert(errno == 0);
	errno = 1;
	struct dirent **es = 0;
	int n = scandir(".", &es, sel, alphasort);
	if (n == -1) {
		perror("scandir");
		exit(EXIT_FAILURE);
	}
	while (n > 0) free(es[--n]);
	free(es);
}

$ ./scandir-errno
Assertion failed: errno != 0 (scandir-errno.c: sel: 10)
Aborted (core dumped)
---
 src/dirent/scandir.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index 7456b9b8..eead7e50 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -17,7 +17,11 @@ int scandir(const char *path, struct dirent ***res,
 	if (!d) return -1;
 
 	while ((errno=0), (de = readdir(d))) {
-		if (sel && !sel(de)) continue;
+		if (sel) {
+			/* sel() must not observe that errno was set to 0. */
+			errno = old_errno;
+			if (!sel(de)) continue;
+		}
 		if (cnt >= len) {
 			len = 2*len+1;
 			if (len > SIZE_MAX/sizeof *names) break;
@@ -37,6 +41,7 @@ int scandir(const char *path, struct dirent ***res,
 		free(names);
 		return -1;
 	}
+	/* 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);
-- 
2.43.0

[PATCH 2/4] scandir: don't examine errno after closedir()

[Luca Kellermann]

when closedir() set errno, scandir() misinterpreted this as a failure.
this was wrong for two reasons:

* if closedir() succeeds, errno could still have been set, e.g. by
  __aio_close().

* even if closedir() "fails", it always closes the file descriptor and
  frees memory, so there is no reason to free all directory entries
  and return from scandir() with a failure.

the following program demonstrates the issue with aio:

#include <aio.h>
#include <dirent.h>
#include <errno.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
static void die(const char *msg)
{
	perror(msg);
	exit(EXIT_FAILURE);
}
int main(void)
{
	struct aiocb cb = {
		.aio_fildes = STDIN_FILENO,
		.aio_buf = &(char){0},
		.aio_nbytes = 1,
		.aio_sigevent.sigev_notify = SIGEV_NONE,
	};
	if (aio_read(&cb)) die("aio_read");
	struct dirent **es = 0;
	int e, n = scandir(".", &es, 0, alphasort);
	if (n == -1) die("scandir");
	while (n > 0) free(es[--n]);
	free(es);
	if (aio_suspend(&(const struct aiocb *){&cb}, 1, 0))
		die("aio_suspend");
	if ((e = aio_error(&cb)) == -1) die("aio_error");
	if (aio_return(&cb) == -1) {
		if (!e) die("aio_return");
		errno = e;
		die("read");
	}
}

$ ./scandir-aio
scandir: No such file or directory
---
 src/dirent/scandir.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index eead7e50..cf668535 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -12,7 +12,7 @@ int scandir(const char *path, struct dirent ***res,
 	DIR *d = opendir(path);
 	struct dirent *de, **names=0, **tmp;
 	size_t cnt=0, len=0;
-	int old_errno = errno;
+	int old_errno = errno, err;
 
 	if (!d) return -1;
 
@@ -33,12 +33,18 @@ int scandir(const char *path, struct dirent ***res,
 		if (!names[cnt]) break;
 		memcpy(names[cnt++], de, de->d_reclen);
 	}
+	/* closedir() might set errno via __aio_close(). It might also "fail"
+	 * (return -1 and set errno). But even then, the file descriptor is
+	 * closed and memory is freed, so there is no reason to report the
+	 * "failure" of closedir() as a failure of scandir(). */
+	err = errno;
 
 	closedir(d);
 
-	if (errno) {
+	if (err) {
 		if (names) while (cnt-->0) free(names[cnt]);
 		free(names);
+		errno = err;
 		return -1;
 	}
 	/* cmp() and caller must not observe that errno was set to 0. */
-- 
2.43.0

[PATCH 3/4] scandir: fix leaks caused by cancellation

[Luca Kellermann]

opendir(), sel(), closedir(), or cmp() might act upon a cancellation
request. because scandir() did not install a cancellation cleanup
handler, this could lead to memory and file descriptor leaks.

the following program demonstrates the leaks:

#include <dirent.h>
#include <errno.h>
#include <pthread.h>
#include <semaphore.h>
#include <stdio.h>
#include <stdlib.h>
static void die(const char *msg)
{
	int cs, e = errno;
	if (!pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs)) {
		errno = e;
		perror(msg);
	}
	_Exit(EXIT_FAILURE);
}
static int posted;
static sem_t sem;
static int sel(const struct dirent *e)
{
	if (!posted && sem_post(&sem)) die("sem_post");
	posted = 1;
	return 1;
}
static void *run(void *arg)
{
	posted = 0;
	struct dirent **es = 0;
	int n = scandir(".", &es, sel, alphasort);
	if (n == -1) die("scandir");
	while (n > 0) free(es[--n]);
	free(es);
	return 0;
}
int main(void)
{
	if (sem_init(&sem, 0, 0)) die("sem_init");
	void *res = 0;
	do {
		pthread_t tid;
		if (errno = pthread_create(&tid, 0, run, 0))
			die("pthread_create");
		if (sem_wait(&sem)) die("sem_wait");
		if (errno = pthread_cancel(tid))
			die("pthread_cancel");
		if (errno = pthread_join(tid, &res))
			die("pthread_join");
	} while (res != PTHREAD_CANCELED);
	if (sem_destroy(&sem)) die("sem_destroy");
}

$ valgrind -q --leak-check=full --show-leak-kinds=all ./scandir-leak
==31293== 296 bytes in 9 blocks are indirectly lost in loss record 1 of 3
==31293==    at 0x48AF828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C500: scandir (scandir.c:32)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==
==31293== 416 (120 direct, 296 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 3
==31293==    at 0x48B6B80: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C4EA: scandir (scandir.c:28)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==
==31293== 2,072 bytes in 1 blocks are definitely lost in loss record 3 of 3
==31293==    at 0x48B6953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C2E0: opendir (opendir.c:15)
==31293==    by 0x401C491: scandir (scandir.c:12)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==

$ strace -fe t=open,close ./scandir-leak
strace: Process 27136 attached
[pid 27136] open(".", O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_DIRECTORY) = 3
[pid 27136] --- SIGRT_1 {si_signo=SIGRT_1, si_code=SI_TKILL, si_pid=27135, si_uid=1000} ---
[pid 27136] +++ exited with 0 +++
+++ exited with 0 +++
---
 src/dirent/scandir.c | 75 ++++++++++++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 23 deletions(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index cf668535..8883b9f8 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -1,37 +1,61 @@
 #include <dirent.h>
+#include <pthread.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdint.h>
 #include <errno.h>
 #include <stddef.h>
 
+struct cleanup_params {
+	DIR *d;
+	struct dirent **names;
+	size_t cnt;
+};
+
+static void cleanup(void *p)
+{
+	struct cleanup_params *c = p;
+	/* Cancellation is disabled if c->d is not NULL, so closedir()
+	 * won't act upon a cancellation request here. */
+	if (c->d) closedir(c->d);
+	if (c->names) {
+		size_t cnt = c->cnt;
+		while (cnt-->0) free(c->names[cnt]);
+		free(c->names);
+	}
+}
+
 int scandir(const char *path, struct dirent ***res,
 	int (*sel)(const struct dirent *),
 	int (*cmp)(const struct dirent **, const struct dirent **))
 {
-	DIR *d = opendir(path);
-	struct dirent *de, **names=0, **tmp;
-	size_t cnt=0, len=0;
+	/* opendir() might act upon a cancellation request. */
+	struct cleanup_params c = {.d = opendir(path)};
+	struct dirent *de, **tmp;
+	size_t len=0;
 	int old_errno = errno, err;
 
-	if (!d) return -1;
+	if (!c.d) return -1;
+
+	/* sel(), closedir(), or cmp() might act upon a cancellation request. */
+	pthread_cleanup_push(cleanup, &c);
 
-	while ((errno=0), (de = readdir(d))) {
+	while ((errno=0), (de = readdir(c.d))) {
 		if (sel) {
 			/* sel() must not observe that errno was set to 0. */
 			errno = old_errno;
 			if (!sel(de)) continue;
 		}
-		if (cnt >= len) {
+		if (c.cnt >= len) {
 			len = 2*len+1;
-			if (len > SIZE_MAX/sizeof *names) break;
-			tmp = realloc(names, len * sizeof *names);
+			if (len > SIZE_MAX/sizeof *c.names) break;
+			tmp = realloc(c.names, len * sizeof *c.names);
 			if (!tmp) break;
-			names = tmp;
+			c.names = tmp;
 		}
-		names[cnt] = malloc(de->d_reclen);
-		if (!names[cnt]) break;
-		memcpy(names[cnt++], de, de->d_reclen);
+		c.names[c.cnt] = malloc(de->d_reclen);
+		if (!c.names[c.cnt]) break;
+		memcpy(c.names[c.cnt++], de, de->d_reclen);
 	}
 	/* closedir() might set errno via __aio_close(). It might also "fail"
 	 * (return -1 and set errno). But even then, the file descriptor is
@@ -39,18 +63,23 @@ int scandir(const char *path, struct dirent ***res,
 	 * "failure" of closedir() as a failure of scandir(). */
 	err = errno;
 
-	closedir(d);
+	/* If closedir() acts upon a cancellation request, it is retried by
+	 * cleanup() with cancellation disabled. closedir() won't act upon a
+	 * cancellation request in masked cancellation mode, meaning errno does
+	 * not need to be inspected for ECANCELED. */
+	closedir(c.d);
+	/* cleanup() shouldn't call closedir() from here on, because the
+	 * directory stream is already closed. */
+	c.d = 0;
 
-	if (err) {
-		if (names) while (cnt-->0) free(names[cnt]);
-		free(names);
-		errno = err;
-		return -1;
+	if (!err) {
+		/* cmp() and caller must not observe that errno was set to 0. */
+		errno = old_errno;
+		if (cmp) qsort(c.names, c.cnt, sizeof *c.names, (int (*)(const void *, const void *))cmp);
+		*res = c.names;
 	}
-	/* 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);
-	*res = names;
-	return cnt;
+	pthread_cleanup_pop(err);
+
+	return err ? (errno = err), -1 : c.cnt;
 }
-- 
2.43.0

[PATCH 4/4] scandir: report ENOMEM and EOVERFLOW

[Luca Kellermann]

if the loop is exited because len * sizeof *c.names does not fit into
size_t, errno should be explicitly set to ENOMEM. previously, the
behavior differed depending on which value errno happened to have at
this point.

if c.cnt reached a value > INT_MAX, scandir() returned an incorrect
value. EOVERFLOW should be reported instead.

it's unlikely that these errors can actually occur. it may not even
be possible to have directories with that many entries, and even then
malloc() or realloc() will probably fail long before len or c.cnt
reach those large values.
---
 src/dirent/scandir.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index 8883b9f8..07b54806 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -1,4 +1,5 @@
 #include <dirent.h>
+#include <limits.h>
 #include <pthread.h>
 #include <string.h>
 #include <stdlib.h>
@@ -46,9 +47,16 @@ int scandir(const char *path, struct dirent ***res,
 			errno = old_errno;
 			if (!sel(de)) continue;
 		}
+		if (c.cnt >= INT_MAX) {
+			errno = EOVERFLOW;
+			break;
+		}
 		if (c.cnt >= len) {
 			len = 2*len+1;
-			if (len > SIZE_MAX/sizeof *c.names) break;
+			if (len > SIZE_MAX/sizeof *c.names) {
+				errno = ENOMEM;
+				break;
+			}
 			tmp = realloc(c.names, len * sizeof *c.names);
 			if (!tmp) break;
 			c.names = tmp;
-- 
2.43.0

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Markus Wichmann]

Am Thu, Jul 10, 2025 at 08:51:30PM +0200 schrieb Luca Kellermann:
> opendir(), sel(), closedir(), or cmp() might act upon a cancellation
> request. because scandir() did not install a cancellation cleanup
> handler, this could lead to memory and file descriptor leaks.

scandir() is on the "may be cancel-point" list in POSIX-2024, so it
would also be acceptable to just disable cancellation for the duration
of the function (since it's not on the "shall be cancel-point" list).

Ciao,
Markus

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Rich Felker]

On Thu, Jul 10, 2025 at 09:19:24PM +0200, Markus Wichmann wrote:
> Am Thu, Jul 10, 2025 at 08:51:30PM +0200 schrieb Luca Kellermann:
> > opendir(), sel(), closedir(), or cmp() might act upon a cancellation
> > request. because scandir() did not install a cancellation cleanup
> > handler, this could lead to memory and file descriptor leaks.
> 
> scandir() is on the "may be cancel-point" list in POSIX-2024, so it
> would also be acceptable to just disable cancellation for the duration
> of the function (since it's not on the "shall be cancel-point" list).

Yes, generally we do not support cancellation for the "may be a
cancellation point" functions except in some trivial cases. Here it's
complicated by the fact that we're calling back to application code,
and scandir is underspecified in that it doesn't actually specify what
happens if the sel or compar function does not return in the normative
text. However, the APPLICATION USAGE section mentions this possibility
and advises applications not to do it, but doesn't seem to forbid it.

Rich

Re: [PATCH 1/4] scandir: hide that errno is set to 0

[Thorsten Glaser]

Luca Kellermann dixit:

>+		if (sel) {
>+			/* sel() must not observe that errno was set to 0. */
>+			errno = old_errno;
>+			if (!sel(de)) continue;
>+		}

Should this not restore errrno to 0 for the next code?

>+			if (!sel(de)) {
>+				errno = 0;
>+				continue;
>+			}
>+			errno = 0;

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Luca Kellermann]

On Thu, Jul 10, 2025 at 11:57:11PM -0400, Rich Felker wrote:
> On Thu, Jul 10, 2025 at 09:19:24PM +0200, Markus Wichmann wrote:
> > Am Thu, Jul 10, 2025 at 08:51:30PM +0200 schrieb Luca Kellermann:
> > > opendir(), sel(), closedir(), or cmp() might act upon a cancellation
> > > request. because scandir() did not install a cancellation cleanup
> > > handler, this could lead to memory and file descriptor leaks.
> >
> > scandir() is on the "may be cancel-point" list in POSIX-2024, so it
> > would also be acceptable to just disable cancellation for the duration
> > of the function (since it's not on the "shall be cancel-point" list).
>
> Yes, generally we do not support cancellation for the "may be a
> cancellation point" functions except in some trivial cases. Here it's
> complicated by the fact that we're calling back to application code,
> and scandir is underspecified in that it doesn't actually specify what
> happens if the sel or compar function does not return in the normative
> text. However, the APPLICATION USAGE section mentions this possibility
> and advises applications not to do it, but doesn't seem to forbid it.

So should I update the patch to disable cancellation? It would
definitely simplify the code. If so, should it be disabled before or
after the call to opendir()? There is no need for a cancellation
cleanup handler if opendir() acts upon a cancellation request.

Would I use pthread_setcancelstate() or __pthread_setcancelstate() to
disable cancellation? Some functions in musl use
pthread_setcancelstate() (e.g. sem_open()), others use
__pthread_setcancelstate() (e.g. pthread_join()). I think I don't
really understand hidden and weak_alias and how/why musl uses them.

Luca

Re: [PATCH 1/4] scandir: hide that errno is set to 0

[Luca Kellermann]

On Fri, Jul 11, 2025 at 07:15:20PM +0000, Thorsten Glaser wrote:
> Luca Kellermann dixit:
>
> >+              if (sel) {
> >+                      /* sel() must not observe that errno was set to 0. */
> >+                      errno = old_errno;
> >+                      if (!sel(de)) continue;
> >+              }
>
> Should this not restore errrno to 0 for the next code?

The other functions called in the loop (realloc(), malloc(), memcpy())
shouldn't care about the value of errno. The controlling expression of
the while loop already takes care of setting errno to 0 before calling
readdir() again:

    while ((errno=0), (de = readdir(d))) {

Luca

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Markus Wichmann]

Am Sat, Jul 12, 2025 at 07:48:53AM +0200 schrieb Luca Kellermann:
> So should I update the patch to disable cancellation? It would
> definitely simplify the code. If so, should it be disabled before or
> after the call to opendir()? There is no need for a cancellation
> cleanup handler if opendir() acts upon a cancellation request.
> 

I would put the calls as far to the outside of that function as
possible. I don't really see a reason to do otherwise; every other
function that disables cancellation does so.

For the application, the question is "is scandir() a cancel point?", and
I would think that an unequivocal "no" is better than "sometimes" (if
the cancellation happens early enough).

> Would I use pthread_setcancelstate() or __pthread_setcancelstate() to
> disable cancellation? Some functions in musl use
> pthread_setcancelstate() (e.g. sem_open()), others use
> __pthread_setcancelstate() (e.g. pthread_join()). I think I don't
> really understand hidden and weak_alias and how/why musl uses them.
> 

That's for namespacing. The implementations of ISO-C functions must not
pull in POSIX functions, since the POSIX names aren't reserved in ISO-C,
so the application may be using them (this would cause linking errors,
or unwanted interposition in some cases). So all ISO-C functions must be
implemented only in terms of other ISO-C functions or double-underscore
functions. And that of course applies recursively.

In the case of pthread_join(), the ISO-C functions thrd_detach() and
thrd_join() reference it using the double-underscore name. Therefore it
must not use POSIX names itself, and so all functions it calls must use
double-underscores ore be ISO-C functions.

No such requirement exists for scandir(), so you should just use the
POSIX names.

Ciao,
Markus

Re: [musl] [PATCH 3/4] scandir: fix leaks caused by cancellation

[Rich Felker]

On Sat, Jul 12, 2025 at 07:48:53AM +0200, Luca Kellermann wrote:
> On Thu, Jul 10, 2025 at 11:57:11PM -0400, Rich Felker wrote:
> > On Thu, Jul 10, 2025 at 09:19:24PM +0200, Markus Wichmann wrote:
> > > Am Thu, Jul 10, 2025 at 08:51:30PM +0200 schrieb Luca Kellermann:
> > > > opendir(), sel(), closedir(), or cmp() might act upon a cancellation
> > > > request. because scandir() did not install a cancellation cleanup
> > > > handler, this could lead to memory and file descriptor leaks.
> > >
> > > scandir() is on the "may be cancel-point" list in POSIX-2024, so it
> > > would also be acceptable to just disable cancellation for the duration
> > > of the function (since it's not on the "shall be cancel-point" list).
> >
> > Yes, generally we do not support cancellation for the "may be a
> > cancellation point" functions except in some trivial cases. Here it's
> > complicated by the fact that we're calling back to application code,
> > and scandir is underspecified in that it doesn't actually specify what
> > happens if the sel or compar function does not return in the normative
> > text. However, the APPLICATION USAGE section mentions this possibility
> > and advises applications not to do it, but doesn't seem to forbid it.
> 
> So should I update the patch to disable cancellation? It would
> definitely simplify the code. If so, should it be disabled before or
> after the call to opendir()? There is no need for a cancellation
> cleanup handler if opendir() acts upon a cancellation request.

If the function is not a cancellation point, it shouldn't act on
cancellation at any point during it, so I think it should be blocked
across the open and close.

I'm not sure if it's strictly conforming for the blocking of
cancellation to be exposed to the callbacks to application-provided
functions though. While it's arguably *nicer* to leave it blocked,
POSIX grants us the choice of whether scandir should be a cancellation
point, not of whether it calls the callbacks with altered thread
state. So I think we probably need to just block around the opendir
and closedir calls, but otherwise leave it unblocked.

Rich

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Luca Kellermann]

On Sat, Jul 12, 2025 at 10:44:56AM -0400, Rich Felker wrote:
> So I think we probably need to just block around the opendir
> and closedir calls, but otherwise leave it unblocked.

In that case, the cancellation cleanup handler will still be needed
to prevent leaks when sel() or cmp() act upon a cancellation request.

Luca

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Rich Felker]

On Sun, Jul 20, 2025 at 06:46:20AM +0200, Luca Kellermann wrote:
> On Sat, Jul 12, 2025 at 10:44:56AM -0400, Rich Felker wrote:
> > So I think we probably need to just block around the opendir
> > and closedir calls, but otherwise leave it unblocked.
> 
> In that case, the cancellation cleanup handler will still be needed
> to prevent leaks when sel() or cmp() act upon a cancellation request.

As documented in the application usage notes in the spec, if these
functions don't return (it mentions longjmp) it's expected that
resources may not be freed.

I'm unclear why they put such a non-normative note there rather than
normative text saying the behavior is undefined if these functions
don't satisfy their contract to return a value (and thereby, not to
cause cancellation to be acted upon, not call longjmp to leave the
caller's context, etc.). Maybe this should be opened as a bug against
the spec. For at least some of the other functions that take
callbacks, it's explicitly undefined if they don't return normally.

Rich

[PATCH v2 1/4] scandir: hide that errno is set to 0

[Luca Kellermann]

POSIX.1-2024 requires that standard functions don't set errno to 0.
commit dae17a1aaf25d8333e729173d86659066607d87d ensured that cmp() and
the caller of scandir() cannot observe that errno is set to 0
internally. however, this was not yet the case for the sel() callback.

the following program demonstrates the issue:

#include <assert.h>
#include <dirent.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
static int sel(const struct dirent *e)
{
	/* XSH errno / XSH 2.3 Error Numbers: "No function in this
	 * volume of POSIX.1-2024 shall set errno to [0 / zero]." */
	assert(errno != 0);
	return 1;
}
int main(void)
{
	/* XSH errno: "The value of errno in the initial thread shall
	 * be zero at program startup [...]" */
	assert(errno == 0);
	errno = 1;
	struct dirent **es = 0;
	int n = scandir(".", &es, sel, alphasort);
	if (n == -1) {
		perror("scandir");
		exit(EXIT_FAILURE);
	}
	while (n > 0) free(es[--n]);
	free(es);
}

$ ./scandir-errno
Assertion failed: errno != 0 (scandir-errno.c: sel: 10)
Aborted (core dumped)
---
 src/dirent/scandir.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index 7456b9b8..eead7e50 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -17,7 +17,11 @@ int scandir(const char *path, struct dirent ***res,
 	if (!d) return -1;
 
 	while ((errno=0), (de = readdir(d))) {
-		if (sel && !sel(de)) continue;
+		if (sel) {
+			/* sel() must not observe that errno was set to 0. */
+			errno = old_errno;
+			if (!sel(de)) continue;
+		}
 		if (cnt >= len) {
 			len = 2*len+1;
 			if (len > SIZE_MAX/sizeof *names) break;
@@ -37,6 +41,7 @@ int scandir(const char *path, struct dirent ***res,
 		free(names);
 		return -1;
 	}
+	/* 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);
-- 
2.43.0

[PATCH v2 2/4] scandir: don't examine errno after closedir()

[Luca Kellermann]

when closedir() set errno, scandir() misinterpreted this as a failure.
this was wrong for two reasons:

* if closedir() succeeds, errno could still have been set, e.g. by
  __aio_close().

* even if closedir() "fails", it always closes the file descriptor and
  frees memory, so there is no reason to free all directory entries
  and return from scandir() with a failure.

the following program demonstrates the issue with aio:

#include <aio.h>
#include <dirent.h>
#include <errno.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
static void die(const char *msg)
{
	perror(msg);
	exit(EXIT_FAILURE);
}
int main(void)
{
	struct aiocb cb = {
		.aio_fildes = STDIN_FILENO,
		.aio_buf = &(char){0},
		.aio_nbytes = 1,
		.aio_sigevent.sigev_notify = SIGEV_NONE,
	};
	if (aio_read(&cb)) die("aio_read");
	struct dirent **es = 0;
	int e, n = scandir(".", &es, 0, alphasort);
	if (n == -1) die("scandir");
	while (n > 0) free(es[--n]);
	free(es);
	if (aio_suspend(&(const struct aiocb *){&cb}, 1, 0))
		die("aio_suspend");
	if ((e = aio_error(&cb)) == -1) die("aio_error");
	if (aio_return(&cb) == -1) {
		if (!e) die("aio_return");
		errno = e;
		die("read");
	}
}

$ ./scandir-aio
scandir: No such file or directory
---
 src/dirent/scandir.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index eead7e50..cf668535 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -12,7 +12,7 @@ int scandir(const char *path, struct dirent ***res,
 	DIR *d = opendir(path);
 	struct dirent *de, **names=0, **tmp;
 	size_t cnt=0, len=0;
-	int old_errno = errno;
+	int old_errno = errno, err;
 
 	if (!d) return -1;
 
@@ -33,12 +33,18 @@ int scandir(const char *path, struct dirent ***res,
 		if (!names[cnt]) break;
 		memcpy(names[cnt++], de, de->d_reclen);
 	}
+	/* closedir() might set errno via __aio_close(). It might also "fail"
+	 * (return -1 and set errno). But even then, the file descriptor is
+	 * closed and memory is freed, so there is no reason to report the
+	 * "failure" of closedir() as a failure of scandir(). */
+	err = errno;
 
 	closedir(d);
 
-	if (errno) {
+	if (err) {
 		if (names) while (cnt-->0) free(names[cnt]);
 		free(names);
+		errno = err;
 		return -1;
 	}
 	/* cmp() and caller must not observe that errno was set to 0. */
-- 
2.43.0

[PATCH v2 3/4] scandir: disable cancellation around cancellation points

[Luca Kellermann]

opendir() or closedir() might act upon a cancellation request. because
scandir() did not disable cancellation or install a cancellation
cleanup handler, this could lead to memory and file descriptor leaks.

the following program demonstrates the leaks:

#include <dirent.h>
#include <errno.h>
#include <pthread.h>
#include <semaphore.h>
#include <stdio.h>
#include <stdlib.h>
static void die(const char *msg)
{
	int cs, e = errno;
	if (!pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs)) {
		errno = e;
		perror(msg);
	}
	_Exit(EXIT_FAILURE);
}
static int posted;
static sem_t sem;
static int sel(const struct dirent *e)
{
	if (!posted && sem_post(&sem)) die("sem_post");
	posted = 1;
	return 1;
}
static void *run(void *arg)
{
	posted = 0;
	struct dirent **es = 0;
	int n = scandir(".", &es, sel, alphasort);
	if (n == -1) die("scandir");
	while (n > 0) free(es[--n]);
	free(es);
	return 0;
}
int main(void)
{
	if (sem_init(&sem, 0, 0)) die("sem_init");
	void *res = 0;
	do {
		pthread_t tid;
		if (errno = pthread_create(&tid, 0, run, 0))
			die("pthread_create");
		if (sem_wait(&sem)) die("sem_wait");
		if (errno = pthread_cancel(tid))
			die("pthread_cancel");
		if (errno = pthread_join(tid, &res))
			die("pthread_join");
	} while (res != PTHREAD_CANCELED);
	if (sem_destroy(&sem)) die("sem_destroy");
}

$ valgrind -q --leak-check=full --show-leak-kinds=all ./scandir-leak
==31293== 296 bytes in 9 blocks are indirectly lost in loss record 1 of 3
==31293==    at 0x48AF828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C500: scandir (scandir.c:32)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==
==31293== 416 (120 direct, 296 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 3
==31293==    at 0x48B6B80: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C4EA: scandir (scandir.c:28)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==
==31293== 2,072 bytes in 1 blocks are definitely lost in loss record 3 of 3
==31293==    at 0x48B6953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==31293==    by 0x401C2E0: opendir (opendir.c:15)
==31293==    by 0x401C491: scandir (scandir.c:12)
==31293==    by 0x10933F: run (scandir-leak.c:28)
==31293==    by 0x405E653: start (pthread_create.c:207)
==31293==    by 0x4060DEA: ??? (clone.s:22)
==31293==

$ strace -fe t=open,close ./scandir-leak
strace: Process 27136 attached
[pid 27136] open(".", O_RDONLY|O_LARGEFILE|O_CLOEXEC|O_DIRECTORY) = 3
[pid 27136] --- SIGRT_1 {si_signo=SIGRT_1, si_code=SI_TKILL, si_pid=27135, si_uid=1000} ---
[pid 27136] +++ exited with 0 +++
+++ exited with 0 +++
---
 src/dirent/scandir.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index cf668535..4f91af9c 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -1,4 +1,5 @@
 #include <dirent.h>
+#include <pthread.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -9,11 +10,20 @@ int scandir(const char *path, struct dirent ***res,
 	int (*sel)(const struct dirent *),
 	int (*cmp)(const struct dirent **, const struct dirent **))
 {
-	DIR *d = opendir(path);
+	DIR *d;
 	struct dirent *de, **names=0, **tmp;
 	size_t cnt=0, len=0;
-	int old_errno = errno, err;
+	int old_errno = errno, err, cs;
 
+	/* opendir() and closedir() are cancellation points. scandir() is also
+	 * allowed to be a cancellation point but we choose not to make it one.
+	 * To avoid calling sel() and cmp() with altered thread state,
+	 * cancellation is not explicitly disabled for those calls. This means
+	 * if either of the callbacks acts upon a cancellation request, there
+	 * can be memory and file descriptor leaks. */
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+	d = opendir(path);
+	pthread_setcancelstate(cs, 0);
 	if (!d) return -1;
 
 	while ((errno=0), (de = readdir(d))) {
@@ -39,7 +49,9 @@ int scandir(const char *path, struct dirent ***res,
 	 * "failure" of closedir() as a failure of scandir(). */
 	err = errno;
 
+	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	closedir(d);
+	pthread_setcancelstate(cs, 0);
 
 	if (err) {
 		if (names) while (cnt-->0) free(names[cnt]);
-- 
2.43.0

[PATCH v2 4/4] scandir: report ENOMEM and EOVERFLOW

[Luca Kellermann]

if the loop is exited because len * sizeof *names does not fit into
size_t, errno should be explicitly set to ENOMEM. previously, the
behavior differed depending on which value errno happened to have at
this point.

if cnt reached a value > INT_MAX, scandir() returned an incorrect
value. EOVERFLOW should be reported instead.

it's unlikely that these errors can actually occur. it may not even
be possible to have directories with that many entries, and even then
malloc() or realloc() will probably fail long before len or cnt reach
those large values.
---
 src/dirent/scandir.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/dirent/scandir.c b/src/dirent/scandir.c
index 4f91af9c..9e4e623f 100644
--- a/src/dirent/scandir.c
+++ b/src/dirent/scandir.c
@@ -1,4 +1,5 @@
 #include <dirent.h>
+#include <limits.h>
 #include <pthread.h>
 #include <string.h>
 #include <stdlib.h>
@@ -32,9 +33,16 @@ int scandir(const char *path, struct dirent ***res,
 			errno = old_errno;
 			if (!sel(de)) continue;
 		}
+		if (cnt >= INT_MAX) {
+			errno = EOVERFLOW;
+			break;
+		}
 		if (cnt >= len) {
 			len = 2*len+1;
-			if (len > SIZE_MAX/sizeof *names) break;
+			if (len > SIZE_MAX/sizeof *names) {
+				errno = ENOMEM;
+				break;
+			}
 			tmp = realloc(names, len * sizeof *names);
 			if (!tmp) break;
 			names = tmp;
-- 
2.43.0

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Luca Kellermann]

On Sun, Jul 20, 2025 at 00:55:24AM -0400, Rich Felker wrote:
> I'm unclear why they put such a non-normative note there rather than
> normative text saying the behavior is undefined if these functions
> don't satisfy their contract to return a value (and thereby, not to
> cause cancellation to be acted upon, not call longjmp to leave the
> caller's context, etc.). Maybe this should be opened as a bug against
> the spec. For at least some of the other functions that take
> callbacks, it's explicitly undefined if they don't return normally.

If anyone files a bug, it could maybe also include these other issues
I noticed when reading the spec for scandir():

* The spec talks about "strings" but scandir() actually allocates and
  returns dirent structures.
* The RETURN VALUE section does not explicitly say that errno shall
  be set on error.
* The code in the EXAMPLES section calls free(namelist) even if
  scandir() fails.
* compar is not allowed to be a null pointer. That might be
  intentional though.

Luca

Re: [PATCH 3/4] scandir: fix leaks caused by cancellation

[Haelwenn (lanodan) Monnier]

[2025-07-21 03:08:14+0200] Luca Kellermann:
>On Sun, Jul 20, 2025 at 00:55:24AM -0400, Rich Felker wrote:
>> I'm unclear why they put such a non-normative note there rather than
>> normative text saying the behavior is undefined if these functions
>> don't satisfy their contract to return a value (and thereby, not to
>> cause cancellation to be acted upon, not call longjmp to leave the
>> caller's context, etc.). Maybe this should be opened as a bug against
>> the spec. For at least some of the other functions that take
>> callbacks, it's explicitly undefined if they don't return normally.
>
>If anyone files a bug, it could maybe also include these other issues
>I noticed when reading the spec for scandir():
>
>* The spec talks about "strings" but scandir() actually allocates and
>  returns dirent structures.
>* The RETURN VALUE section does not explicitly say that errno shall
>  be set on error.
>* The code in the EXAMPLES section calls free(namelist) even if
>  scandir() fails.
>* compar is not allowed to be a null pointer. That might be
>  intentional though.
>
>Luca

By the way, scandir() errorneously talking about "strings"
as well as free(namelist) has been addressed last week in
http://austingroupbugs.net/view.php?id=1931#c7222

Although free(NULL) is a no-op so free(namelist) should still
be fine in case of failure.

Best regards