trap bug in recent versions of dash

trap bug in recent versions of dash

[Guido Berhoerster]

Hello,

with the latest git version of dash trap actions are not
evaluated in the context of a function.

The following script demonstrates the bug:
----8<----
read_timeout () {
    saved_traps="$(trap)"
    trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
    ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &
    timer_pid=$!
    read $2
    kill $timer_pid 2>/dev/null
}

read_timeout 5 value
printf "read \"%s\"\n" "${value:=default}"

---->8----
The return statement in the trap inside the read_timeout function
does not return from the function but rather exits the script.

With dash 0.5.5.1 it works as expected.
-- 
Guido Berhoerster

Re: trap bug in recent versions of dash

[Jilles Tjoelker]

On Wed, Aug 11, 2010 at 10:06:16AM +0200, Guido Berhoerster wrote:
> with the latest git version of dash trap actions are not
> evaluated in the context of a function.

I think dotrap()'s return value should be removed or at least ignored.
An "evalskip" (break/continue/return-function/return-file) should never
lead to an immediate exit. I'm not supplying a patch because I am not
entirely sure about the side effects this could have.

Related, there may be a bug with SKIPFILE. This constant is never tested
against, and this makes 'break' inside a dot script behave strangely.
Example (this must be saved to a file, as it sources itself):

if [ "$1" != nested ]; then
        while :; do
                set -- nested
                . "$0"
                echo bad2
                exit 2
        done
        exit 0
fi
break
echo bad1
exit 1

> The following script demonstrates the bug:
> ----8<----
> read_timeout () {
>     saved_traps="$(trap)"

This does not work in dash, it always returns an empty string because
trap is evaluated in a subshell.

Some other ash variants do not evaluate most command substitutions
containing only a single builtin in a subshell, but this was removed in
NetBSD sh and dash because such commands can affect the shell in wrong
ways (e.g. $(exit 1) causes FreeBSD sh to exit).

A recent or proposed POSIX interpretation has said that $(trap) should
work, and that this may be done by treating a lone trap command
specially or by having trap in a subshell output the parent's traps
until a trap has been set in the subshell. To help the former case,
stuff like TRAP=trap; y=$($TRAP) is not required to work.

A problem in the script is that it does not handle TERM set to the
default action properly, as this is not included in trap's output.

>     trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
>     ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &

For portability, I recommend braces
    { sleep $1; kill -TERM $$; } >/dev/null 2>&1 &

Some shells do not treat a background subshell specially and fork twice,
which would cause the wrong process to be killed below.

>     timer_pid=$!
>     read $2
>     kill $timer_pid 2>/dev/null

eval "${saved_traps}"  is missing here.

> }

> read_timeout 5 value
> printf "read \"%s\"\n" "${value:=default}"

> ---->8----
> The return statement in the trap inside the read_timeout function
> does not return from the function but rather exits the script.

> With dash 0.5.5.1 it works as expected.

-- 
Jilles Tjoelker

Re: trap bug in recent versions of dash

[Guido Berhoerster]

* Jilles Tjoelker <jilles@stack.nl> [2010-08-15 22:05]:
> On Wed, Aug 11, 2010 at 10:06:16AM +0200, Guido Berhoerster wrote:
> > with the latest git version of dash trap actions are not
> > evaluated in the context of a function.
> 
> I think dotrap()'s return value should be removed or at least ignored.
> An "evalskip" (break/continue/return-function/return-file) should never
> lead to an immediate exit. I'm not supplying a patch because I am not
> entirely sure about the side effects this could have.

OK, I'm not familiar with the source but it'd be nice to have it
fixed before the next release since it is a regression breaking
scripts.

