From patchwork Thu Aug 9 18:49:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luke Shumaker X-Patchwork-Id: 729 Return-Path: Delivered-To: patchwork@archlinux.org Received: from apollo.archlinux.org (localhost [127.0.0.1]) by apollo.archlinux.org (Postfix) with ESMTP id 872EF619FD71 for ; Thu, 9 Aug 2018 18:49:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on apollo X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no version=3.4.1 X-Spam-BL-Results: [127.0.9.2] Received: from orion.archlinux.org (orion.archlinux.org [IPv6:2a01:4f8:160:6087::1]) by apollo.archlinux.org (Postfix) with ESMTPS for ; Thu, 9 Aug 2018 18:49:24 +0000 (UTC) Received: from orion.archlinux.org (localhost [127.0.0.1]) by orion.archlinux.org (Postfix) with ESMTP id 54F67CAA68BC3; Thu, 9 Aug 2018 18:49:20 +0000 (UTC) Received: from luna.archlinux.org (luna.archlinux.org [5.9.250.164]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by orion.archlinux.org (Postfix) with ESMTPS; Thu, 9 Aug 2018 18:49:20 +0000 (UTC) Received: from luna.archlinux.org (luna.archlinux.org [127.0.0.1]) by luna.archlinux.org (Postfix) with ESMTP id 3B3F72B867; Thu, 9 Aug 2018 18:49:20 +0000 (UTC) Authentication-Results: luna.archlinux.org; dkim=none Received: from luna.archlinux.org (luna.archlinux.org [127.0.0.1]) by luna.archlinux.org (Postfix) with ESMTP id E67FB2B866 for ; Thu, 9 Aug 2018 18:49:15 +0000 (UTC) Received: from orion.archlinux.org (orion.archlinux.org [88.198.91.70]) by luna.archlinux.org (Postfix) with ESMTPS for ; Thu, 9 Aug 2018 18:49:15 +0000 (UTC) Received: from orion.archlinux.org (localhost [127.0.0.1]) by orion.archlinux.org (Postfix) with ESMTP id 955DBCAA68BC0 for ; Thu, 9 Aug 2018 18:49:11 +0000 (UTC) Received: from mav.lukeshu.com (mav.lukeshu.com [104.207.138.63]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by orion.archlinux.org (Postfix) with ESMTPS for ; Thu, 9 Aug 2018 18:49:11 +0000 (UTC) Received: from build64-par (unknown [IPv6:2601:803:202:9275:da50:e6ff:fe00:4a5b]) by mav.lukeshu.com (Postfix) with ESMTPSA id EE27080502 for ; Thu, 9 Aug 2018 14:49:09 -0400 (EDT) From: Luke Shumaker To: pacman-dev@archlinux.org Date: Thu, 9 Aug 2018 14:49:09 -0400 Message-Id: <20180809184909.21304-1-lukeshu@lukeshu.com> X-Mailer: git-send-email 2.18.0 Subject: [pacman-dev] [PATCH] makepkg: Handle errors when canonicalizing paths X-BeenThere: pacman-dev@archlinux.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: Discussion list for pacman development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Discussion list for pacman development Errors-To: pacman-dev-bounces@archlinux.org Sender: "pacman-dev" From: Luke Shumaker If you don't have adequate permission to `cd` to one of the DEST directories, it's clear that that something's not doing error handling right: $ install -d -m0000 srcdest $ SRCDEST=$PWD/srcdest makepkg /usr/share/makepkg/util/util.sh: line 75: cd: /.../srcdest: Permission denied ==> ERROR: Failed to change to directory /.../srcdest Aborting... ==> ERROR: Failed to create the directory $SRCDEST (). Aborting... The first error comes from cd_safe() inside of canonicalize_path(). Because that bit runs in a subshell, it doesn't actually abort right there. Then, makepkg.sh doesn't check for failures from canonicalize_path. So then the variable gets set to an empty string (which makes the next error message from ensure_writable_dir() look funny), and keeps going. As an additional "fun" quirk, canonicalize_path doesn't canonicalize it if ensure_writable_dir has any work to do. That's probably not intended, but I'm not sure it is wrong either (see below). 1. Combine canonicalize_path() and ensure_writable_dir() in to canonicalize_and_ensure_writable_dir(). Both are only ever called on exactly the same variables (except that ensure_writable_dir() might skip PKGDEST, SRCPKGDEST, or LOGDEST; depending on the flags). 2. Use plain `cd` and check the status ourselves, rather than `cd_safe`. We can come up with a better (context-aware) error message than `cd_safe`, and we avoid duplicating the "Aborting..." message. This has a few consequences: a. We no longer canonicalize dirpaths that we don't ensure are writable. That's probably safe. This means that makepkg might now error in fewer cases; for example: $ install -d -m0000 pkgdest $ PKGDEST=$PWD/pkgdest makepkg -g There's no reason for it to care that it can't `cd` to PKGDEST; it won't be using it. b. We now canonicalize paths that we create that didn't exist before. That said, I'm not sure why we ever need to canonicalize the paths; I'm not sure why c0f58ea was necessary even after 960c2cd was already applied. --- scripts/libmakepkg/util/util.sh.in | 31 +++++++++++------------------- scripts/makepkg.sh.in | 14 +++++++------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index e1ca5cb7..1dfe6da9 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -49,20 +49,6 @@ is_array() { return $ret } -# Canonicalize a directory path if it exists -canonicalize_path() { - local path="$1"; - - if [[ -d $path ]]; then - ( - cd_safe "$path" - pwd -P - ) - else - printf "%s\n" "$path" - fi -} - dir_is_empty() { ( shopt -s dotglob nullglob @@ -79,10 +65,10 @@ cd_safe() { fi } -# Try to create directory if one does not yet exist. Fails if the directory -# exists but has no write permissions, or if there is an existing file with -# the same name. -ensure_writable_dir() { +# Try to create directory if one does not yet exist, and prints the +# canonical path to the directory. Fails if the directory exists but has no +# write permissions, or if there is an existing file with the same name. +canonicalize_and_ensure_writable_dir() { local dirtype="$1" dirpath="$2" if ! mkdir -p "$dirpath" 2>/dev/null; then @@ -92,6 +78,11 @@ ensure_writable_dir() { error "$(gettext "You do not have write permission for the directory \$%s (%s).")" "$dirtype" "$dirpath" return 1 fi - - return 0 + ( + if ! cd "$dirpath"; then + error "$(gettext "Could not canonicalize the directory \$%s (%s).")" "$dirtype" "$dirpath" + return 1 + fi + pwd -P + ) } diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8ee0f5c5..62c2db80 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1353,9 +1353,9 @@ if (( ${#extra_environment[*]} )); then export "${extra_environment[@]}" fi -# canonicalize paths and provide defaults if anything is still undefined +# provide defaults if anything is still undefined for var in PKGDEST SRCDEST SRCPKGDEST LOGDEST BUILDDIR; do - printf -v "$var" "$(canonicalize_path "${!var:-$startdir}")" + printf -v "$var" "${!var:-$startdir}" done unset var PACKAGER=${PACKAGER:-"Unknown Packager"} @@ -1378,23 +1378,23 @@ lint_config || exit $E_CONFIG_ERROR # check that all settings directories are user-writable -if ! ensure_writable_dir "BUILDDIR" "$BUILDDIR"; then +if ! BUILDDIR=$(canonicalize_and_ensure_writable_dir "BUILDDIR" "$BUILDDIR"); then plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi -if (( ! (NOBUILD || GENINTEG) )) && ! ensure_writable_dir "PKGDEST" "$PKGDEST"; then +if (( ! (NOBUILD || GENINTEG) )) && ! PKGDEST=$(canonicalize_and_ensure_writable_dir "PKGDEST" "$PKGDEST"); then plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi -if ! ensure_writable_dir "SRCDEST" "$SRCDEST" ; then +if ! SRCDEST=$(canonicalize_and_ensure_writable_dir "SRCDEST" "$SRCDEST"); then plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi if (( SOURCEONLY )); then - if ! ensure_writable_dir "SRCPKGDEST" "$SRCPKGDEST"; then + if ! SRCPKGDEST=$(canonicalize_and_ensure_writable_dir "SRCPKGDEST" "$SRCPKGDEST"); then plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi @@ -1404,7 +1404,7 @@ if (( SOURCEONLY )); then IGNOREARCH=1 fi -if (( LOGGING )) && ! ensure_writable_dir "LOGDEST" "$LOGDEST"; then +if (( LOGGING )) && ! LOGDEST=$(canonicalize_and_ensure_writable_dir "LOGDEST" "$LOGDEST"); then plain "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi