From c0899e5b913b70f40b4ac4a314d18437f82ad5bc Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Mon, 14 Mar 2022 09:18:28 +0200 Subject: [PATCH 1/2] Fix file descriptor leak regression on install (#1947) Commit 0e3024ca3e7450104e70ec8d213cf223e71f7c02 introduced a leak on directory file descriptors from hardlinked sets, preventing some large packages with many hardlinks from installing at all. fsmMkfile() needs to close the firstdir fd when done with it because that's the only place that knows when it's safe to do so. However, there could be non-hardlink entries left in the same directory, so we must not close *that* descriptor. Dup the firstdir descriptor so we're free to close it without worrying about the other state. Fixes: #1947 --- lib/fsm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/fsm.c b/lib/fsm.c index 4859d62ee6..5a74b43cd7 100644 --- a/lib/fsm.c +++ b/lib/fsm.c @@ -208,7 +208,7 @@ static int fsmMkfile(int dirfd, rpmfi fi, struct filedata_s *fp, rpmfiles files, if (fp->sb.st_nlink > 1) { *firstlink = fp; *firstlinkfile = fd; - *firstdir = dirfd; + *firstdir = dup(dirfd); } } else { /* Create hard links for others and avoid redundant metadata setting */ @@ -227,7 +227,7 @@ static int fsmMkfile(int dirfd, rpmfi fi, struct filedata_s *fp, rpmfiles files, fp->setmeta = 1; *firstlink = NULL; *firstlinkfile = -1; - *firstdir = -1; + fsmClose(firstdir); } } *fdp = fd; @@ -841,8 +841,7 @@ static int onChdir(rpmfi fi, void *data) struct diriter_s *di = data; if (di->dirfd >= 0) { - if (di->dirfd != di->firstdir) - close(di->dirfd); + close(di->dirfd); di->dirfd = -1; } return 0; From 661a19f08f07fdf1e4b246f8d8bbcfc193531033 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Mon, 14 Mar 2022 09:52:21 +0200 Subject: [PATCH 2/2] Use fsmClose() for closing file descriptors everywhere within fsm fsmClose() does all the necessary checks and resets the fd to -1 after close, why duplicate these all over the code when we already have a function... There's no difference wrt file descriptors getting closed here, but there is a side-effect to this: if %_flush_io is enabled, they now get fsync() on the associated directories too, which I think is a good thing for those who care about it. --- lib/fsm.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/lib/fsm.c b/lib/fsm.c index 5a74b43cd7..28eb9431bd 100644 --- a/lib/fsm.c +++ b/lib/fsm.c @@ -66,6 +66,7 @@ struct filedata_s { */ static const char * fileActionString(rpmFileAction a); static int fsmOpenat(int dirfd, const char *path, int flags, int dir); +static int fsmClose(int *wfdp); /** \ingroup payload * Build path to file from file info, optionally ornamented with suffix. @@ -101,7 +102,7 @@ static int cap_set_fileat(int dirfd, const char *path, cap_t fcaps) int fd = fsmOpenat(dirfd, path, O_RDONLY|O_NOFOLLOW, 0); if (fd >= 0) { rc = cap_set_fd(fd, fcaps); - close(fd); + fsmClose(&fd); } return rc; } @@ -326,8 +327,7 @@ static int fsmOpenat(int dirfd, const char *path, int flags, int dir) /* O_DIRECTORY equivalent */ if (dir && fd >= 0 && fstat(fd, &sb) == 0 && !S_ISDIR(sb.st_mode)) { errno = ENOTDIR; - close(fd); - fd = -1; + fsmClose(&fd); } return fd; } @@ -395,7 +395,7 @@ static int ensureDir(rpmPlugins plugins, const char *p, int owned, int create, rc = fsmDoMkDir(plugins, dirfd, bn, apath, owned, mode, &fd); } - close(dirfd); + fsmClose(&dirfd); if (fd >= 0) { dirfd = fd; } else { @@ -411,9 +411,8 @@ static int ensureDir(rpmPlugins plugins, const char *p, int owned, int create, } if (rc) { - close(fd); - close(dirfd); - dirfd = -1; + fsmClose(&fd); + fsmClose(&dirfd); } else { rc = 0; } @@ -840,10 +839,7 @@ static int onChdir(rpmfi fi, void *data) { struct diriter_s *di = data; - if (di->dirfd >= 0) { - close(di->dirfd); - di->dirfd = -1; - } + fsmClose(&(di->dirfd)); return 0; } @@ -861,14 +857,8 @@ static rpmfi fsmIter(FD_t payload, rpmfiles files, rpmFileIter iter, void *data) static rpmfi fsmIterFini(rpmfi fi, struct diriter_s *di) { - if (di->dirfd >= 0) { - close(di->dirfd); - di->dirfd = -1; - } - if (di->firstdir >= 0) { - close(di->firstdir); - di->firstdir = -1; - } + fsmClose(&(di->dirfd)); + fsmClose(&(di->firstdir)); return rpmfiFree(fi); }