[BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'

[BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'

[Martijn Dekker]

This bug is on both dash and busybox ash.

The "command" builtin is supposed to stop special builtins (such as
"eval") from exiting the shell on error. So doing something like

   isreadonly() {
      ! command eval "$1=" 2>/dev/null
   }

ought to be a way to test if a variable is read-only without the
performance hit of forking a subshell.

Using this function works fine in scripts, but it immediately makes an
interactive dash or ash exit. The same happens if you try a similar
command manually.

bash$ dash
$ readonly bla=123
$ command eval bla=457
dash: 1: eval: bla: is read only
$
bash$

After the "is read only" error, dash prints a prompt, but does not wait
for input and exits instead. Busybox ash does not even print the prompt
before exiting.

The funny part is that the interactive shell does not exit if you leave
out the 'command'. The very thing that is supposed to stop a
non-interactive shell from exiting makes an interactive shell exit.

HTH,

- M.

Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'

[Denys Vlasenko]

This will probably be mangled by gmail, but here is the proposed fix:

Date: Fri, 28 Oct 2016 15:43:50 +0200
Subject: [PATCH] ash: fix interactive "command eval STRING" exiting on errors.

This bug is also present in current dash

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 shell/ash.c                             | 25 ++++++++++++++++++++++++-
 shell/ash_test/ash-vars/readonly1.right |  2 ++
 shell/ash_test/ash-vars/readonly1.tests |  7 +++++++
 3 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 shell/ash_test/ash-vars/readonly1.right
 create mode 100755 shell/ash_test/ash-vars/readonly1.tests

diff --git a/shell/ash.c b/shell/ash.c
index 1ef02b8..fe11245 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -2180,6 +2180,7 @@ setvareq(char *s, int flags)
             if (flags & VNOSAVE)
                 free(s);
             n = vp->var_text;
+            exitstatus = 1;
             ash_msg_and_raise_error("%.*s: is read only",
strchrnul(n, '=') - n, n);
         }

@@ -9599,7 +9600,7 @@ evalcommand(union node *cmd, int flags)
         if (evalbltin(cmdentry.u.cmd, argc, argv, flags)) {
             if (exception_type == EXERROR && spclbltin <= 0) {
                 FORCE_INT_ON;
-                break;
+                goto readstatus;
             }
  raise:
             longjmp(exception_handler->loc, 1);
@@ -12280,6 +12281,10 @@ expandstr(const char *ps)
 static int
 evalstring(char *s, int flags)
 {
+    struct jmploc *volatile savehandler = exception_handler;
+    struct jmploc jmploc;
+    int ex;
+
     union node *n;
     struct stackmark smark;
     int status;
@@ -12289,6 +12294,19 @@ evalstring(char *s, int flags)
     setstackmark(&smark);

     status = 0;
+    /* On exception inside execution loop, we must popfile().
+     * Try interactively:
+     *    readonly a=a
+     *    command eval "a=b"  # throws "is read only" error
+     * "command BLTIN" is not supposed to abort (even in non-interactive use).
+     * But if we skip popfile(), we hit EOF in eval's string, and exit.
+     */
+    savehandler = exception_handler;
+    exception_handler = &jmploc;
+    ex = setjmp(jmploc.loc);
+    if (ex)
+        goto out;
+
     while ((n = parsecmd(0)) != NODE_EOF) {
         int i;

@@ -12299,10 +12317,15 @@ evalstring(char *s, int flags)
         if (evalskip)
             break;
     }
+ out:
     popstackmark(&smark);
     popfile();
     stunalloc(s);

+    exception_handler = savehandler;
+    if (ex)
+                longjmp(exception_handler->loc, ex);
+
     return status;
 }

diff --git a/shell/ash_test/ash-vars/readonly1.right
b/shell/ash_test/ash-vars/readonly1.right
new file mode 100644
index 0000000..2b363e3
--- /dev/null
+++ b/shell/ash_test/ash-vars/readonly1.right
@@ -0,0 +1,2 @@
+One:1
+One:1
diff --git a/shell/ash_test/ash-vars/readonly1.tests
b/shell/ash_test/ash-vars/readonly1.tests
new file mode 100755
index 0000000..81b461f
--- /dev/null
+++ b/shell/ash_test/ash-vars/readonly1.tests
@@ -0,0 +1,7 @@
+readonly bla=123
+# Bare "eval bla=123" should abort ("eval" is a special builtin):
+(eval bla=123 2>/dev/null; echo BUG)
+echo One:$?
+# "command BLTIN" disables "special-ness", should not abort:
+command eval bla=123 2>/dev/null
+echo One:$?
-- 
2.9.2

Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'

[Harald van Dijk]

On 28/10/16 15:55, Denys Vlasenko wrote:
> This will probably be mangled by gmail, but here is the proposed fix:

This looks about the right approach, but it causes problems in 
subshells, a double free:

$ ./busybox ash -c 'readonly x; echo $(command eval x=2)'
ash: eval: line 1: x: is read only
*** Error in `./busybox': free(): invalid pointer: 0x000055a784c1c300 ***
[...]

That's with busybox checked out from git (commit 
9db74e49e5b462089c6eec0182d819c0d4708e57), where your patch is applied, 
completely unpatched and completely default config.

I omitted the backtrace output, but it's popfile() getting called, after 
popallfiles() has already been called.

Cheers,
Harald van Dijk

Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'

[Denys Vlasenko]

On Sat, Oct 29, 2016 at 8:22 PM, Harald van Dijk <harald@gigawatt.nl> wrote:
> On 28/10/16 15:55, Denys Vlasenko wrote:
>>
>> This will probably be mangled by gmail, but here is the proposed fix:
>
>
> This looks about the right approach, but it causes problems in subshells, a
> double free:
>
> $ ./busybox ash -c 'readonly x; echo $(command eval x=2)'
> ash: eval: line 1: x: is read only
> *** Error in `./busybox': free(): invalid pointer: 0x000055a784c1c300 ***
> [...]
>
> That's with busybox checked out from git (commit
> 9db74e49e5b462089c6eec0182d819c0d4708e57), where your patch is applied,
> completely unpatched and completely default config.
>
> I omitted the backtrace output, but it's popfile() getting called, after
> popallfiles() has already been called.

Thanks! Hopefully fixed in git, please try it.