[pacman-dev] [PATCH 1/3] makepkg: restrict usage of errexit to user functions
It's expected that this will lead to unwanted behavior, and needs widespread testing. It's desirable to commit this for a few reasons: - there's no reason we can't do our own error checking for code that we write. - it avoids the need for ||true hacks scattered about in the code. - it makes us immune to upstream changes in exit codes (FS#28248) Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Allan, just making sure we're on the same page -- this _will_ cause breakage, and the next patch in this series addresses one specific case. I figure getting this patch in now gives us "ample time" to uncover and fix all these before the next release. scripts/makepkg.sh.in | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f0a2252..61c95af 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1,4 +1,4 @@ -#!/bin/bash -e +#!/bin/bash # # makepkg - make packages compatible for use with pacman # @configure_input@ @@ -430,13 +430,10 @@ run_pacman() { check_deps() { (( $# > 0 )) || return 0 - # Disable error trap in pacman subshell call as this breaks bash-3.2 compatibility - # Also, a non-zero return value is not unexpected and we are manually dealing them - set +E local ret=0 local pmout - pmout=$(run_pacman -T "$@") || ret=$? - set -E + pmout=$(run_pacman -T "$@") + ret=$? if (( ret == 127 )); then #unresolved deps echo "$pmout" @@ -469,13 +466,7 @@ handle_deps() { fi # we might need the new system environment - # avoid triggering the ERR trap and exiting - set +e - local restoretrap=$(trap -p ERR) - trap - ERR source /etc/profile &>/dev/null - eval $restoretrap - set -e return $R_DEPS_SATISFIED } @@ -867,6 +858,23 @@ error_function() { exit 2 # $E_BUILD_FAILED } +run_function_safe() { + local restoretrap + + set -e + set -E + + restoretrap=$(trap -p ERR) + trap 'error_function $pkgfunc' ERR + + run_function "$1" + + eval $restoretrap + + set +E + set +e +} + run_function() { if [[ -z $1 ]]; then return 1 @@ -892,7 +900,6 @@ run_function() { local shellopts=$(shopt -p) local ret=0 - local restoretrap if (( LOGGING )); then local fullver=$(get_full_version) local BUILDLOG="${startdir}/${pkgbase}-${fullver}-${CARCH}-$pkgfunc.log" @@ -914,18 +921,12 @@ run_function() { tee "$BUILDLOG" < "$logpipe" & local teepid=$! - restoretrap=$(trap -p ERR) - trap 'error_function $pkgfunc' ERR $pkgfunc &>"$logpipe" - eval $restoretrap wait $teepid rm "$logpipe" else - restoretrap=$(trap -p ERR) - trap 'error_function $pkgfunc' ERR $pkgfunc 2>&1 - eval $restoretrap fi # reset our shell options eval "$shellopts" @@ -943,11 +944,11 @@ run_build() { [[ -d /usr/lib/ccache/bin ]] && export PATH="/usr/lib/ccache/bin:$PATH" fi - run_function "build" + run_function_safe "build" } run_check() { - run_function "check" + run_function_safe "check" } run_package() { @@ -958,7 +959,7 @@ run_package() { pkgfunc="package_$1" fi - run_function "$pkgfunc" + run_function_safe "$pkgfunc" } tidy_install() { @@ -1973,8 +1974,6 @@ for signal in TERM HUP QUIT; do trap "trap_exit \"$(gettext "%s signal caught. Exiting...")\" \"$signal\"" "$signal" done trap 'trap_exit "$(gettext "Aborted by user! Exiting...")"' INT -trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR -set -E # preserve environment variables and canonicalize path [[ -n ${PKGDEST} ]] && _PKGDEST=$(canonicalize_path ${PKGDEST}) -- 1.7.9
create source_safe() function which temporarily disables extglob and exits on error. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- scripts/makepkg.sh.in | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 61c95af..c7bde9c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -858,6 +858,15 @@ error_function() { exit 2 # $E_BUILD_FAILED } +source_safe() { + shopt -u extglob + if ! source "$@"; then + error "$(gettext "Failed to source %s")" "$1" + exit 1 + fi + shopt -s extglob +} + run_function_safe() { local restoretrap @@ -1781,7 +1790,7 @@ devel_update() { if [[ -f $BUILDFILE && -w $BUILDFILE ]]; then @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" - source "$BUILDFILE" + source_safe "$BUILDFILE" fi fi fi @@ -1990,7 +1999,7 @@ MAKEPKG_CONF=${MAKEPKG_CONF:-$confdir/makepkg.conf} # Source the config file; fail if it is not found if [[ -r $MAKEPKG_CONF ]]; then - source "$MAKEPKG_CONF" + source_safe "$MAKEPKG_CONF" else error "$(gettext "%s not found.")" "$MAKEPKG_CONF" plain "$(gettext "Aborting...")" @@ -2000,7 +2009,7 @@ fi # Source user-specific makepkg.conf overrides, but only if no override config # file was specified if [[ $MAKEPKG_CONF = "$confdir/makepkg.conf" && -r ~/.makepkg.conf ]]; then - source ~/.makepkg.conf + source_safe ~/.makepkg.conf fi # set pacman command if not already defined @@ -2116,9 +2125,7 @@ if [[ ! -f $BUILDFILE ]]; then else # PKGBUILD passed through a pipe BUILDFILE=/dev/stdin - shopt -u extglob - source "$BUILDFILE" - shopt -s extglob + source_safe "$BUILDFILE" fi else crlftest=$(file "$BUILDFILE" | grep -F 'CRLF' || true) @@ -2130,9 +2137,7 @@ else if [[ ${BUILDFILE:0:1} != "/" ]]; then BUILDFILE="$startdir/$BUILDFILE" fi - shopt -u extglob - source "$BUILDFILE" - shopt -s extglob + source_safe "$BUILDFILE" fi # set defaults if they weren't specified in buildfile -- 1.7.9
If the PKGBUILD isn't writeable for devel_update, throw a warning instead of silently ignoring it. Some logical reordering is present in this patch to reduce the number of nested if's. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- scripts/makepkg.sh.in | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index c7bde9c..5cdf2f5 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1785,13 +1785,13 @@ devel_update() { # ... # _foo=pkgver # - if [[ -n $newpkgver ]]; then - if [[ $newpkgver != "$pkgver" ]]; then - if [[ -f $BUILDFILE && -w $BUILDFILE ]]; then - @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" - @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" - source_safe "$BUILDFILE" - fi + if [[ -n $newpkgver && $newpkgver != "$pkgver" && -f $BUILDFILE ]]; then + if [[ -w $BUILDFILE ]]; then + @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" + @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" + source "$BUILDFILE" + else + warning "$(gettext "%s is not writeable -- pkgver will not be updated")" "$BUILDFILE" fi fi } -- 1.7.9
On 15/02/12 04:58, Dave Reisner wrote:
If the PKGBUILD isn't writeable for devel_update, throw a warning instead of silently ignoring it. Some logical reordering is present in this patch to reduce the number of nested if's.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- scripts/makepkg.sh.in | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index c7bde9c..5cdf2f5 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1785,13 +1785,13 @@ devel_update() { # ... # _foo=pkgver # - if [[ -n $newpkgver ]]; then - if [[ $newpkgver != "$pkgver" ]]; then - if [[ -f $BUILDFILE && -w $BUILDFILE ]]; then - @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" - @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" - source_safe "$BUILDFILE" - fi + if [[ -n $newpkgver && $newpkgver != "$pkgver" && -f $BUILDFILE ]]; then + if [[ -w $BUILDFILE ]]; then
Do not change this... -f $BUILDFILE is needed as PKGBUILDs can be red from /dev/stdin...
+ @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" + @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" + source "$BUILDFILE" + else + warning "$(gettext "%s is not writeable -- pkgver will not be updated")" "$BUILDFILE" fi fi }
On Wed, Feb 15, 2012 at 6:09 AM, Allan McRae <allan@archlinux.org> wrote:
On 15/02/12 04:58, Dave Reisner wrote:
If the PKGBUILD isn't writeable for devel_update, throw a warning instead of silently ignoring it. Some logical reordering is present in this patch to reduce the number of nested if's.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- scripts/makepkg.sh.in | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index c7bde9c..5cdf2f5 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1785,13 +1785,13 @@ devel_update() { # ... # _foo=pkgver # - if [[ -n $newpkgver ]]; then - if [[ $newpkgver != "$pkgver" ]]; then - if [[ -f $BUILDFILE && -w $BUILDFILE ]]; then - @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" - @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" - source_safe "$BUILDFILE" - fi + if [[ -n $newpkgver && $newpkgver != "$pkgver" && -f $BUILDFILE ]]; then + if [[ -w $BUILDFILE ]]; then
Do not change this... -f $BUILDFILE is needed as PKGBUILDs can be red from /dev/stdin...
Yes, good catch. We should warn in that case as well.
+ @SEDINPLACE@ "s/^pkgver=[^ ]*/pkgver=$newpkgver/" "$BUILDFILE" + @SEDINPLACE@ "s/^pkgrel=[^ ]*/pkgrel=1/" "$BUILDFILE" + source "$BUILDFILE" + else + warning "$(gettext "%s is not writeable -- pkgver will not be updated")" "$BUILDFILE" fi fi }
On 15/02/12 04:57, Dave Reisner wrote:
It's expected that this will lead to unwanted behavior, and needs widespread testing. It's desirable to commit this for a few reasons:
- there's no reason we can't do our own error checking for code that we write. - it avoids the need for ||true hacks scattered about in the code. - it makes us immune to upstream changes in exit codes (FS#28248)
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Allan, just making sure we're on the same page -- this _will_ cause breakage, and the next patch in this series addresses one specific case. I figure getting this patch in now gives us "ample time" to uncover and fix all these before the next release.
OK... I like this idea somewhat as I have seen the issues this can cause. But I have also seen it validly stop the scripts execution due to errors that would have been non-obvious. Here goes a few concerns you might help me alleviate: 1) does it really make us immune to upstream changes in exit codes? In the particular example of mercurial, would we not be checking the exit code of the "hg pull" part anyway? 2) how much extra error checking are we going to need to do? e.g. if I look at create_srcpackage() would we only need to check the mkdir and ln lines? Allan
On Wed, Feb 15, 2012 at 7:05 AM, Allan McRae <allan@archlinux.org> wrote:
On 15/02/12 04:57, Dave Reisner wrote:
It's expected that this will lead to unwanted behavior, and needs widespread testing. It's desirable to commit this for a few reasons:
- there's no reason we can't do our own error checking for code that we write. - it avoids the need for ||true hacks scattered about in the code. - it makes us immune to upstream changes in exit codes (FS#28248)
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Allan, just making sure we're on the same page -- this _will_ cause breakage, and the next patch in this series addresses one specific case. I figure getting this patch in now gives us "ample time" to uncover and fix all these before the next release.
OK... I like this idea somewhat as I have seen the issues this can cause. But I have also seen it validly stop the scripts execution due to errors that would have been non-obvious.
Here goes a few concerns you might help me alleviate:
1) does it really make us immune to upstream changes in exit codes? In the particular example of mercurial, would we not be checking the exit code of the "hg pull" part anyway?
Yeah, this is probably accurate. I feel like there's always going to be commands which can exit non-zero for which we still expect that they did what we told them to, I just picked a bad example.
2) how much extra error checking are we going to need to do? e.g. if I look at create_srcpackage() would we only need to check the mkdir and ln lines?
So, the big stuff is: file/directory creation, directory changes, and executing arbitrary code (e.g. sourcing files). There's no reason we couldn't create a "lighter weight" ERR trap with errtrace if we expect to go through an entire function that needs to succeed, rather than checking everything. create_srcpackage() looks like a good candidate for this sort of thing. We can do something like this: somefunc() { true; false; true; echo 'shouldnt be here' } set -E trap -- 'return 1 2>/dev/null' ERR somefunc set +E trap -- ERR echo 'success' The stderr redirect is due to what I suspect is a bug in bash (exists in bash3 as well) -- it errors out saying you can only return from a function, but it gives us the behavior we want (evidenced by the debug echo's). I'm going to followup with bug-bash@gnu to see if this is really the case, though. d
On 16/02/12 02:26, Dave Reisner wrote:
On Wed, Feb 15, 2012 at 7:05 AM, Allan McRae <allan@archlinux.org> wrote:
On 15/02/12 04:57, Dave Reisner wrote:
It's expected that this will lead to unwanted behavior, and needs widespread testing. It's desirable to commit this for a few reasons:
- there's no reason we can't do our own error checking for code that we write. - it avoids the need for ||true hacks scattered about in the code. - it makes us immune to upstream changes in exit codes (FS#28248)
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- Allan, just making sure we're on the same page -- this _will_ cause breakage, and the next patch in this series addresses one specific case. I figure getting this patch in now gives us "ample time" to uncover and fix all these before the next release.
OK... I like this idea somewhat as I have seen the issues this can cause. But I have also seen it validly stop the scripts execution due to errors that would have been non-obvious.
Here goes a few concerns you might help me alleviate:
1) does it really make us immune to upstream changes in exit codes? In the particular example of mercurial, would we not be checking the exit code of the "hg pull" part anyway?
Yeah, this is probably accurate. I feel like there's always going to be commands which can exit non-zero for which we still expect that they did what we told them to, I just picked a bad example.
2) how much extra error checking are we going to need to do? e.g. if I look at create_srcpackage() would we only need to check the mkdir and ln lines?
So, the big stuff is: file/directory creation, directory changes, and executing arbitrary code (e.g. sourcing files). There's no reason we couldn't create a "lighter weight" ERR trap with errtrace if we expect to go through an entire function that needs to succeed, rather than checking everything. create_srcpackage() looks like a good candidate for this sort of thing. We can do something like this:
somefunc() { true; false; true; echo 'shouldnt be here' } set -E trap -- 'return 1 2>/dev/null' ERR somefunc set +E trap -- ERR echo 'success'
The stderr redirect is due to what I suspect is a bug in bash (exists in bash3 as well) -- it errors out saying you can only return from a function, but it gives us the behavior we want (evidenced by the debug echo's). I'm going to followup with bug-bash@gnu to see if this is really the case, though.
You are not going to convince anyone with that code! :D Would it be useful to have wrapper functions around cd, ln, mkdir that would take most of the additional testing burden away? Anyway, I am happy for this change to go ahead. But I would hold off committing it master until a few more of the obvious checks that will now be needed have been added. Allan
participants (2)
-
Allan McRae
-
Dave Reisner