Segfault in =( ) substitution

Segfault in =( ) substitution

[Vin Shelton]

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

Greetings -

Starting very recently, I'm getting a core dump from this code:

Contents of ./tst1
#!/opt/build/zsh-2012-10-11/Src/zsh

emulate -LR zsh

file1=/opt/build/xemacs-21.5-2012-10-01/Installation
pat1='s/2012-10-01/2012-10-XXX/g'

diff1=$file1
[[ -n $pat1 ]] && diff1=( =(perl -p -e $pat1 < $file1) )


./tst1
zsh: segmentation fault (core dumped)  ./tst1
legolas [./tst1] ~
            12-10-14 10:15PM
: ~ Sun 14 22:15; gdb /opt/build/zsh-2012-10-11/Src/zsh core.*(om[1])
GNU gdb (GDB) Fedora (7.4.50.20120120-50.fc17)
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /opt/build/zsh-2012-10-11/Src/zsh...done.

warning: core file may not match specified executable file.
[New LWP 20029]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `/opt/build/zsh-2012-10-11/Src/zsh ./tst1'.
Program terminated with signal 11, Segmentation fault.
#0  0x0000000000475305 in stringsubst (list=list@entry=0x7f8d5a861440,
node=0x7f8d5a861458,
    pf_flags=pf_flags@entry=0, asssub=asssub@entry=0)
    at ../../../src/zsh-2012-10-11/Src/subst.c:175
175                 restlen = strlen(rest);
Missing separate debuginfos, use: debuginfo-install
gdbm-1.10-2.fc17.x86_64 glibc-2.15-57.fc17.x86_64
ncurses-libs-5.9-4.20120204.fc17.x86_64 pcre-8.21-5.fc17.x86_64
(gdb) bt
#0  0x0000000000475305 in stringsubst (list=list@entry=0x7f8d5a861440,
node=0x7f8d5a861458,
    pf_flags=pf_flags@entry=0, asssub=asssub@entry=0)
    at ../../../src/zsh-2012-10-11/Src/subst.c:175
#1  0x000000000047a95b in prefork (list=list@entry=0x7f8d5a861440, flags=2)
    at ../../../src/zsh-2012-10-11/Src/subst.c:77
#2  0x00000000004251b4 in addvars (state=0x7fffd3e81270, pc=<optimized
out>, addflags=0)
    at ../../../src/zsh-2012-10-11/Src/exec.c:2199
#3  0x000000000042555b in execsimple (state=0x0, state@entry=0x7fffd3e81270)
    at ../../../src/zsh-2012-10-11/Src/exec.c:1097
#4  0x000000000042b965 in execlist (state=state@entry=0x7fffd3e81270,
    dont_change_job=dont_change_job@entry=0, exiting=exiting@entry=0)
    at ../../../src/zsh-2012-10-11/Src/exec.c:1243
#5  0x000000000042bf30 in execode (p=p@entry=0x7f8d5a861368,
    dont_change_job=dont_change_job@entry=0, exiting=exiting@entry=0,
    context=context@entry=0x4899b1 "toplevel") at
../../../src/zsh-2012-10-11/Src/exec.c:1057
#6  0x000000000043bb21 in loop (toplevel=toplevel@entry=1,
justonce=justonce@entry=0)
    at ../../../src/zsh-2012-10-11/Src/init.c:185
#7  0x000000000043e6cd in zsh_main (argc=<optimized out>, argv=<optimized out>)
    at ../../../src/zsh-2012-10-11/Src/init.c:1616
#8  0x000000000040dfd5 in main (argc=<optimized out>, argv=<optimized out>)
    at ../../../src/zsh-2012-10-11/Src/main.c:93
(gdb) p subst
$1 = <optimized out>
(gdb) p str
$2 = 0x7f8d5a861470 "\213\210perl -p -e $pat1 < $file1\211"
(gdb) p sublen
$3 = 0
(gdb) p rest
$4 = 0x0


It looks like a guard on rest is needed; in case it's just that
simple, a patch is attached.

  - Vin

[-- Attachment #2: d --]
[-- Type: application/octet-stream, Size: 1019 bytes --]

? last_update
Index: ChangeLog
===================================================================
RCS file: /cvsroot/zsh/zsh/ChangeLog,v
retrieving revision 1.5745
diff -a -u -r1.5745 ChangeLog
--- ChangeLog	11 Oct 2012 20:14:01 -0000	1.5745
+++ ChangeLog	15 Oct 2012 03:27:14 -0000
@@ -1,3 +1,7 @@
+2012-10-14  Vin Shelton  <acs@xemacs.org>
+
+	* Src/subst.c: guard against rest being NULL.
+
 2012-10-11  Peter Stephenson  <p.w.stephenson@ntlworld.com>
 
 	* 30726: Doc/Zsh/builtins.yo, Src/builtin.c, Src/exec.c,
Index: Src/subst.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/subst.c,v
retrieving revision 1.139
diff -a -u -r1.139 subst.c
--- Src/subst.c	5 Oct 2012 21:35:06 -0000	1.139
+++ Src/subst.c	15 Oct 2012 03:27:14 -0000
@@ -170,7 +170,9 @@
 		return NULL;
 	    if (!subst)
 		subst = "";
-
+	    if (!rest)
+		rest = "";
+	    
 	    sublen = strlen(subst);
 	    restlen = strlen(rest);
 	    sptr = snew = hcalloc(str3len + sublen + restlen + 1);

Re: Segfault in =( ) substitution

[Peter Stephenson]

On 15 October 2012 04:30, Vin Shelton <acs@alumni.princeton.edu> wrote:
> Starting very recently, I'm getting a core dump from this code:
>
> [[ -n $pat1 ]] && diff1=( =(perl -p -e $pat1 < $file1) )

What's happened here is that =(...) failed because there was no longer
an associated job for it to attach itself to within the variable
assignment.  This is exacerbated by the fact that error handling
in getoutputfile() is poor.

The change in behaviour comes from my patch to invalidate thisjob
within execsimple().  I still think that's correct: the "thisjob" the
expansion was (previously) assigning itself too would be whatever
thisjob happened to be left set to after the last command ran, which
isn't the right thing to do (except by coincidence).

The right thing to do would be to ensure that assignments that have
complex substitutions in aren't marked as "simple".  Failing that,
marking array substitutions as not simple might be good enough.

Now I have to understand parse.c...

pws

Re: Segfault in =( ) substitution

[Peter Stephenson]

On Sun, 14 Oct 2012 23:30:28 -0400
Vin Shelton <acs@alumni.princeton.edu> wrote:
> Starting very recently, I'm getting a core dump from this code:
>...
> [[ -n $pat1 ]] && diff1=( =(perl -p -e $pat1 < $file1) )

This should fix the crash, however the resulting code isn't very useful.
The job created for the assignment finishes after the assigment
finishes, so the contents of the array is a file that's already been
deleted.

I'm not really sure if there's anything sensible to be done about this.
It might be possible to detect that the assignment is within some other
structure, e.g. shell function, and associate it with that (maybe this
used to happen by accident before), but in a top-level script or
interactive shell there's no such job, so this doesn't generally work.

Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.92
diff -p -u -r1.92 parse.c
--- Src/parse.c	11 Oct 2012 20:14:03 -0000	1.92
+++ Src/parse.c	17 Oct 2012 19:16:13 -0000
@@ -1610,6 +1610,11 @@ par_simple(int *complex, int nr)
 	} else if (tok == ENVARRAY) {
 	    int oldcmdpos = incmdpos, n, type2;
 
+	    /*
+	     * We consider array setting complex because it can
+	     * contain process substitutions, which need a valid job.
+	     */
+	    *complex = c = 1;
 	    p = ecadd(0);
 	    incmdpos = 0;
 	    if ((type2 = strlen(tokstr) - 1) && tokstr[type2] == '+') {

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Segfault in =( ) substitution

[Peter Stephenson]

On Wed, 17 Oct 2012 20:20:30 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sun, 14 Oct 2012 23:30:28 -0400
> Vin Shelton <acs@alumni.princeton.edu> wrote:
> > Starting very recently, I'm getting a core dump from this code:
> >...
> > [[ -n $pat1 ]] && diff1=( =(perl -p -e $pat1 < $file1) )
> 
> This should fix the crash, however the resulting code isn't very useful.

I suppose it's worth pointing out you can now do things like this...

() {
   print File $1:
   cat $1
} =(print This be the verse)

producing:

File /tmp/zsh0wPl3Y:
This be the verse

That's a better bet than any hack within the shell that I can think of
because the scoping is obvious.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Segfault in =( ) substitution

[Bart Schaefer]

On Oct 17,  9:16pm, Peter Stephenson wrote:
:
: I suppose it's worth pointing out you can now do things like this...
: 
: () {
:    print File $1:
:    cat $1
: } =(print This be the verse)

More than worth pointing out, it's worth putting in the docs as an
example!

Question is, whether it's better as an example of anonymous functions or
of process substitution.

Re: Segfault in =( ) substitution

[Peter Stephenson]

On 18 October 2012 16:07, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 17,  9:16pm, Peter Stephenson wrote:
> :
> : I suppose it's worth pointing out you can now do things like this...
> :
> : () {
> :    print File $1:
> :    cat $1
> : } =(print This be the verse)
>
> More than worth pointing out, it's worth putting in the docs as an
> example!
>
> Question is, whether it's better as an example of anonymous functions or
> of process substitution.

Index: Doc/Zsh/expn.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/expn.yo,v
retrieving revision 1.146
diff -p -u -r1.146 expn.yo
--- Doc/Zsh/expn.yo	18 Oct 2012 08:28:48 -0000	1.146
+++ Doc/Zsh/expn.yo	18 Oct 2012 15:24:50 -0000
@@ -483,6 +483,24 @@ example(LPAR()mycmd =(myoutput)RPAR() &!
 as the forked subshell will wait for the command to finish then remove
 the temporary file.

+A general workaround to ensure a process substitution endures for
+an appropriate length of time is to pass it as a parameter to
+an anonymous shell function (a piece of shell code that is run
+immediately with function scope).  For example, this code:
+
+example(LPAR()RPAR() {
+   print File $1:
+   cat $1
+} =+LPAR()print This be the verse+RPAR())
+
+outputs something resembling the following
+
+example(File /tmp/zsh6nU0kS:
+This be the verse)
+
+The temporary file created by the process substitution will be deleted
+when the function exits.
+
 texinode(Parameter Expansion)(Command Substitution)(Process
Substitution)(Expansion)
 sect(Parameter Expansion)
 cindex(parameter expansion)

pws

Re: Segfault in =( ) substitution

[Han Pingtian]

On Wed, Oct 17, 2012 at 09:16:03PM +0100, Peter Stephenson wrote:
> I suppose it's worth pointing out you can now do things like this...
> 
> () {
>    print File $1:
>    cat $1
> } =(print This be the verse)
> 
> producing:
> 
> File /tmp/zsh0wPl3Y:
> This be the verse
> 
> That's a better bet than any hack within the shell that I can think of
> because the scoping is obvious.
> 

Hello,

I just noticed that this example crashed on zsh both 5.0 and 5.0.1. If I
compile zsh with debug enabled, it runs like this:

% () {
function> print File $1:
function> cat $1
function> } =(print hello)
File :
cat: : No such file or directory

Please take a look. Thanks.

Re: Segfault in =( ) substitution

[Han Pingtian]

On Fri, Dec 21, 2012 at 03:57:51PM +0800, Han Pingtian wrote:
> On Wed, Oct 17, 2012 at 09:16:03PM +0100, Peter Stephenson wrote:
> > I suppose it's worth pointing out you can now do things like this...
> > 
> > () {
> >    print File $1:
> >    cat $1
> > } =(print This be the verse)
> > 
> > producing:
> > 
> > File /tmp/zsh0wPl3Y:
> > This be the verse
> > 
> > That's a better bet than any hack within the shell that I can think of
> > because the scoping is obvious.
> > 
> 
> Hello,
> 
> I just noticed that this example crashed on zsh both 5.0 and 5.0.1. If I
> compile zsh with debug enabled, it runs like this:
> 
> % () {
> function> print File $1:
> function> cat $1
> function> } =(print hello)
> File :
> cat: : No such file or directory
> 
> Please take a look. Thanks.

Looks like if revert the below commit this example will work again:

commit ad92cb3203e5d95be91019633e8f1f5835b12794
Author: Peter Stephenson <pws@users.sourceforge.net>
Date:   Thu Oct 11 16:36:14 2012 +0000

    30724: shell code optimisd to use execsimple() doesn't have a valid
    thisjob

    diff --git a/ChangeLog b/ChangeLog
    index 9768aa8..5a69f01 100644
    --- a/ChangeLog
    +++ b/ChangeLog
    @@ -1,3 +1,8 @@
    +2012-10-11  Peter Stephenson  <pws@csr.com>
    +
    +       * 30724: Src/exec.c, Src/jobs.c: shell code optimised to use
    +       execsimple() doesn't have a valid thisjob.
    +
     2012-10-09  Peter Stephenson  <pws@csr.com>

... ...

Re: Segfault in =( ) substitution

[Han Pingtian]

On Fri, Dec 21, 2012 at 05:01:26PM +0800, Han Pingtian wrote:
> 
> commit ad92cb3203e5d95be91019633e8f1f5835b12794
> Author: Peter Stephenson <pws@users.sourceforge.net>
> Date:   Thu Oct 11 16:36:14 2012 +0000
> 
>     30724: shell code optimisd to use execsimple() doesn't have a valid
>     thisjob
> 
>     diff --git a/ChangeLog b/ChangeLog
>     index 9768aa8..5a69f01 100644
>     --- a/ChangeLog
>     +++ b/ChangeLog
>     @@ -1,3 +1,8 @@
>     +2012-10-11  Peter Stephenson  <pws@csr.com>
>     +
>     +       * 30724: Src/exec.c, Src/jobs.c: shell code optimised to use
>     +       execsimple() doesn't have a valid thisjob.
>     +
>      2012-10-09  Peter Stephenson  <pws@csr.com>
> 
> ... ...

And it looks like I cannot reproduce the crash reported in 30723 with
the build without this commit at all. Maybe the problem of 30723 isn't
caused by reasons we trusted at that time? 

I first checkout a 'detached HEAD' by 'git checkout 34ed3ea', where the
34ed3ea commit is just before ad92cb3 commit. And it looks the build
works just fine:

localhost% cat ./t.sh
precmd () {
   {}
}
localhost% source t.sh
localhost% type -f precmd
precmd () {
        {

        }
}
localhost% sleep 50 &; sleep 30
[1] 10503
^Z
zsh: suspended  sleep 30
localhost%
[1]  - done       sleep 50
localhost%

Please correct me if I'm missing something here. Thanks.

Re: Segfault in =( ) substitution

[Peter Stephenson]

On Fri, 21 Dec 2012 15:57:51 +0800
Han Pingtian <hanpt@linux.vnet.ibm.com> wrote:
> I just noticed that this example crashed on zsh both 5.0 and 5.0.1.
> If I compile zsh with debug enabled, it runs like this:
> 
> % () {
> function> print File $1:
> function> cat $1
> function> } =(print hello)
> File :
> cat: : No such file or directory

Glad you noticed this, it's a standard piece of luck we found out
a day late...

Anonymous functions can't be "simple", they need a valid process
environment because (unlike normal function definitions) they can take
arbitrary arguments, as in this case.  (We could probe the arguments
individually to see, but this fix is a good deal simpler.)

If anyone can further elucidate the relationship between job control (in
particular, the current job recorded in "thisjob") and shell code
optimised to "simple" execution, feel free, otherwise whether or not it
fails to crash because it's happened to pick up a version of "thisjob"
from somewhere else isn't really interesting.  The change in 30876 would
have further perturbed the environment seen by execsimple().

Moral: when you spot "gr8 new featurez" you hadn't realised the shell
could do, add tests for them.

Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.93
diff -p -u -r1.93 parse.c
--- Src/parse.c	25 Oct 2012 08:54:33 -0000	1.93
+++ Src/parse.c	21 Dec 2012 10:20:59 -0000
@@ -846,7 +846,7 @@ par_cmd(int *complex)
 	break;
     case FUNC:
 	cmdpush(CS_FUNCDEF);
-	par_funcdef();
+	par_funcdef(complex);
 	cmdpop();
 	break;
     case DINBRACK:
@@ -1420,7 +1420,7 @@ par_subsh(int *complex)
 
 /**/
 static void
-par_funcdef(void)
+par_funcdef(int *complex)
 {
     int oecused = ecused, num = 0, onp, p, c = 0;
     int so, oecssub = ecssub;
@@ -1471,6 +1471,7 @@ par_funcdef(void)
 	if (num == 0) {
 	    /* Anonymous function, possibly with arguments */
 	    incmdpos = 0;
+	    *complex = 1;
 	}
 	zshlex();
     } else if (unset(SHORTLOOPS)) {
@@ -1735,6 +1736,7 @@ par_simple(int *complex, int nr)
 		if (argc == 0) {
 		    /* Anonymous function, possibly with arguments */
 		    incmdpos = 0;
+		    *complex = 1;
 		}
 		zshlex();
 	    } else {
Index: Test/D03procsubst.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/D03procsubst.ztst,v
retrieving revision 1.6
diff -p -u -r1.6 D03procsubst.ztst
--- Test/D03procsubst.ztst	17 Aug 2011 20:26:05 -0000	1.6
+++ Test/D03procsubst.ztst	21 Dec 2012 10:20:59 -0000
@@ -88,3 +88,22 @@
   print something=${:-=(echo 'C,D),(F,G)'}
 1: Graceful handling of bad substitution in enclosed context
 ?(eval):1: unterminated `=(...)'
+
+  () {
+     print -n "first: "
+     cat $1
+     print -n "second: "
+     cat $2
+  } =(echo This becomes argument one) =(echo and this argument two)
+  function {
+     print -n "third: "
+     cat $1
+     print -n "fourth: "
+     cat $2
+  } =(echo This becomes argument three) =(echo and this argument four)
+0:Process environment of anonymous functions
+>first: This becomes argument one
+>second: and this argument two
+>third: This becomes argument three
+>fourth: and this argument four
+

-- 
Peter Stephenson <p.stephenson@samsung.com>       Consultant, Software
Tel: +44 (0)1223 434724              Samsung Cambridge Solution Centre
St John's House, St John's Innovation Park,
Cowley Road, Cambridge, CB4 0ZT, UK