[pacman-dev] [PATCH] Use O_CLOEXEC as much as possible when opening files
When calling open(), use O_CLOEXEC as much as possible to ensure the file descriptor is closed when and if a process using libalpm forks. For most of these cases, and especially in utility functions, the file descriptor is opened and closed in the same function, so we don't have too much to worry about. However, for things like the log file and database lock file, we should ensure descriptors aren't left hanging around for children to touch. This patch is inspired by the problem in FS#36161, where an open file descriptor to the current working directory prevents chroot() from working on FreeBSD. We don't need this file descriptor in the child process, so open it (and now several others) with O_CLOEXEC. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/add.c | 2 +- lib/libalpm/handle.c | 2 +- lib/libalpm/log.c | 8 ++++++-- lib/libalpm/util.c | 14 +++++++------- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 8ef9ef0..2d93077 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -542,7 +542,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, } /* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..274e3bc 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -110,7 +110,7 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir); do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC, 0000); } while(fd == -1 && errno == EINTR); if(fd >= 0) { FILE *f = fdopen(fd, "w"); diff --git a/lib/libalpm/log.c b/lib/libalpm/log.c index 271bd00..5f3b094 100644 --- a/lib/libalpm/log.c +++ b/lib/libalpm/log.c @@ -49,9 +49,13 @@ int SYMEXPORT alpm_logaction(alpm_handle_t *handle, const char *prefix, /* check if the logstream is open already, opening it if needed */ if(handle->logstream == NULL) { - handle->logstream = fopen(handle->logfile, "a"); + int fd; + OPEN(fd, handle->logfile, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC); + if(fd >= 0) { + handle->logstream = fdopen(fd, "a"); + } /* if we couldn't open it, we have an issue */ - if(handle->logstream == NULL) { + if(fd < 0 || handle->logstream == NULL) { if(errno == EACCES) { handle->pm_errno = ALPM_ERR_BADPERMS; } else if(errno == ENOENT) { diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 1e21362..863614e 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -154,9 +154,9 @@ int _alpm_copyfile(const char *src, const char *dest) MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1); - OPEN(in, src, O_RDONLY); + OPEN(in, src, O_RDONLY | O_CLOEXEC); do { - out = open(dest, O_WRONLY | O_CREAT, 0000); + out = open(dest, O_WRONLY | O_CREAT | O_BINARY | O_CLOEXEC, 0000); } while(out == -1 && errno == EINTR); if(in < 0 || out < 0) { goto cleanup; @@ -245,7 +245,7 @@ int _alpm_open_archive(alpm_handle_t *handle, const char *path, archive_read_support_format_all(*archive); _alpm_log(handle, ALPM_LOG_DEBUG, "opening archive %s\n", path); - OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), path, strerror(errno)); @@ -327,7 +327,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix, oldmask = umask(0022); /* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } @@ -503,7 +503,7 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) int retval = 0; /* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } @@ -769,7 +769,7 @@ static int md5_file(const char *path, unsigned char output[16]) MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1); - OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { free(buf); return 1; @@ -811,7 +811,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224) MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1); - OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { free(buf); return 1; -- 1.8.3.2
On 16/07/13 04:32, Dan McGee wrote:
When calling open(), use O_CLOEXEC as much as possible to ensure the file descriptor is closed when and if a process using libalpm forks.
For most of these cases, and especially in utility functions, the file descriptor is opened and closed in the same function, so we don't have too much to worry about. However, for things like the log file and database lock file, we should ensure descriptors aren't left hanging around for children to touch.
This patch is inspired by the problem in FS#36161, where an open file descriptor to the current working directory prevents chroot() from working on FreeBSD. We don't need this file descriptor in the child process, so open it (and now several others) with O_CLOEXEC.
Signed-off-by: Dan McGee <dan@archlinux.org> ---
Compile error: CC libalpm_la-log.lo In file included from /usr/include/fcntl.h:296:0, from util.h:41, from <command-line>:27: In function 'open', inlined from 'alpm_logaction' at <command-line>:52:3: /usr/include/bits/fcntl2.h:50:24: error: call to '__open_missing_mode' declared with attribute error: open with O_CREAT in second argument needs 3 arguments __open_missing_mode (); ^
lib/libalpm/add.c | 2 +- lib/libalpm/handle.c | 2 +- lib/libalpm/log.c | 8 ++++++-- lib/libalpm/util.c | 14 +++++++------- 4 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 8ef9ef0..2d93077 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -542,7 +542,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, }
/* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..274e3bc 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -110,7 +110,7 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir);
do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC, 0000); } while(fd == -1 && errno == EINTR); if(fd >= 0) { FILE *f = fdopen(fd, "w"); diff --git a/lib/libalpm/log.c b/lib/libalpm/log.c index 271bd00..5f3b094 100644 --- a/lib/libalpm/log.c +++ b/lib/libalpm/log.c @@ -49,9 +49,13 @@ int SYMEXPORT alpm_logaction(alpm_handle_t *handle, const char *prefix,
/* check if the logstream is open already, opening it if needed */ if(handle->logstream == NULL) { - handle->logstream = fopen(handle->logfile, "a"); + int fd; + OPEN(fd, handle->logfile, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC); + if(fd >= 0) { + handle->logstream = fdopen(fd, "a"); + } /* if we couldn't open it, we have an issue */ - if(handle->logstream == NULL) { + if(fd < 0 || handle->logstream == NULL) { if(errno == EACCES) { handle->pm_errno = ALPM_ERR_BADPERMS; } else if(errno == ENOENT) { diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 1e21362..863614e 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -154,9 +154,9 @@ int _alpm_copyfile(const char *src, const char *dest)
MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
- OPEN(in, src, O_RDONLY); + OPEN(in, src, O_RDONLY | O_CLOEXEC); do { - out = open(dest, O_WRONLY | O_CREAT, 0000); + out = open(dest, O_WRONLY | O_CREAT | O_BINARY | O_CLOEXEC, 0000); } while(out == -1 && errno == EINTR); if(in < 0 || out < 0) { goto cleanup; @@ -245,7 +245,7 @@ int _alpm_open_archive(alpm_handle_t *handle, const char *path, archive_read_support_format_all(*archive);
_alpm_log(handle, ALPM_LOG_DEBUG, "opening archive %s\n", path); - OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), path, strerror(errno)); @@ -327,7 +327,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix, oldmask = umask(0022);
/* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } @@ -503,7 +503,7 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) int retval = 0;
/* save the cwd so we can restore it later */ - OPEN(cwdfd, ".", O_RDONLY); + OPEN(cwdfd, ".", O_RDONLY | O_CLOEXEC); if(cwdfd < 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not get current working directory\n")); } @@ -769,7 +769,7 @@ static int md5_file(const char *path, unsigned char output[16])
MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
- OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { free(buf); return 1; @@ -811,7 +811,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224)
MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, return 1);
- OPEN(fd, path, O_RDONLY); + OPEN(fd, path, O_RDONLY | O_CLOEXEC); if(fd < 0) { free(buf); return 1;
On Mon, Jul 15, 2013 at 7:11 PM, Allan McRae <allan@archlinux.org> wrote:
On 16/07/13 04:32, Dan McGee wrote:
When calling open(), use O_CLOEXEC as much as possible to ensure the file descriptor is closed when and if a process using libalpm forks.
For most of these cases, and especially in utility functions, the file descriptor is opened and closed in the same function, so we don't have too much to worry about. However, for things like the log file and database lock file, we should ensure descriptors aren't left hanging around for children to touch.
This patch is inspired by the problem in FS#36161, where an open file descriptor to the current working directory prevents chroot() from working on FreeBSD. We don't need this file descriptor in the child process, so open it (and now several others) with O_CLOEXEC.
Signed-off-by: Dan McGee <dan@archlinux.org> ---
Compile error:
CC libalpm_la-log.lo In file included from /usr/include/fcntl.h:296:0, from util.h:41, from <command-line>:27: In function 'open', inlined from 'alpm_logaction' at <command-line>:52:3: /usr/include/bits/fcntl2.h:50:24: error: call to '__open_missing_mode' declared with attribute error: open with O_CREAT in second argument needs 3 arguments __open_missing_mode (); ^
I just saw this too, was compiling with clang earlier and it doesn't warn on this. Weird! I'll submit an updated version later tonight. -Dan
On Mon, Jul 15, 2013 at 07:26:04PM -0500, Dan McGee wrote:
Compile error:
CC libalpm_la-log.lo In file included from /usr/include/fcntl.h:296:0, from util.h:41, from <command-line>:27: In function 'open', inlined from 'alpm_logaction' at <command-line>:52:3: /usr/include/bits/fcntl2.h:50:24: error: call to '__open_missing_mode' declared with attribute error: open with O_CREAT in second argument needs 3 arguments __open_missing_mode (); ^
I just saw this too, was compiling with clang earlier and it doesn't warn on this. Weird!
I'll submit an updated version later tonight.
Could you not use the "e" flag to fopen? It is available since glibc 2.7 (I'm not sure if there are targets that have O_CLOEXEC that don't support the "e" flag). -- Ross Lagerwall
On Tue, Jul 16, 2013 at 08:27:03PM +0200, Ross Lagerwall wrote:
On Mon, Jul 15, 2013 at 07:26:04PM -0500, Dan McGee wrote:
Compile error:
CC libalpm_la-log.lo In file included from /usr/include/fcntl.h:296:0, from util.h:41, from <command-line>:27: In function 'open', inlined from 'alpm_logaction' at <command-line>:52:3: /usr/include/bits/fcntl2.h:50:24: error: call to '__open_missing_mode' declared with attribute error: open with O_CREAT in second argument needs 3 arguments __open_missing_mode (); ^
I just saw this too, was compiling with clang earlier and it doesn't warn on this. Weird!
I'll submit an updated version later tonight.
Could you not use the "e" flag to fopen? It is available since glibc 2.7 (I'm not sure if there are targets that have O_CLOEXEC that don't support the "e" flag).
-- Ross Lagerwall
This is a glibc extension. BSD doesn't support this.
participants (5)
-
Allan McRae
-
Dan McGee
-
Dan McGee
-
Dave Reisner
-
Ross Lagerwall