eval: Always set exitstatus in evaltree

Commit Message

Herbert Xu Dec. 6, 2022, 8:49 a.m. UTC

There is no harm in setting exitstatus unconditionally in evaltree.

Signed-off-by: Herbert Xu <[e-mail]>

Comments

Harald van Dijk Dec. 6, 2022, 10:15 a.m. UTC | #1

On 06/12/2022 08:49, Herbert Xu wrote:
> There is no harm in setting exitstatus unconditionally in evaltree.

There is a long-standing bug that may or may not be harder to fix if 
this patch goes in, depending on how you want to fix it. Here's a script 
that already fails on current dash.

   f() {
     if ! return 0
     then :
     fi
   }
   f

This should return 0, and does return 0 in bash and ksh (and almost all 
shells), but returns 1 in dash.

There are a few possible ways of fixing it. Some of them rely on 
continuing to conditionally set exitstatus.

Cheers,
Harald van Dijk

Herbert Xu Dec. 6, 2022, 10:40 a.m. UTC | #2

On Tue, Dec 06, 2022 at 10:15:03AM +0000, Harald van Dijk wrote:
>
> There is a long-standing bug that may or may not be harder to fix if this
> patch goes in, depending on how you want to fix it. Here's a script that
> already fails on current dash.
> 
>   f() {
>     if ! return 0
>     then :
>     fi
>   }
>   f
> 
> This should return 0, and does return 0 in bash and ksh (and almost all
> shells), but returns 1 in dash.
> 
> There are a few possible ways of fixing it. Some of them rely on continuing
> to conditionally set exitstatus.

Thanks.  I'll keep this in mind.  We could always back this out
if needed.

Patch

diff --git a/src/eval.c b/src/eval.c
index 3337f71..eda251e 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -239,7 +239,7 @@  evaltree(union node *n, int flags)
 #endif
 	case NNOT:
 		status = !evaltree(n->nnot.com, EV_TESTED);
-		goto setstatus;
+		break;
 	case NREDIR:
 		errlinno = lineno = n->nredir.linno;
 		if (funcline)
@@ -250,7 +250,7 @@  evaltree(union node *n, int flags)
 			 evaltree(n->nredir.n, flags & EV_TESTED);
 		if (n->nredir.redirect)
 			popredir(0);
-		goto setstatus;
+		break;
 	case NCMD:
 		evalfn = evalcommand;
 checkexit:
@@ -292,7 +292,7 @@  evaln:
 		evalfn = evaltree;
 calleval:
 		status = evalfn(n, flags);
-		goto setstatus;
+		break;
 	case NIF:
 		status = evaltree(n->nif.test, EV_TESTED);
 		if (evalskip)
@@ -305,13 +305,14 @@  calleval:
 			goto evaln;
 		}
 		status = 0;
-		goto setstatus;
+		break;
 	case NDEFUN:
 		defun(n);
-setstatus:
-		exitstatus = status;
 		break;
 	}
+
+	exitstatus = status;
+
 out:
 	dotrap();