> > The following script demonstrates the bug:
> > ----8<----
> > read_timeout () {
> >     saved_traps="$(trap)"
> 
> This does not work in dash, it always returns an empty string because
> trap is evaluated in a subshell.
> 
> Some other ash variants do not evaluate most command substitutions
> containing only a single builtin in a subshell, but this was removed in
> NetBSD sh and dash because such commands can affect the shell in wrong
> ways (e.g. $(exit 1) causes FreeBSD sh to exit).

POSIX:2008 lists

save_traps=$(trap)
...
eval "$save_traps"

as an example in the specification of the trap special builtin,
although it somewhat contradicts the description. This goes back
to at lease the SUSv2 in 1997 and it currently works e.g. in bash
and ksh93.

> A recent or proposed POSIX interpretation has said that $(trap) should
> work, and that this may be done by treating a lone trap command
> specially or by having trap in a subshell output the parent's traps
> until a trap has been set in the subshell. To help the former case,
> stuff like TRAP=trap; y=$($TRAP) is not required to work.
> 
> A problem in the script is that it does not handle TERM set to the
> default action properly, as this is not included in trap's output.

Right, the following interpretation has been approved last year (see
http://austingroupbugs.net/view.php?id=53):
"When a subshell is entered, traps that are not being ignored shall
be set to the default actions, except in the case of a command
substitution containing only a single trap command, when the traps
need not be altered. Implementations may check for this case
using only lexical analysis; for example if `trap` and $( trap -- )
do not alter the traps in the subshell, cases such as assigning
var=trap and then using $($var) may still alter them."
and
"The trap command with no operands shall write to standard output
a list of commands associated with each condition. If the command
is executed in a subshell, the implementation does not perform
the optional check described above for a command substitution
containing only a single trap command, and no trap commands with
operands have been executed since entry to the subshell, the list
shall contain the commands that were associated with each
condition immediately before the subshell environment was entered.
Otherwise, the list shall contain the commands currently
associated with each condition."

So this IMHO this should be implemented as a special case in dash
as well.

> >     trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
> >     ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &
> 
> For portability, I recommend braces
>     { sleep $1; kill -TERM $$; } >/dev/null 2>&1 &
> 
> Some shells do not treat a background subshell specially and fork twice,
> which would cause the wrong process to be killed below.

Thanks for the hint.

> >     timer_pid=$!
> >     read $2
> >     kill $timer_pid 2>/dev/null
> 
> eval "${saved_traps}"  is missing here.

Yep, forgot that.

-- 
Guido Berhoerster

Re: trap bug in recent versions of dash

[Jilles Tjoelker]

On Mon, Aug 16, 2010 at 01:52:56PM +0200, Guido Berhoerster wrote:
> * Jilles Tjoelker <jilles@stack.nl> [2010-08-15 22:05]:
> > On Wed, Aug 11, 2010 at 10:06:16AM +0200, Guido Berhoerster wrote:
> > > with the latest git version of dash trap actions are not
> > > evaluated in the context of a function.

> > I think dotrap()'s return value should be removed or at least ignored.
> > An "evalskip" (break/continue/return-function/return-file) should never
> > lead to an immediate exit. I'm not supplying a patch because I am not
> > entirely sure about the side effects this could have.

> OK, I'm not familiar with the source but it'd be nice to have it
> fixed before the next release since it is a regression breaking
> scripts.

If you want to try something, here is a patch. I have verified that the
only change to the results of FreeBSD sh's testsuite is that the test
builtins/break2.0 starts working (there are still 51 other broken
tests). There is no change in output from the posh testsuite (run with
  -C sh,posix,no-typeset,no-arrays,no-coprocs,no-herestrings,no-history
).

diff --git a/src/eval.c b/src/eval.c
index d5e5c95..e484bec 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -307,9 +307,9 @@ setstatus:
 		break;
 	}
 out:
-	if ((checkexit & exitstatus) ||
-	    (pendingsigs && dotrap()) ||
-	    (flags & EV_EXIT))
+	if (pendingsigs)
+		dotrap();
+	if ((flags & EV_EXIT) || (checkexit & exitstatus))
 		exraise(EXEXIT);
 }

-- 
Jilles Tjoelker

Re: trap bug in recent versions of dash

[Guido Berhoerster]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

* Jilles Tjoelker <jilles@stack.nl> [2010-08-23 00:32]:
> If you want to try something, here is a patch. I have verified that the
> only change to the results of FreeBSD sh's testsuite is that the test
> builtins/break2.0 starts working (there are still 51 other broken
> tests). There is no change in output from the posh testsuite (run with
>   -C sh,posix,no-typeset,no-arrays,no-coprocs,no-herestrings,no-history
> ).
> 
> diff --git a/src/eval.c b/src/eval.c
> index d5e5c95..e484bec 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -307,9 +307,9 @@ setstatus:
>  		break;
>  	}
>  out:
> -	if ((checkexit & exitstatus) ||
> -	    (pendingsigs && dotrap()) ||
> -	    (flags & EV_EXIT))
> +	if (pendingsigs)
> +		dotrap();
> +	if ((flags & EV_EXIT) || (checkexit & exitstatus))
>  		exraise(EXEXIT);
>  }

