On operating systems we support, the behavior is always such that the kernel will do the right thing as far as invalidating the file descriptor, regardless of the eventual return value. Therefore, potentially looping and calling close multiple times is wrong. At best, we call close again on an invalid FD and throw a spurious EBADF error. At worst, we might close an FD which doesn't belong to us when a multi-threaded application opens its own file descriptor between iterations of the loop. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- This goes further than the current patch on the ML and removes the CLOSE macro defined in alpm as well. lib/libalpm/add.c | 6 +++--- lib/libalpm/be_package.c | 10 +++++----- lib/libalpm/be_sync.c | 2 +- lib/libalpm/dload.c | 2 +- lib/libalpm/util.c | 28 ++++++++++++++-------------- lib/libalpm/util.h | 1 - src/pacman/conf.c | 5 +---- 7 files changed, 25 insertions(+), 29 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index c20e7c6..d6dc748 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -550,7 +550,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, _alpm_log(handle, ALPM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); ret = -1; goto cleanup; } @@ -580,7 +580,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, errors += extract_single_file(handle, archive, entry, newpkg, oldpkg); } _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); /* restore the old cwd if we have it */ if(cwdfd >= 0) { @@ -588,7 +588,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } if(errors) { diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 5a70968..6262621 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -76,7 +76,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) if(!changelog) { pkg->handle->pm_errno = ALPM_ERR_MEMORY; _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); return NULL; } changelog->archive = archive; @@ -86,7 +86,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) } /* we didn't find a changelog */ _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); errno = ENOENT; return NULL; @@ -126,7 +126,7 @@ static int _package_changelog_close(const alpm_pkg_t UNUSED *pkg, void *fp) int ret; struct package_changelog *changelog = fp; ret = _alpm_archive_read_free(changelog->archive); - CLOSE(changelog->fd); + close(changelog->fd); free(changelog); return ret; } @@ -477,7 +477,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); /* internal fields for package struct */ newpkg->origin = ALPM_PKG_FROM_FILE; @@ -510,7 +510,7 @@ error: _alpm_pkg_free(newpkg); _alpm_archive_read_free(archive); if(fd >= 0) { - CLOSE(fd); + close(fd); } return NULL; diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index f9fd5d1..feda6f5 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -474,7 +474,7 @@ static int sync_db_populate(alpm_db_t *db) cleanup: _alpm_archive_read_free(archive); if(fd >= 0) { - CLOSE(fd); + close(fd); } return count; } diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 7513822..b759c78 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -366,7 +366,7 @@ static FILE *create_tempfile(struct dload_payload *payload, const char *localpat fchmod(fd, ~(_getumask()) & 0666) || !(fp = fdopen(fd, payload->tempfile_openmode))) { unlink(randpath); - CLOSE(fd); + close(fd); _alpm_log(payload->handle, ALPM_LOG_ERROR, _("failed to create temporary file for download\n")); free(randpath); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index f84fb66..78cfc49 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -186,10 +186,10 @@ int _alpm_copyfile(const char *src, const char *dest) cleanup: free(buf); if(in >= 0) { - CLOSE(in); + close(in); } if(out >= 0) { - CLOSE(out); + close(out); } return ret; } @@ -275,7 +275,7 @@ error: _alpm_archive_read_free(*archive); *archive = NULL; if(fd >= 0) { - CLOSE(fd); + close(fd); } RET_ERR(handle, error, -1); } @@ -394,13 +394,13 @@ int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix, cleanup: umask(oldmask); _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); if(cwdfd >= 0) { if(fchdir(cwdfd) != 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } return ret; @@ -537,12 +537,12 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - CLOSE(1); - CLOSE(2); + close(1); + close(2); while(dup2(pipefd[1], 1) == -1 && errno == EINTR); while(dup2(pipefd[1], 2) == -1 && errno == EINTR); - CLOSE(pipefd[0]); - CLOSE(pipefd[1]); + close(pipefd[0]); + close(pipefd[1]); /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { @@ -564,10 +564,10 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) int status; FILE *pipe_file; - CLOSE(pipefd[1]); + close(pipefd[1]); pipe_file = fdopen(pipefd[0], "r"); if(pipe_file == NULL) { - CLOSE(pipefd[0]); + close(pipefd[0]); retval = 1; } else { while(!feof(pipe_file)) { @@ -609,7 +609,7 @@ cleanup: _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } return retval; @@ -784,7 +784,7 @@ static int md5_file(const char *path, unsigned char output[16]) MD5_Update(&ctx, buf, n); } - CLOSE(fd); + close(fd); free(buf); if(n < 0) { @@ -834,7 +834,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224) } } - CLOSE(fd); + close(fd); free(buf); if(n < 0) { diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 56031f3..8e577de 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -88,7 +88,6 @@ void _alpm_alloc_fail(size_t size); #endif #define OPEN(fd, path, flags) do { fd = open(path, flags | O_BINARY); } while(fd == -1 && errno == EINTR) -#define CLOSE(fd) do { int _ret; do { _ret = close(fd); } while(_ret == -1 && errno == EINTR); } while(0) /** * Used as a buffer/state holder for _alpm_archive_fgets(). diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 5adc96c..2985aba 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -257,14 +257,11 @@ static int download_with_xfercommand(const char *url, const char *localpath, cleanup: /* restore the old cwd if we have it */ if(cwdfd >= 0) { - int close_ret; if(fchdir(cwdfd) != 0) { pm_printf(ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - do { - close_ret = close(cwdfd); - } while(close_ret == -1 && errno == EINTR); + close(cwdfd); } if(ret == -1) { -- 1.8.3.2