[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250710185131.186-3-mailto.luca.kellermann@gmail.com>
Date: Thu, 10 Jul 2025 20:51:30 +0200
From: Luca Kellermann <mailto.luca.kellermann@...il.com>
To: musl@...ts.openwall.com
Subject: [PATCH 3/4] scandir: fix leaks caused by cancellation
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
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.