Unfortunately this seems to corrupt variables.
See the attached test script, after the TERM signal $value
is not empty anymore but contains garbage.

Where can I find FreeBSD's sh tests?
-- 
Guido Berhoerster

[-- Attachment #2: timed-read.sh --]
[-- Type: application/x-shellscript, Size: 353 bytes --]

Re: trap bug in recent versions of dash

[Jilles Tjoelker]

On Mon, Aug 23, 2010 at 12:40:48PM +0200, Guido Berhoerster wrote:
> * Jilles Tjoelker <jilles@stack.nl> [2010-08-23 00:32]:
> > If you want to try something, here is a patch. I have verified that the
> > only change to the results of FreeBSD sh's testsuite is that the test
> > builtins/break2.0 starts working (there are still 51 other broken
> > tests). There is no change in output from the posh testsuite (run with
> >   -C sh,posix,no-typeset,no-arrays,no-coprocs,no-herestrings,no-history
> > ).

> > diff --git a/src/eval.c b/src/eval.c
> > index d5e5c95..e484bec 100644
> > --- a/src/eval.c
> > +++ b/src/eval.c
> > @@ -307,9 +307,9 @@ setstatus:
> >  		break;
> >  	}
> >  out:
> > -	if ((checkexit & exitstatus) ||
> > -	    (pendingsigs && dotrap()) ||
> > -	    (flags & EV_EXIT))
> > +	if (pendingsigs)
> > +		dotrap();
> > +	if ((flags & EV_EXIT) || (checkexit & exitstatus))
> >  		exraise(EXEXIT);
> >  }

> Unfortunately this seems to corrupt variables.
> See the attached test script, after the TERM signal $value
> is not empty anymore but contains garbage.

I think this is the same strange bug as Harald van Dijk reported. The
below gives incorrect output, and quoting the command substitution works
around the issue:

dash -c 'printf "a\t\tb\n" | { IFS=$(printf "\t") read a b c; echo "|$a|$b|$c|"; }'

In your script, if I write  read "$2"  instead of  read $2  it works.

> Where can I find FreeBSD's sh tests?

You need the tools/regression/bin/sh directory from the head (cvs: HEAD)
branch of the FreeBSD src repository. See
http://www.freebsd.org/developers/cvs.html for access methods. There are
also unofficial git, hg and other mirrors if you prefer that.

The tests are designed to run under perl's prove(1) tool.

The tests test the first instance of "sh" in the PATH. To test other
shells, create a symlink named sh in a new directory and add that
directory in front of your PATH. This is a little clumsy but also
ensures bash and zsh enable their compatibility modes.

-- 
Jilles Tjoelker

Re: trap bug in recent versions of dash

[Herbert Xu]

On Wed, Aug 11, 2010 at 08:06:16AM +0000, Guido Berhoerster wrote:
> Hello,
> 
> with the latest git version of dash trap actions are not
> evaluated in the context of a function.
> 
> The following script demonstrates the bug:
> ----8<----
> read_timeout () {
>     saved_traps="$(trap)"
>     trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
>     ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &
>     timer_pid=$!
>     read $2
>     kill $timer_pid 2>/dev/null
> }
> 
> read_timeout 5 value
> printf "read \"%s\"\n" "${value:=default}"
> 
> ---->8----
> The return statement in the trap inside the read_timeout function
> does not return from the function but rather exits the script.
> 
> With dash 0.5.5.1 it works as expected.

Indeed this was a regression caused by the SKIPEVAL removal.

This patch should fix it.

