[pacman-dev] [PATCH] Check return value of chdir and getwd

[Dan McGee]

Typo in patch title ^^^, we use getcwd

On Sun, Jun 27, 2010 at 5:40 AM, Allan McRae <allan at archlinux.org> wrote:
> Prevents compiler warnings when building with -D_FORTIFY_SOURCE=2
>
> Signed-off-by: Allan McRae <allan at 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