dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'
@ 2016-10-27 21:40 Martijn Dekker
  2016-10-28 13:55 ` Denys Vlasenko
  0 siblings, 1 reply; 4+ messages in thread
From: Martijn Dekker @ 2016-10-27 21:40 UTC (permalink / raw)
  To: dash, busybox

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'
  2016-10-27 21:40 [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval' Martijn Dekker
@ 2016-10-28 13:55 ` Denys Vlasenko
  2016-10-29 18:22   ` Harald van Dijk
  0 siblings, 1 reply; 4+ messages in thread
From: Denys Vlasenko @ 2016-10-28 13:55 UTC (permalink / raw)
  To: Martijn Dekker, Herbert Xu; +Cc: dash, busybox

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'
  2016-10-28 13:55 ` Denys Vlasenko
@ 2016-10-29 18:22   ` Harald van Dijk
  2016-10-30 17:32     ` Denys Vlasenko
  0 siblings, 1 reply; 4+ messages in thread
From: Harald van Dijk @ 2016-10-29 18:22 UTC (permalink / raw)
  To: Denys Vlasenko, Martijn Dekker, Herbert Xu; +Cc: dash, busybox

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval'
  2016-10-29 18:22   ` Harald van Dijk
@ 2016-10-30 17:32     ` Denys Vlasenko
  0 siblings, 0 replies; 4+ messages in thread
From: Denys Vlasenko @ 2016-10-30 17:32 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: Martijn Dekker, Herbert Xu, dash, busybox

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-10-30 17:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-27 21:40 [BUG] Interactive (d)ash exits on assigning to readonly from 'command eval' Martijn Dekker
2016-10-28 13:55 ` Denys Vlasenko
2016-10-29 18:22   ` Harald van Dijk
2016-10-30 17:32     ` Denys Vlasenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).