commit faefce1ebe585366b1458031f2c1f723dc630def
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Sun Nov 28 15:17:45 2010 +0800

    [EVAL] Fixed trap/return regression due to SKIPEVAL removal
    
    On Wed, Aug 11, 2010 at 08:06:16AM +0000, Guido Berhoerster wrote:
    >
    > with the latest git version of dash trap actions are not
    > evaluated in the context of a function.
    >
    > The following script demonstrates the bug:
    > ----8<----
    > read_timeout () {
    >     saved_traps="$(trap)"
    >     trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
    >     ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &
    >     timer_pid=$!
    >     read $2
    >     kill $timer_pid 2>/dev/null
    > }
    >
    > read_timeout 5 value
    > printf "read \"%s\"\n" "${value:=default}"
    >
    > ---->8----
    > The return statement in the trap inside the read_timeout function
    > does not return from the function but rather exits the script.
    >
    > With dash 0.5.5.1 it works as expected.
    
    This bug was caused by the SKIPEVAL removal.  When the SKIPEVAL
    hack was added to improve set -e support in traps, dotrap was
    changed to return whether set -e was detected.  After the removal
    of SKIPEVAL, set -e is now handled through exraise.
    
    However, dotrap still returned a value which is now incorrectly
    used to trigger an exraise.
    
    This patch removes the vestigial link between dotrap and exraise.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index 5ace9ff..f148ef6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2010-11-28  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Fixed trap/return regression due to SKIPEVAL removal.
+
 2010-10-18  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Fix ifsfirst/ifslastp leak in casematch.
diff --git a/src/eval.c b/src/eval.c
index b966749..ea4afc6 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -304,10 +304,16 @@ setstatus:
 		break;
 	}
 out:
-	if ((checkexit & exitstatus) ||
-	    (pendingsigs && dotrap()) ||
-	    (flags & EV_EXIT))
+	if (checkexit & exitstatus)
+		goto exexit;
+
+	if (pendingsigs)
+		dotrap();
+
+	if (flags & EV_EXIT) {
+exexit:
 		exraise(EXEXIT);
+	}
 }
 
 
diff --git a/src/trap.c b/src/trap.c
index 7bd60ab..3d28485 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -309,8 +309,7 @@ onsig(int signo)
  * handlers while we are executing a trap handler.
  */
 
-int
-dotrap(void)
+void dotrap(void)
 {
 	char *p;
 	char *q;
@@ -332,10 +331,8 @@ dotrap(void)
 		evalstring(p, 0);
 		exitstatus = savestatus;
 		if (evalskip)
-			return evalskip;
+			break;
 	}

Re: trap bug in recent versions of dash

[Herbert Xu]

On Sun, Aug 22, 2010 at 10:32:27PM +0000, Jilles Tjoelker wrote:
>
> diff --git a/src/eval.c b/src/eval.c
> index d5e5c95..e484bec 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -307,9 +307,9 @@ setstatus:
>  		break;
>  	}
>  out:
> -	if ((checkexit & exitstatus) ||
> -	    (pendingsigs && dotrap()) ||
> -	    (flags & EV_EXIT))
> +	if (pendingsigs)
> +		dotrap();
> +	if ((flags & EV_EXIT) || (checkexit & exitstatus))
>  		exraise(EXEXIT);
>  }

Thanks, this is pretty much what I applied except that the set -e
check should be applied before dotraps (as was the case originally).
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: trap bug in recent versions of dash

[Guido Berhoerster]

* Herbert Xu <herbert@gondor.apana.org.au> [2010-11-28 08:55]:
> On Wed, Aug 11, 2010 at 08:06:16AM +0000, Guido Berhoerster wrote:
> > Hello,
> > 
> > with the latest git version of dash trap actions are not
> > evaluated in the context of a function.
> > 
> > The following script demonstrates the bug:
> > ----8<----
> > read_timeout () {
> >     saved_traps="$(trap)"
> >     trap 'printf "timed out\n"; eval "${saved_traps}"; return' TERM
> >     ( sleep $1; kill -TERM $$ ) >/dev/null 2>&1 &
> >     timer_pid=$!
> >     read $2
> >     kill $timer_pid 2>/dev/null
> > }
> > 
> > read_timeout 5 value
> > printf "read \"%s\"\n" "${value:=default}"
> > 
> > ---->8----
> > The return statement in the trap inside the read_timeout function
> > does not return from the function but rather exits the script.
> > 
> > With dash 0.5.5.1 it works as expected.
> 
> Indeed this was a regression caused by the SKIPEVAL removal.
> 
> This patch should fix it.

Thanks, it does fix it.

-- 
Guido Berhoerster