[pacman-dev] [PATCH] Check return value of chdir and getwd
Prevents compiler warnings when building with -D_FORTIFY_SOURCE=2 Signed-off-by: Allan McRae <allan@archlinux.org> --- In commit_single_pkg in lib/libalpm/add.c the failure of changing directory has now become an error. From the comment above it, it looks like it should be an error if it fails, but I have been wrong before... lib/libalpm/add.c | 18 +++++++++++++----- lib/libalpm/util.c | 8 ++++++-- src/pacman/pacman.c | 17 +++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f39a0ec..e45073e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -556,6 +556,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, struct archive *archive; struct archive_entry *entry; char cwd[PATH_MAX] = ""; + int restore_cwd = 0; _alpm_log(PM_LOG_DEBUG, "extracting files\n"); @@ -579,11 +580,16 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, /* save the cwd so we can restore it later */ if(getcwd(cwd, PATH_MAX) == NULL) { _alpm_log(PM_LOG_ERROR, _("could not get current working directory\n")); - cwd[0] = 0; + } else { + restore_cwd = 1; } /* libarchive requires this for extracting hard links */ - chdir(handle->root); + if(chdir(handle->root) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); + ret = -1; + goto cleanup; + } /* call PROGRESS once with 0 percent, as we sort-of skip that here */ if(is_upgrade) { @@ -629,9 +635,11 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, } archive_read_finish(archive); - /* restore the old cwd is we have it */ - if(strlen(cwd)) { - chdir(cwd); + /* restore the old cwd if we have it */ + if(restore_cwd) { + if(chdir(cwd) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } } if(errors) { diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index b033e0a..383568c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -365,7 +365,9 @@ cleanup: umask(oldmask); archive_read_finish(_archive); if(restore_cwd) { - chdir(cwd); + if(chdir(cwd) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } } return(ret); } @@ -535,7 +537,9 @@ int _alpm_run_chroot(const char *root, const char *cmd) cleanup: if(restore_cwd) { - chdir(cwd); + if(chdir(cwd) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } } return(retval); diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 78407d6..913b487 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -676,6 +676,7 @@ int download_with_xfercommand(const char *url, const char *localpath, struct stat st; char *parsedcmd,*tempcmd; char cwd[PATH_MAX]; + int restore_cwd = 0; char *destfile, *tempfile, *filename; if(!config->xfercommand) { @@ -708,8 +709,14 @@ int download_with_xfercommand(const char *url, const char *localpath, parsedcmd = strreplace(tempcmd, "%u", url); free(tempcmd); + /* save the cwd so we can restore it later */ + if(getcwd(cwd, PATH_MAX) == NULL) { + pm_printf(PM_LOG_ERROR, _("could not get current working directory\n")); + } else { + restore_cwd = 1; + } + /* cwd to the download directory */ - getcwd(cwd, PATH_MAX); if(chdir(localpath)) { pm_printf(PM_LOG_WARNING, _("could not chdir to download directory %s\n"), localpath); ret = -1; @@ -736,7 +743,13 @@ int download_with_xfercommand(const char *url, const char *localpath, } cleanup: - chdir(cwd); + /* restore the old cwd if we have it */ + if(restore_cwd) { + if(chdir(cwd) != 0) { + pm_printf(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } + } + if(ret == -1) { /* hack to let an user the time to cancel a download */ sleep(2); -- 1.7.1
Typo in patch title ^^^, we use getcwd On Sun, Jun 27, 2010 at 5:40 AM, Allan McRae <allan@archlinux.org> wrote:
Prevents compiler warnings when building with -D_FORTIFY_SOURCE=2
Signed-off-by: Allan McRae <allan@archlinux.org> ---
In commit_single_pkg in lib/libalpm/add.c the failure of changing directory has now become an error. From the comment above it, it looks like it should be an error if it fails, but I have been wrong before... Yes, it probably should be, given the bit about hardlink extraction.
lib/libalpm/add.c | 18 +++++++++++++----- lib/libalpm/util.c | 8 ++++++-- src/pacman/pacman.c | 17 +++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f39a0ec..e45073e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -556,6 +556,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, struct archive *archive; struct archive_entry *entry; char cwd[PATH_MAX] = ""; + int restore_cwd = 0; _alpm_log(PM_LOG_DEBUG, "extracting files\n");
@@ -579,11 +580,16 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, /* save the cwd so we can restore it later */ if(getcwd(cwd, PATH_MAX) == NULL) { _alpm_log(PM_LOG_ERROR, _("could not get current working directory\n")); - cwd[0] = 0; + } else { + restore_cwd = 1; }
/* libarchive requires this for extracting hard links */ - chdir(handle->root); + if(chdir(handle->root) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); + ret = -1; + goto cleanup; + }
/* call PROGRESS once with 0 percent, as we sort-of skip that here */ if(is_upgrade) { @@ -629,9 +635,11 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, } archive_read_finish(archive);
- /* restore the old cwd is we have it */ - if(strlen(cwd)) { - chdir(cwd); + /* restore the old cwd if we have it */ + if(restore_cwd) { + if(chdir(cwd) != 0) { Since there is no branching in the conditional, I'd probably prefer if(restore_cwd && chdir(cwd) != 0) {
+ _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } }
if(errors) { diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index b033e0a..383568c 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -365,7 +365,9 @@ cleanup: umask(oldmask); archive_read_finish(_archive); if(restore_cwd) { - chdir(cwd); + if(chdir(cwd) != 0) { Same as above.
+ _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } } return(ret); } @@ -535,7 +537,9 @@ int _alpm_run_chroot(const char *root, const char *cmd)
cleanup: if(restore_cwd) { - chdir(cwd); + if(chdir(cwd) != 0) { Same as above.
+ _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } }
return(retval); diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 78407d6..913b487 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -676,6 +676,7 @@ int download_with_xfercommand(const char *url, const char *localpath, struct stat st; char *parsedcmd,*tempcmd; char cwd[PATH_MAX]; + int restore_cwd = 0; char *destfile, *tempfile, *filename;
if(!config->xfercommand) { @@ -708,8 +709,14 @@ int download_with_xfercommand(const char *url, const char *localpath, parsedcmd = strreplace(tempcmd, "%u", url); free(tempcmd);
+ /* save the cwd so we can restore it later */ + if(getcwd(cwd, PATH_MAX) == NULL) { + pm_printf(PM_LOG_ERROR, _("could not get current working directory\n")); + } else { + restore_cwd = 1; + } + /* cwd to the download directory */ - getcwd(cwd, PATH_MAX); if(chdir(localpath)) { pm_printf(PM_LOG_WARNING, _("could not chdir to download directory %s\n"), localpath); ret = -1; @@ -736,7 +743,13 @@ int download_with_xfercommand(const char *url, const char *localpath, }
cleanup: - chdir(cwd); + /* restore the old cwd if we have it */ + if(restore_cwd) { + if(chdir(cwd) != 0) { Same.
+ pm_printf(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), cwd, strerror(errno)); + } + } + if(ret == -1) { /* hack to let an user the time to cancel a download */ sleep(2); -- 1.7.1
On 30/06/10 23:38, Dan McGee wrote:
Typo in patch title ^^^, we use getcwd
On Sun, Jun 27, 2010 at 5:40 AM, Allan McRae<allan@archlinux.org> wrote:
Prevents compiler warnings when building with -D_FORTIFY_SOURCE=2
Signed-off-by: Allan McRae<allan@archlinux.org> ---
In commit_single_pkg in lib/libalpm/add.c the failure of changing directory has now become an error. From the comment above it, it looks like it should be an error if it fails, but I have been wrong before... Yes, it probably should be, given the bit about hardlink extraction.
lib/libalpm/add.c | 18 +++++++++++++----- lib/libalpm/util.c | 8 ++++++-- src/pacman/pacman.c | 17 +++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f39a0ec..e45073e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -556,6 +556,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, struct archive *archive; struct archive_entry *entry; char cwd[PATH_MAX] = ""; + int restore_cwd = 0; _alpm_log(PM_LOG_DEBUG, "extracting files\n");
@@ -579,11 +580,16 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, /* save the cwd so we can restore it later */ if(getcwd(cwd, PATH_MAX) == NULL) { _alpm_log(PM_LOG_ERROR, _("could not get current working directory\n")); - cwd[0] = 0; + } else { + restore_cwd = 1; }
/* libarchive requires this for extracting hard links */ - chdir(handle->root); + if(chdir(handle->root) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); + ret = -1; + goto cleanup; + }
/* call PROGRESS once with 0 percent, as we sort-of skip that here */ if(is_upgrade) { @@ -629,9 +635,11 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, } archive_read_finish(archive);
- /* restore the old cwd is we have it */ - if(strlen(cwd)) { - chdir(cwd); + /* restore the old cwd if we have it */ + if(restore_cwd) { + if(chdir(cwd) != 0) { Since there is no branching in the conditional, I'd probably prefer if(restore_cwd&& chdir(cwd) != 0) {
<snip> All is fixed as suggested on my working branch. Allan
participants (2)
-
Allan McRae
-
Dan McGee