[PATCH] shell script error management in ash (set of 6 patches)

[Harald van Dijk]

On 22/08/2021 00:31, Roberto A. Foglietta wrote:
> Il giorno sab 21 ago 2021 alle ore 23:33 Harald van Dijk 
> <harald at gigawatt.nl <mailto:harald at gigawatt.nl>> ha scritto:
> 
>     Hi again,
> 
>     Another bug: the exit status is not preserved.
> 
>     Consider
> 
>         busybox ash -c 'trap "echo ERR" ERR; false; echo $?'
> 
>     This prints ERR, and then 0, because the echo command completed
>     successfully. It is supposed to print ERR, and then 1, because like
>     EXIT
>     actions, ERR actions are supposed to preserve the exit status of the
>     last command before the action was invoked.
> 
> 
> Patch 11 and testsuite attached.
> In the test suite two cases are evaluated exit status =0 !0

I think that's probably correct but suboptimal. You're saving exitstatus 
using the savestatus global variable, but in that block, you already 
know that exitstatus is always going to be equal to the local status 
variable: the only time that exitstatus != status is if the earlier 
exitstatus = status; assignment is bypassed, but the only way that can 
get bypassed is if evaltree(NULL, flags) is called, in which case status 
will equal 0 so the block never gets entered. You should be able to 
repeat the earlier exitstatus = status; assignment without saving it in 
a new variable.

>     I am also somewhat concerned that there may be cases where the global
>     variables "doingtrap" and "recursive" are set, but evalstring() exits
>     via raise_exception() and they are never cleared, preventing future ERR
>     handlers from running. I do not currently have a test case where it
>     matters, but am not convinced there are no test cases where it matters.
> 
> 
> line 13.320 in evalstring
> 
> exception_handler = savehandler;
> if (ex)
> longjmp(exception_handler->loc, ex);
> 
> do you think that this might resolve the issue?
> 
> exception_handler = savehandler;
> if (ex) {
> doingtrap = 0; recursive = 0;
> longjmp(exception_handler->loc, ex);
> }

I don't think that's right. evalstring() may end up calling evalstring() 
again recursively, and in that case you wouldn't want to clear doingtrap 
or recursive.

> Obviously recursive should become a global variable.
> When an exception is raised, what is going to happen? Exit?
> In case of exit then it is not a problem, IMHO.

That's why I haven't been able to create a test case yet where it goes 
wrong: in *almost* all cases where an exception is raised, the shell 
will terminate. The shell won't immediately terminate, EXIT handlers run 
first, but if that runs in the context of an ERR handler, leaving 
doingtrap and recursive alone is the right thing to do, as ERR handlers 
shouldn't run again.

But only almost all cases, not all cases. One exception, and I am not 
sure if it is the only one, is that the 'command' builtin can catch and 
handle exceptions that come from errors raised by special built-ins. 
Normally they cause the shell to terminate, but the 'command' built-in 
makes such errors non-fatal.

If it is possible for a special built-in within an ERR handler to raise 
an exception, and for that exception to be caught and handled by an 
outer 'command' builtin, it should be possible to run more ERR handlers 
after that.

> Thank you, -R