[pacman-dev] [PATCH 0/6] fix some safety issues in makepkg
With the upcoming makepkg-lobotomy (removing the error trap apart from in the actual packaging steps), we need to add some more error checking and be as safe as possible when potentially dealing with strange variable contents. Some of these are a bit unnecessary (e.g. using printf to output variables that almost certainly cannot start with a "-n"), but there is no harm in applying them globally. Allan McRae (6): makepkg: the rhs in string comparisons should be quoted makepkg: use printf rather than echo to output variable makepkg: prevent issues with files starting with a hyphen makepkg: abort when failing to create BUILDDIR makepkg: quote removed filename as it can have spaces makepkg: safely change directories scripts/makepkg.sh.in | 147 ++++++++++++++++++++++++++----------------------- 1 file changed, 79 insertions(+), 68 deletions(-) -- 1.7.9.3
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8763ffb..601c7e2 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -311,7 +311,7 @@ in_opt_array() { local opt for opt in "$@"; do - if [[ $opt = $needle ]]; then + if [[ $opt = "$needle" ]]; then echo 'y' # Enabled return elif [[ $opt = "!$needle" ]]; then @@ -333,7 +333,7 @@ in_array() { local needle=$1; shift local item for item in "$@"; do - [[ $item = $needle ]] && return 0 # Found + [[ $item = "$needle" ]] && return 0 # Found done return 1 # Not Found } @@ -357,7 +357,7 @@ get_downloadclient() { local i for i in "${DLAGENTS[@]}"; do local handler="${i%%::*}" - if [[ $proto = $handler ]]; then + if [[ $proto = "$handler" ]]; then local agent="${i##*::}" break fi @@ -557,7 +557,7 @@ download_sources() { local url=$(get_url "$netfile") # if we get here, check to make sure it was a URL, else fail - if [[ $file = $url ]]; then + if [[ $file = "$url" ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" exit 1 # $E_MISSING_FILE fi @@ -683,7 +683,7 @@ check_checksums() { local expectedsum=$(tr '[:upper:]' '[:lower:]' <<< "${integrity_sums[$idx]}") local realsum="$(openssl dgst -${integ} "$file")" realsum="${realsum##* }" - if [[ $expectedsum = $realsum ]]; then + if [[ $expectedsum = "$realsum" ]]; then printf -- "$(gettext "Passed")\n" >&2 else printf -- "$(gettext "FAILED")\n" >&2 @@ -842,7 +842,7 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" - if [[ $cmd = bsdtar ]]; then + if [[ $cmd = "bsdtar" ]]; then $cmd -xf "$file" || ret=$? else rm -f "${file%.*}" @@ -981,7 +981,7 @@ tidy_install() { msg2 "$(gettext "Purging unwanted files...")" local pt for pt in "${PURGE_TARGETS[@]}"; do - if [[ ${pt} = ${pt//\/} ]]; then + if [[ ${pt} = "${pt//\/}" ]]; then find . -type f -name "${pt}" -exec rm -f -- '{}' \; else rm -f ${pt} @@ -1025,7 +1025,7 @@ tidy_install() { done fi - if [[ $(check_option strip) = y ]]; then + if [[ $(check_option strip) = "y" ]]; then msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" # make sure library stripping variables are defined to prevent excess stripping [[ -z ${STRIP_SHARED+x} ]] && STRIP_SHARED="-S" @@ -1207,7 +1207,7 @@ write_pkginfo() { for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then - if [[ $ret = y ]]; then + if [[ $ret = "y" ]]; then echo "makepkgopt = $it" else echo "makepkgopt = !$it" @@ -1370,7 +1370,7 @@ create_srcpackage() { local file for file in "${source[@]}"; do - if [[ "$file" == $(get_filename "$file") ]] || (( SOURCEONLY == 2 )); then + if [[ "$file" = "$(get_filename "$file")" ]] || (( SOURCEONLY == 2 )); then local absfile absfile=$(get_filepath "$file") || missing_source_file "$file" msg2 "$(gettext "Adding %s...")" "${absfile##*/}" @@ -1580,7 +1580,7 @@ check_sanity() { known=0 # check if option matches a known option or its inverse for kopt in ${packaging_options[@]} ${other_options[@]}; do - if [[ ${i} = ${kopt} || ${i} = "!${kopt}" ]]; then + if [[ ${i} = "${kopt}" || ${i} = "!${kopt}" ]]; then known=1 fi done @@ -1704,7 +1704,7 @@ devel_check() { # Do not update pkgver if --holdver is set, when building a source package, repackaging, # reading PKGBUILD from pipe (-f), or if we cannot write to the file (-w) if (( HOLDVER || SOURCEONLY || REPKG )) || - [[ ! -f $BUILDFILE || ! -w $BUILDFILE || $BUILDFILE = /dev/stdin ]]; then + [[ ! -f $BUILDFILE || ! -w $BUILDFILE || $BUILDFILE = "/dev/stdin" ]]; then return fi -- 1.7.9.3
On Fri, Mar 09, 2012 at 05:59:04PM +1000, Allan McRae wrote:
Signed-off-by: Allan McRae <allan@archlinux.org> ---
ack.
scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8763ffb..601c7e2 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -311,7 +311,7 @@ in_opt_array() {
local opt for opt in "$@"; do - if [[ $opt = $needle ]]; then + if [[ $opt = "$needle" ]]; then echo 'y' # Enabled return elif [[ $opt = "!$needle" ]]; then @@ -333,7 +333,7 @@ in_array() { local needle=$1; shift local item for item in "$@"; do - [[ $item = $needle ]] && return 0 # Found + [[ $item = "$needle" ]] && return 0 # Found done return 1 # Not Found } @@ -357,7 +357,7 @@ get_downloadclient() { local i for i in "${DLAGENTS[@]}"; do local handler="${i%%::*}" - if [[ $proto = $handler ]]; then + if [[ $proto = "$handler" ]]; then local agent="${i##*::}" break fi @@ -557,7 +557,7 @@ download_sources() { local url=$(get_url "$netfile")
# if we get here, check to make sure it was a URL, else fail - if [[ $file = $url ]]; then + if [[ $file = "$url" ]]; then error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file" exit 1 # $E_MISSING_FILE fi @@ -683,7 +683,7 @@ check_checksums() { local expectedsum=$(tr '[:upper:]' '[:lower:]' <<< "${integrity_sums[$idx]}") local realsum="$(openssl dgst -${integ} "$file")" realsum="${realsum##* }" - if [[ $expectedsum = $realsum ]]; then + if [[ $expectedsum = "$realsum" ]]; then printf -- "$(gettext "Passed")\n" >&2 else printf -- "$(gettext "FAILED")\n" >&2 @@ -842,7 +842,7 @@ extract_sources() {
local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" - if [[ $cmd = bsdtar ]]; then + if [[ $cmd = "bsdtar" ]]; then $cmd -xf "$file" || ret=$? else rm -f "${file%.*}" @@ -981,7 +981,7 @@ tidy_install() { msg2 "$(gettext "Purging unwanted files...")" local pt for pt in "${PURGE_TARGETS[@]}"; do - if [[ ${pt} = ${pt//\/} ]]; then + if [[ ${pt} = "${pt//\/}" ]]; then find . -type f -name "${pt}" -exec rm -f -- '{}' \; else rm -f ${pt} @@ -1025,7 +1025,7 @@ tidy_install() { done fi
- if [[ $(check_option strip) = y ]]; then + if [[ $(check_option strip) = "y" ]]; then msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" # make sure library stripping variables are defined to prevent excess stripping [[ -z ${STRIP_SHARED+x} ]] && STRIP_SHARED="-S" @@ -1207,7 +1207,7 @@ write_pkginfo() { for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then - if [[ $ret = y ]]; then + if [[ $ret = "y" ]]; then echo "makepkgopt = $it" else echo "makepkgopt = !$it" @@ -1370,7 +1370,7 @@ create_srcpackage() {
local file for file in "${source[@]}"; do - if [[ "$file" == $(get_filename "$file") ]] || (( SOURCEONLY == 2 )); then + if [[ "$file" = "$(get_filename "$file")" ]] || (( SOURCEONLY == 2 )); then local absfile absfile=$(get_filepath "$file") || missing_source_file "$file" msg2 "$(gettext "Adding %s...")" "${absfile##*/}" @@ -1580,7 +1580,7 @@ check_sanity() { known=0 # check if option matches a known option or its inverse for kopt in ${packaging_options[@]} ${other_options[@]}; do - if [[ ${i} = ${kopt} || ${i} = "!${kopt}" ]]; then + if [[ ${i} = "${kopt}" || ${i} = "!${kopt}" ]]; then known=1 fi done @@ -1704,7 +1704,7 @@ devel_check() { # Do not update pkgver if --holdver is set, when building a source package, repackaging, # reading PKGBUILD from pipe (-f), or if we cannot write to the file (-w) if (( HOLDVER || SOURCEONLY || REPKG )) || - [[ ! -f $BUILDFILE || ! -w $BUILDFILE || $BUILDFILE = /dev/stdin ]]; then + [[ ! -f $BUILDFILE || ! -w $BUILDFILE || $BUILDFILE = "/dev/stdin" ]]; then return fi
-- 1.7.9.3
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 62 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 601c7e2..384e142 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -211,7 +211,7 @@ get_filepath() { return 1 fi - echo "$file" + printf "%s\n" "$file" } # Print 'source not found' error message and exit makepkg @@ -226,13 +226,13 @@ get_filename() { # if a filename is specified, use it local filename="${1%%::*}" # if it is just an URL, we only keep the last component - echo "${filename##*/}" + printf "%s\n" "${filename##*/}" } # extract the URL from a source entry get_url() { # strip an eventual filename - echo "${1#*::}" + printf "%s\n" "${1#*::}" } ## @@ -242,9 +242,9 @@ get_url() { get_full_version() { if [[ -z $1 ]]; then if [[ $epoch ]] && (( ! $epoch )); then - echo $pkgver-$pkgrel + printf "%s\n" $pkgver-$pkgrel else - echo $epoch:$pkgver-$pkgrel + printf "%s\n" $epoch:$pkgver-$pkgrel fi else for i in pkgver pkgrel epoch; do @@ -253,9 +253,9 @@ get_full_version() { [[ -z ${!indirect} ]] && eval ${indirect}=\"${!i}\" done if (( ! $epoch_override )); then - echo $pkgver_override-$pkgrel_override + printf "%s\n" $pkgver_override-$pkgrel_override else - echo $epoch_override:$pkgver_override-$pkgrel_override + printf "%s\n" $epoch_override:$pkgver_override-$pkgrel_override fi fi } @@ -272,14 +272,14 @@ get_full_version() { check_option() { local ret=$(in_opt_array "$1" ${options[@]}) if [[ $ret != '?' ]]; then - echo $ret + printf "%s\n" "$ret" return fi # fall back to makepkg.conf options ret=$(in_opt_array "$1" ${OPTIONS[@]}) if [[ $ret != '?' ]]; then - echo $ret + printf "%s\n" "$ret" return fi @@ -379,7 +379,7 @@ get_downloadclient() { exit 1 # $E_MISSING_PROGRAM fi - echo "$agent" + printf "%s\n" "$agent" } download_file() { @@ -446,7 +446,7 @@ check_deps() { set -E if (( ret == 127 )); then #unresolved deps - echo "$pmout" + printf "%s\n" "$pmout" elif (( ret )); then error "$(gettext "'%s' returned a fatal error (%i): %s")" "$PACMAN" "$ret" "$pmout" return "$ret" @@ -594,9 +594,9 @@ get_integlist() { done if (( ${#integlist[@]} > 0 )); then - echo ${integlist[@]} + printf "%s\n" ${integlist[@]} else - echo ${INTEGRITY_CHECK[@]} + printf "%s\n" ${INTEGRITY_CHECK[@]} fi } @@ -627,7 +627,7 @@ generate_checksums() { local ct=0 local numsrc=${#source[@]} - echo -n "${integ}sums=(" + printf "%s" "${integ}sums=(" local i local indent='' @@ -641,8 +641,8 @@ generate_checksums() { file="$(get_filepath "$netfile")" || missing_source_file "$netfile" local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - (( ct )) && echo -n "$indent" - echo -n "'$sum'" + (( ct )) && printf "%s" "$indent" + printf "%s" "'$sum'" ct=$(($ct+1)) (( $ct < $numsrc )) && echo done @@ -668,7 +668,7 @@ check_checksums() { for file in "${source[@]}"; do local found=1 file="$(get_filename "$file")" - echo -n " $file ... " >&2 + printf "%s" " $file ... " >&2 if ! file="$(get_filepath "$file")"; then printf -- "$(gettext "NOT FOUND")\n" >&2 @@ -1082,7 +1082,7 @@ find_libdepends() { if in_array "${soname}" ${depends[@]}; then if ! in_array "${soname}=${soversion}-${soarch}" ${libdepends[@]}; then # libfoo.so=1-64 - echo "${soname}=${soversion}-${soarch}" + printf "%s" "${soname}=${soversion}-${soarch}" libdepends=(${libdepends[@]} "${soname}=${soversion}-${soarch}") fi fi @@ -1137,7 +1137,7 @@ find_libprovides() { fi done - echo ${libprovides[@]} + printf "%s" ${libprovides[@]} } check_license() { @@ -1164,15 +1164,15 @@ write_pkginfo() { echo "# using $(fakeroot -v)" fi echo "# $(LC_ALL=C date -u)" - echo "pkgname = $1" + printf "pkgname = %s\n" "$1" (( SPLITPKG )) && echo pkgbase = $pkgbase echo "pkgver = $(get_full_version)" - echo "pkgdesc = $pkgdesc" - echo "url = $url" - echo "builddate = $builddate" - echo "packager = $packager" - echo "size = $size" - echo "arch = $PKGARCH" + printf "pkgdesc = %s\n" "$pkgdesc" + printf "url = %s\n" "$url" + printf "builddate = %s\n" "$builddate" + printf "packager = %s\n" "$packager" + printf "size = %s\n" "$size" + printf "arch = %s\n" "$PKGARCH" [[ $license ]] && printf "license = %s\n" "${license[@]}" [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" @@ -1200,7 +1200,7 @@ write_pkginfo() { return 1 fi else - echo "depend = $it" + printf "depend = %s\n" "$it" fi done @@ -1208,9 +1208,9 @@ write_pkginfo() { local ret="$(check_option $it)" if [[ $ret != "?" ]]; then if [[ $ret = "y" ]]; then - echo "makepkgopt = $it" + printf "makepkgopt = %s\n" "$it" else - echo "makepkgopt = !$it" + printf "makepkgopt = %s\n" "!$it" fi fi done @@ -1851,7 +1851,7 @@ canonicalize_path() { pwd -P ) else - echo "$path" + printf "%s\n" "$path" fi } @@ -1916,7 +1916,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # determine whether we have gettext; make it a no-op if we do not if ! type -p gettext >/dev/null; then gettext() { - echo "$@" + printf "%s\n" "$@" } fi -- 1.7.9.3
On Fri, Mar 09, 2012 at 05:59:05PM +1000, Allan McRae wrote:
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 62 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 601c7e2..384e142 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -211,7 +211,7 @@ get_filepath() { return 1 fi
- echo "$file" + printf "%s\n" "$file" }
# Print 'source not found' error message and exit makepkg @@ -226,13 +226,13 @@ get_filename() { # if a filename is specified, use it local filename="${1%%::*}" # if it is just an URL, we only keep the last component - echo "${filename##*/}" + printf "%s\n" "${filename##*/}" }
# extract the URL from a source entry get_url() { # strip an eventual filename - echo "${1#*::}" + printf "%s\n" "${1#*::}" }
## @@ -242,9 +242,9 @@ get_url() { get_full_version() { if [[ -z $1 ]]; then if [[ $epoch ]] && (( ! $epoch )); then - echo $pkgver-$pkgrel + printf "%s\n" $pkgver-$pkgrel
There's a few instances of these var expansions being unquoted. Can we fix that while we're touching these lines?
else - echo $epoch:$pkgver-$pkgrel + printf "%s\n" $epoch:$pkgver-$pkgrel fi else for i in pkgver pkgrel epoch; do @@ -253,9 +253,9 @@ get_full_version() { [[ -z ${!indirect} ]] && eval ${indirect}=\"${!i}\" done if (( ! $epoch_override )); then - echo $pkgver_override-$pkgrel_override + printf "%s\n" $pkgver_override-$pkgrel_override else - echo $epoch_override:$pkgver_override-$pkgrel_override + printf "%s\n" $epoch_override:$pkgver_override-$pkgrel_override fi fi } @@ -272,14 +272,14 @@ get_full_version() { check_option() { local ret=$(in_opt_array "$1" ${options[@]}) if [[ $ret != '?' ]]; then - echo $ret + printf "%s\n" "$ret" return fi
# fall back to makepkg.conf options ret=$(in_opt_array "$1" ${OPTIONS[@]}) if [[ $ret != '?' ]]; then - echo $ret + printf "%s\n" "$ret" return fi
@@ -379,7 +379,7 @@ get_downloadclient() { exit 1 # $E_MISSING_PROGRAM fi
- echo "$agent" + printf "%s\n" "$agent" }
download_file() { @@ -446,7 +446,7 @@ check_deps() { set -E
if (( ret == 127 )); then #unresolved deps - echo "$pmout" + printf "%s\n" "$pmout" elif (( ret )); then error "$(gettext "'%s' returned a fatal error (%i): %s")" "$PACMAN" "$ret" "$pmout" return "$ret" @@ -594,9 +594,9 @@ get_integlist() { done
if (( ${#integlist[@]} > 0 )); then - echo ${integlist[@]} + printf "%s\n" ${integlist[@]} else - echo ${INTEGRITY_CHECK[@]} + printf "%s\n" ${INTEGRITY_CHECK[@]} fi }
@@ -627,7 +627,7 @@ generate_checksums() {
local ct=0 local numsrc=${#source[@]} - echo -n "${integ}sums=(" + printf "%s" "${integ}sums=("
local i local indent='' @@ -641,8 +641,8 @@ generate_checksums() { file="$(get_filepath "$netfile")" || missing_source_file "$netfile" local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - (( ct )) && echo -n "$indent" - echo -n "'$sum'" + (( ct )) && printf "%s" "$indent" + printf "%s" "'$sum'" ct=$(($ct+1)) (( $ct < $numsrc )) && echo done @@ -668,7 +668,7 @@ check_checksums() { for file in "${source[@]}"; do local found=1 file="$(get_filename "$file")" - echo -n " $file ... " >&2 + printf "%s" " $file ... " >&2
if ! file="$(get_filepath "$file")"; then printf -- "$(gettext "NOT FOUND")\n" >&2 @@ -1082,7 +1082,7 @@ find_libdepends() { if in_array "${soname}" ${depends[@]}; then if ! in_array "${soname}=${soversion}-${soarch}" ${libdepends[@]}; then # libfoo.so=1-64 - echo "${soname}=${soversion}-${soarch}" + printf "%s" "${soname}=${soversion}-${soarch}" libdepends=(${libdepends[@]} "${soname}=${soversion}-${soarch}") fi fi @@ -1137,7 +1137,7 @@ find_libprovides() { fi done
- echo ${libprovides[@]} + printf "%s" ${libprovides[@]} }
check_license() { @@ -1164,15 +1164,15 @@ write_pkginfo() { echo "# using $(fakeroot -v)" fi echo "# $(LC_ALL=C date -u)" - echo "pkgname = $1" + printf "pkgname = %s\n" "$1" (( SPLITPKG )) && echo pkgbase = $pkgbase echo "pkgver = $(get_full_version)" - echo "pkgdesc = $pkgdesc" - echo "url = $url" - echo "builddate = $builddate" - echo "packager = $packager" - echo "size = $size" - echo "arch = $PKGARCH" + printf "pkgdesc = %s\n" "$pkgdesc" + printf "url = %s\n" "$url" + printf "builddate = %s\n" "$builddate" + printf "packager = %s\n" "$packager" + printf "size = %s\n" "$size" + printf "arch = %s\n" "$PKGARCH"
[[ $license ]] && printf "license = %s\n" "${license[@]}" [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" @@ -1200,7 +1200,7 @@ write_pkginfo() { return 1 fi else - echo "depend = $it" + printf "depend = %s\n" "$it" fi done
@@ -1208,9 +1208,9 @@ write_pkginfo() { local ret="$(check_option $it)" if [[ $ret != "?" ]]; then if [[ $ret = "y" ]]; then - echo "makepkgopt = $it" + printf "makepkgopt = %s\n" "$it" else - echo "makepkgopt = !$it" + printf "makepkgopt = %s\n" "!$it" fi fi done @@ -1851,7 +1851,7 @@ canonicalize_path() { pwd -P ) else - echo "$path" + printf "%s\n" "$path" fi }
@@ -1916,7 +1916,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")" # determine whether we have gettext; make it a no-op if we do not if ! type -p gettext >/dev/null; then gettext() { - echo "$@" + printf "%s\n" "$@" } fi
-- 1.7.9.3
On 10/03/12 01:20, Dave Reisner wrote:
On Fri, Mar 09, 2012 at 05:59:05PM +1000, Allan McRae wrote:
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 62 ++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 601c7e2..384e142 100644
<snip>
## @@ -242,9 +242,9 @@ get_url() { get_full_version() { if [[ -z $1 ]]; then if [[ $epoch ]] && (( ! $epoch )); then - echo $pkgver-$pkgrel + printf "%s\n" $pkgver-$pkgrel
There's a few instances of these var expansions being unquoted. Can we fix that while we're touching these lines?
Done, and throughout makepkg. On my working branch. Allan
Most places in makepkg deal with full file paths, but a few use the file name only. Protect from potential issues when a file name starts with a hyphen. Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 384e142..8dd2d39 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -833,7 +833,7 @@ extract_sources() { esac ;; *) # See if bsdtar can recognize the file - if bsdtar -tf "$file" -q '*' &>/dev/null; then + if bsdtar -tf "./$file" -q '*' &>/dev/null; then cmd="bsdtar" else continue @@ -843,10 +843,10 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" if [[ $cmd = "bsdtar" ]]; then - $cmd -xf "$file" || ret=$? + $cmd -xf "./$file" || ret=$? else - rm -f "${file%.*}" - $cmd -dcf "$file" > "${file%.*}" || ret=$? + rm -f -- "${file%.*}" + $cmd -dcf "./$file" > "${file%.*}" || ret=$? fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" @@ -974,7 +974,7 @@ tidy_install() { if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" - rm -rf ${DOC_DIRS[@]} + rm -rf -- ${DOC_DIRS[@]} fi if [[ $(check_option purge) = "y" && -n ${PURGE_TARGETS[*]} ]]; then @@ -1001,7 +1001,7 @@ tidy_install() { find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null | while read link ; do rm -f "$link" "${link}.gz" - ln -s "${file}.gz" "${link}.gz" + ln -s -- "${file}.gz" "${link}.gz" done # check file still exists (potentially already compressed due to hardlink) -- 1.7.9.3
On Fri, Mar 09, 2012 at 05:59:06PM +1000, Allan McRae wrote:
Most places in makepkg deal with full file paths, but a few use the file name only. Protect from potential issues when a file name starts with a hyphen.
How sure are we that these will always be relative paths and never ever absolute?
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 384e142..8dd2d39 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -833,7 +833,7 @@ extract_sources() { esac ;; *) # See if bsdtar can recognize the file - if bsdtar -tf "$file" -q '*' &>/dev/null; then + if bsdtar -tf "./$file" -q '*' &>/dev/null; then
not necessary. "$file" is an argument to the -f flag, so we don't need to work around this: $ bsdtar -czf --foo.tar.gz ~/.bash* $ ls -l -- --foo.tar.gz -rw-r--r-- 1 noclaf users 57856 Mar 9 08:52 --foo.tar.gz
cmd="bsdtar" else continue @@ -843,10 +843,10 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" if [[ $cmd = "bsdtar" ]]; then - $cmd -xf "$file" || ret=$? + $cmd -xf "./$file" || ret=$?
same here.
else - rm -f "${file%.*}" - $cmd -dcf "$file" > "${file%.*}" || ret=$? + rm -f -- "${file%.*}" + $cmd -dcf "./$file" > "${file%.*}" || ret=$?
same here (the rm wants it, though)
fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" @@ -974,7 +974,7 @@ tidy_install() {
if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" - rm -rf ${DOC_DIRS[@]} + rm -rf -- ${DOC_DIRS[@]}
i hate that we can't quote this.
fi
if [[ $(check_option purge) = "y" && -n ${PURGE_TARGETS[*]} ]]; then @@ -1001,7 +1001,7 @@ tidy_install() { find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null | while read link ; do rm -f "$link" "${link}.gz" - ln -s "${file}.gz" "${link}.gz" + ln -s -- "${file}.gz" "${link}.gz"
No love for the rm? I admit it would be an extremely nonstandard case, but the same applies for the ln call.
done
# check file still exists (potentially already compressed due to hardlink) -- 1.7.9.3
On 10/03/12 00:01, Dave Reisner wrote:
On Fri, Mar 09, 2012 at 05:59:06PM +1000, Allan McRae wrote:
Most places in makepkg deal with full file paths, but a few use the file name only. Protect from potential issues when a file name starts with a hyphen.
How sure are we that these will always be relative paths and never ever absolute?
I'm not sure what you are meaning there? Are you asking why I did not fix the ones I determined to use the absolute path? In all other cases the files are either prefixed $srcdir, $pkgdir, $startdir or are from get_filepath which returns a full path.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 384e142..8dd2d39 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -833,7 +833,7 @@ extract_sources() { esac ;; *) # See if bsdtar can recognize the file - if bsdtar -tf "$file" -q '*' &>/dev/null; then + if bsdtar -tf "./$file" -q '*' &>/dev/null; then
not necessary. "$file" is an argument to the -f flag, so we don't need to work around this:
$ bsdtar -czf --foo.tar.gz ~/.bash* $ ls -l -- --foo.tar.gz -rw-r--r-- 1 noclaf users 57856 Mar 9 08:52 --foo.tar.gz
Ah... good point...
cmd="bsdtar" else continue @@ -843,10 +843,10 @@ extract_sources() { local ret=0 msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd" if [[ $cmd = "bsdtar" ]]; then - $cmd -xf "$file" || ret=$? + $cmd -xf "./$file" || ret=$?
same here.
else - rm -f "${file%.*}" - $cmd -dcf "$file" > "${file%.*}" || ret=$? + rm -f -- "${file%.*}" + $cmd -dcf "./$file" > "${file%.*}" || ret=$?
same here (the rm wants it, though)
fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" @@ -974,7 +974,7 @@ tidy_install() {
if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" - rm -rf ${DOC_DIRS[@]} + rm -rf -- ${DOC_DIRS[@]}
i hate that we can't quote this.
fi
if [[ $(check_option purge) = "y" && -n ${PURGE_TARGETS[*]} ]]; then @@ -1001,7 +1001,7 @@ tidy_install() { find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null | while read link ; do rm -f "$link" "${link}.gz" - ln -s "${file}.gz" "${link}.gz" + ln -s -- "${file}.gz" "${link}.gz"
No love for the rm? I admit it would be an extremely nonstandard case, but the same applies for the ln call.
Look at what is being rm'ed and what is being ln'ed. One is a full path.
done
# check file still exists (potentially already compressed due to hardlink) -- 1.7.9.3
On Fri, Mar 9, 2012 at 9:01 AM, Dave Reisner <d@falconindy.com> wrote:
On Fri, Mar 09, 2012 at 05:59:06PM +1000, Allan McRae wrote:
Most places in makepkg deal with full file paths, but a few use the file name only. Protect from potential issues when a file name starts with a hyphen.
How sure are we that these will always be relative paths and never ever absolute?
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 384e142..8dd2d39 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -833,7 +833,7 @@ extract_sources() { esac ;; *) # See if bsdtar can recognize the file - if bsdtar -tf "$file" -q '*' &>/dev/null; then + if bsdtar -tf "./$file" -q '*' &>/dev/null; then
not necessary. "$file" is an argument to the -f flag, so we don't need to work around this:
$ bsdtar -czf --foo.tar.gz ~/.bash* $ ls -l -- --foo.tar.gz -rw-r--r-- 1 noclaf users 57856 Mar 9 08:52 --foo.tar.gz
I would definitely prefer to not have to do this, or at least use the -- option in preference to file path manipulation. However, I don't know that we could get away with that in all cases you fixed here. I do agree with Dave on the -f option though- I've never seen a tar program allow the argument for that to come anywhere except directly after the flag. -Dan
On 13/03/12 01:24, Dan McGee wrote:
On Fri, Mar 9, 2012 at 9:01 AM, Dave Reisner <d@falconindy.com> wrote:
On Fri, Mar 09, 2012 at 05:59:06PM +1000, Allan McRae wrote:
Most places in makepkg deal with full file paths, but a few use the file name only. Protect from potential issues when a file name starts with a hyphen.
How sure are we that these will always be relative paths and never ever absolute?
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 384e142..8dd2d39 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -833,7 +833,7 @@ extract_sources() { esac ;; *) # See if bsdtar can recognize the file - if bsdtar -tf "$file" -q '*' &>/dev/null; then + if bsdtar -tf "./$file" -q '*' &>/dev/null; then
not necessary. "$file" is an argument to the -f flag, so we don't need to work around this:
$ bsdtar -czf --foo.tar.gz ~/.bash* $ ls -l -- --foo.tar.gz -rw-r--r-- 1 noclaf users 57856 Mar 9 08:52 --foo.tar.gz
I would definitely prefer to not have to do this, or at least use the -- option in preference to file path manipulation. However, I don't know that we could get away with that in all cases you fixed here. I do agree with Dave on the -f option though- I've never seen a tar program allow the argument for that to come anywhere except directly after the flag.
Note that the "--" option is not possible there. Or at least I could not get the -q '*' bit working if given earlier in the command. Anyway, the ./ prefixing is killed in my working branch. Allan
Am 09.03.2012 15:01, schrieb Dave Reisner:
@@ -974,7 +974,7 @@ tidy_install() {
if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" - rm -rf ${DOC_DIRS[@]} + rm -rf -- ${DOC_DIRS[@]}
i hate that we can't quote this.
We can't? Why?
On Mon, Mar 12, 2012 at 04:40:38PM +0100, Thomas Bächler wrote:
Am 09.03.2012 15:01, schrieb Dave Reisner:
@@ -974,7 +974,7 @@ tidy_install() {
if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then msg2 "$(gettext "Removing doc files...")" - rm -rf ${DOC_DIRS[@]} + rm -rf -- ${DOC_DIRS[@]}
i hate that we can't quote this.
We can't? Why?
Check out /etc/makepkg.conf. We put globs in DOC_DIRS, and we want them to expand when they're passed to find. It's "luck" that they don't expand. You could definitely break this if you created the right file hierarchy in the build directory, forcing the glob to expand at the time when /etc/makepkg.conf is sourced. d
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8dd2d39..9cd8af8 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2052,8 +2052,11 @@ readonly ALL_OFF BOLD BLUE GREEN RED YELLOW BUILDDIR=${_BUILDDIR:-$BUILDDIR} BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined if [[ ! -d $BUILDDIR ]]; then - mkdir -p "$BUILDDIR" || - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" + if ! mkdir -p "$BUILDDIR"; then + error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" + plain "$(gettext "Aborting...")" + exit 1 + fi chmod a-s "$BUILDDIR" fi if [[ ! -w $BUILDDIR ]]; then -- 1.7.9.3
On Fri, Mar 09, 2012 at 05:59:07PM +1000, Allan McRae wrote:
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8dd2d39..9cd8af8 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2052,8 +2052,11 @@ readonly ALL_OFF BOLD BLUE GREEN RED YELLOW BUILDDIR=${_BUILDDIR:-$BUILDDIR} BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined if [[ ! -d $BUILDDIR ]]; then - mkdir -p "$BUILDDIR" || - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" + if ! mkdir -p "$BUILDDIR"; then + error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
Hrmmm, permissions is only one possible reason for mkdir to fail. the manpage for the syscall and library wrapper list a bunch more. Since mkdir will happily report the error for us (with localization), should we just ditch our own possibly-inaccurate error messaging?
+ plain "$(gettext "Aborting...")" + exit 1 + fi chmod a-s "$BUILDDIR" fi if [[ ! -w $BUILDDIR ]]; then -- 1.7.9.3
On 10/03/12 01:21, Dave Reisner wrote:
On Fri, Mar 09, 2012 at 05:59:07PM +1000, Allan McRae wrote:
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8dd2d39..9cd8af8 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -2052,8 +2052,11 @@ readonly ALL_OFF BOLD BLUE GREEN RED YELLOW BUILDDIR=${_BUILDDIR:-$BUILDDIR} BUILDDIR=${BUILDDIR:-$startdir} #default to $startdir if undefined if [[ ! -d $BUILDDIR ]]; then - mkdir -p "$BUILDDIR" || - error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR" + if ! mkdir -p "$BUILDDIR"; then + error "$(gettext "You do not have write permission to create packages in %s.")" "$BUILDDIR"
Hrmmm, permissions is only one possible reason for mkdir to fail. the manpage for the syscall and library wrapper list a bunch more. Since mkdir will happily report the error for us (with localization), should we just ditch our own possibly-inaccurate error messaging?
I guess mkdir is one of the functions we want a safe wrapper for and we could have a generic "Failed to create directory: %s" type message there. So I will just fix the bug for now under the assumption a bigger fix is coming... Allan
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 9cd8af8..534f6f8 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -170,7 +170,7 @@ clean_up() { for pkg in ${pkgname[@]}; do for file in ${pkg}-*-*-${CARCH}{${PKGEXT},${SRCEXT}}; do if [[ -h $file && ! -e $file ]]; then - rm -f $file + rm -f "$file" fi done done -- 1.7.9.3
In preparation for the removal of the global error trap we need a way to ensure changing directories succeeds. Add a "cd_safe" wrapper that performs the necessary check. Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 534f6f8..f21c638 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -874,6 +874,14 @@ error_function() { exit 2 # $E_BUILD_FAILED } +cd_safe() { + if ! cd "$1"; then + error "$(gettext "Failed to change to directory %s")" "$1" + plain "$(gettext "Aborting...")" + exit 1 + fi +} + run_function() { if [[ -z $1 ]]; then return 1 @@ -891,7 +899,7 @@ run_function() { fi msg "$(gettext "Starting %s()...")" "$pkgfunc" - cd "$srcdir" + cd_safe "$srcdir" # ensure all necessary build variables are exported export CFLAGS CXXFLAGS LDFLAGS MAKEFLAGS CHOST @@ -969,7 +977,7 @@ run_package() { } tidy_install() { - cd "$pkgdir" + cd_safe "$pkgdir" msg "$(gettext "Tidying install...")" if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then @@ -1219,7 +1227,7 @@ write_pkginfo() { } check_package() { - cd "$pkgdir" + cd_safe "$pkgdir" # check existence of backup files local file @@ -1248,7 +1256,7 @@ create_package() { check_package - cd "$pkgdir" + cd_safe "$pkgdir" msg "$(gettext "Creating package...")" local nameofpkg @@ -1407,7 +1415,7 @@ create_srcpackage() { # tar it up msg2 "$(gettext "Compressing source package...")" - cd "${srclinks}" + cd_safe "${srclinks}" if ! bsdtar -c${TAR_OPT}Lf "$pkg_file" ${pkgbase}; then error "$(gettext "Failed to create source package file.")" exit 1 # TODO: error code @@ -1423,7 +1431,7 @@ create_srcpackage() { warning "$(gettext "Failed to create symlink to source package file.")" fi - cd "${startdir}" + cd_safe "${startdir}" rm -rf "${srclinks}" } @@ -1757,7 +1765,7 @@ devel_check() { fi msg "$(gettext "Determining latest %s revision...")" 'hg' if [[ -d ./src/$_hgrepo ]] ; then - cd ./src/$_hgrepo + cd_safe ./src/$_hgrepo local ret=0 hg pull || ret=$? if (( ! ret )); then @@ -1768,10 +1776,10 @@ devel_check() { else [[ ! -d ./src/ ]] && mkdir ./src/ hg clone $_hgroot/$_hgrepo ./src/$_hgrepo - cd ./src/$_hgrepo + cd_safe ./src/$_hgrepo fi newpkgver=$(hg tip --template "{rev}") - cd ../../ + cd_safe ../../ fi if [[ -n $newpkgver ]]; then @@ -1847,7 +1855,7 @@ canonicalize_path() { if [[ -d $path ]]; then ( - cd "$path" + cd_safe "$path" pwd -P ) else @@ -2171,7 +2179,7 @@ fi if (( GENINTEG )); then mkdir -p "$srcdir" chmod a-s "$srcdir" - cd "$srcdir" + cd_safe "$srcdir" download_sources generate_checksums exit 0 # $E_OK @@ -2321,14 +2329,14 @@ if (( SOURCEONLY )); then # Get back to our src directory so we can begin with sources. mkdir -p "$srcdir" chmod a-s "$srcdir" - cd "$srcdir" + cd_safe "$srcdir" if ( (( ! SKIPCHECKSUMS )) || \ ( (( ! SKIPPGPCHECK )) && source_has_signatures ) ) || \ (( SOURCEONLY == 2 )); then download_sources fi check_source_integrity - cd "$startdir" + cd_safe "$startdir" # if we are root or if fakeroot is not enabled, then we don't use it if [[ $(check_buildenv fakeroot) != "y" ]] || (( EUID == 0 )); then @@ -2384,7 +2392,7 @@ umask 0022 # get back to our src directory so we can begin with sources mkdir -p "$srcdir" chmod a-s "$srcdir" -cd "$srcdir" +cd_safe "$srcdir" if (( NOEXTRACT )); then warning "$(gettext "Skipping source retrieval -- using existing %s tree")" "src/" @@ -2420,7 +2428,7 @@ else fi mkdir -p "$pkgdir" chmod a-s "$pkgdir" - cd "$startdir" + cd_safe "$startdir" # if we are root or if fakeroot is not enabled, then we don't use it if [[ $(check_buildenv fakeroot) != "y" ]] || (( EUID == 0 )); then @@ -2450,7 +2458,7 @@ else devel_update (( BUILDFUNC )) && run_build (( CHECKFUNC )) && run_check - cd "$startdir" + cd_safe "$startdir" fi enter_fakeroot -- 1.7.9.3
On Fri, Mar 09, 2012 at 05:59:09PM +1000, Allan McRae wrote:
In preparation for the removal of the global error trap we need a way to ensure changing directories succeeds. Add a "cd_safe" wrapper that performs the necessary check.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 534f6f8..f21c638 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -874,6 +874,14 @@ error_function() { exit 2 # $E_BUILD_FAILED }
+cd_safe() { + if ! cd "$1"; then
Do we want to write stdout to /dev/null here? It _should_ be quiet since we unset CDPATH early on...
+ error "$(gettext "Failed to change to directory %s")" "$1" + plain "$(gettext "Aborting...")" + exit 1 + fi +} + run_function() { if [[ -z $1 ]]; then return 1 @@ -891,7 +899,7 @@ run_function() { fi
msg "$(gettext "Starting %s()...")" "$pkgfunc" - cd "$srcdir" + cd_safe "$srcdir"
# ensure all necessary build variables are exported export CFLAGS CXXFLAGS LDFLAGS MAKEFLAGS CHOST @@ -969,7 +977,7 @@ run_package() { }
tidy_install() { - cd "$pkgdir" + cd_safe "$pkgdir" msg "$(gettext "Tidying install...")"
if [[ $(check_option docs) = "n" && -n ${DOC_DIRS[*]} ]]; then @@ -1219,7 +1227,7 @@ write_pkginfo() { }
check_package() { - cd "$pkgdir" + cd_safe "$pkgdir"
# check existence of backup files local file @@ -1248,7 +1256,7 @@ create_package() {
check_package
- cd "$pkgdir" + cd_safe "$pkgdir" msg "$(gettext "Creating package...")"
local nameofpkg @@ -1407,7 +1415,7 @@ create_srcpackage() {
# tar it up msg2 "$(gettext "Compressing source package...")" - cd "${srclinks}" + cd_safe "${srclinks}" if ! bsdtar -c${TAR_OPT}Lf "$pkg_file" ${pkgbase}; then error "$(gettext "Failed to create source package file.")" exit 1 # TODO: error code @@ -1423,7 +1431,7 @@ create_srcpackage() { warning "$(gettext "Failed to create symlink to source package file.")" fi
- cd "${startdir}" + cd_safe "${startdir}" rm -rf "${srclinks}" }
@@ -1757,7 +1765,7 @@ devel_check() { fi msg "$(gettext "Determining latest %s revision...")" 'hg' if [[ -d ./src/$_hgrepo ]] ; then - cd ./src/$_hgrepo + cd_safe ./src/$_hgrepo local ret=0 hg pull || ret=$? if (( ! ret )); then @@ -1768,10 +1776,10 @@ devel_check() { else [[ ! -d ./src/ ]] && mkdir ./src/ hg clone $_hgroot/$_hgrepo ./src/$_hgrepo - cd ./src/$_hgrepo + cd_safe ./src/$_hgrepo fi newpkgver=$(hg tip --template "{rev}") - cd ../../ + cd_safe ../../ fi
if [[ -n $newpkgver ]]; then @@ -1847,7 +1855,7 @@ canonicalize_path() {
if [[ -d $path ]]; then ( - cd "$path" + cd_safe "$path" pwd -P ) else @@ -2171,7 +2179,7 @@ fi if (( GENINTEG )); then mkdir -p "$srcdir" chmod a-s "$srcdir" - cd "$srcdir" + cd_safe "$srcdir" download_sources generate_checksums exit 0 # $E_OK @@ -2321,14 +2329,14 @@ if (( SOURCEONLY )); then # Get back to our src directory so we can begin with sources. mkdir -p "$srcdir" chmod a-s "$srcdir" - cd "$srcdir" + cd_safe "$srcdir" if ( (( ! SKIPCHECKSUMS )) || \ ( (( ! SKIPPGPCHECK )) && source_has_signatures ) ) || \ (( SOURCEONLY == 2 )); then download_sources fi check_source_integrity - cd "$startdir" + cd_safe "$startdir"
# if we are root or if fakeroot is not enabled, then we don't use it if [[ $(check_buildenv fakeroot) != "y" ]] || (( EUID == 0 )); then @@ -2384,7 +2392,7 @@ umask 0022 # get back to our src directory so we can begin with sources mkdir -p "$srcdir" chmod a-s "$srcdir" -cd "$srcdir" +cd_safe "$srcdir"
if (( NOEXTRACT )); then warning "$(gettext "Skipping source retrieval -- using existing %s tree")" "src/" @@ -2420,7 +2428,7 @@ else fi mkdir -p "$pkgdir" chmod a-s "$pkgdir" - cd "$startdir" + cd_safe "$startdir"
# if we are root or if fakeroot is not enabled, then we don't use it if [[ $(check_buildenv fakeroot) != "y" ]] || (( EUID == 0 )); then @@ -2450,7 +2458,7 @@ else devel_update (( BUILDFUNC )) && run_build (( CHECKFUNC )) && run_check - cd "$startdir" + cd_safe "$startdir" fi
enter_fakeroot -- 1.7.9.3
On 10/03/12 01:23, Dave Reisner wrote:
On Fri, Mar 09, 2012 at 05:59:09PM +1000, Allan McRae wrote:
In preparation for the removal of the global error trap we need a way to ensure changing directories succeeds. Add a "cd_safe" wrapper that performs the necessary check.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/makepkg.sh.in | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 534f6f8..f21c638 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -874,6 +874,14 @@ error_function() { exit 2 # $E_BUILD_FAILED }
+cd_safe() { + if ! cd "$1"; then
Do we want to write stdout to /dev/null here? It _should_ be quiet since we unset CDPATH early on...
I am happy not doing that at the moment, mainly because it was never there before and I would like to know if something breaks our assumption that it should be quiet...
participants (4)
-
Allan McRae
-
Dan McGee
-
Dave Reisner
-
Thomas Bächler