Crash of 4.2.0-dev-1

Crash of 4.2.0-dev-1

[Bart Schaefer]

I presume of 4.2.0 also.  Reproduce as follows:

(1) Create a file "kshtest" in a directory in fpath.  This file should NOT
define the function "kshtest", that is, it's not a valid ksh-autoloadable
file (but we're going to autoload it ksh-style anyway).

(2) Run zsh -f and give these commands:

	setopt kshautoload
	autoload +X -k kshtest
	kshtest

(3) Observe the [correct] error:

	zsh: kshtest: function not defined by file

(4) Memory is now corrupted.  At some point soon, usually but not always
during printing of the next prompt, the shell will crash in malloc().

Re: Crash of 4.2.0-dev-1

[Geoff Wing]

Bart Schaefer <schaefer@brasslantern.com> typed:
: I presume of 4.2.0 also.  Reproduce as follows:
:
: (1) Create a file "kshtest" in a directory in fpath.  This file should NOT
: define the function "kshtest", that is, it's not a valid ksh-autoloadable
: file (but we're going to autoload it ksh-style anyway).

Any specific kshtest example for us?

% echo 'echo foo' > /usr/local/share/zsh/site-functions/kshtest
% zsh -f
% setopt kshautoload
% autoload +X -k kshtest
% kshtest
foo
foo
% kshtest
foo
% which kshtest
kshtest () {
        echo foo
}
% echo $ZSH_VERSION
4.2.0
% 

: (3) Observe the [correct] error:
:
: 	zsh: kshtest: function not defined by file

I don't understand why this is the correct error since you  setopt kshautload.
Also, I haven't yet understood why it was run twice in my case (but I've been
sick so maybe my brain isn't working properly yet).

: (4) Memory is now corrupted.  At some point soon, usually but not always
: during printing of the next prompt, the shell will crash in malloc().

I'm still going.  Haven't seen any memory problems yet.  Note that I use
the  --enable-zsh-mem* args to configure.

Regards,
-- 
Geoff Wing

Re: Crash of 4.2.0-dev-1

[Bart Schaefer]

On Apr 11,  8:09am, Geoff Wing wrote:
} Subject: Re: Crash of 4.2.0-dev-1
}
} Bart Schaefer <schaefer@brasslantern.com> typed:
} : I presume of 4.2.0 also.  Reproduce as follows:
} :
} : (1) Create a file "kshtest" in a directory in fpath.  This file should NOT
} : define the function "kshtest"
} 
} Any specific kshtest example for us?

Hmm, I thought I'd reproduced this with a very simple example but I guess
there is one more thing necessary for it to happen reliably.  Here are
the first two steps again, corrected.

(1) Place the following in a file "kshtest" in a directory in fpath:

    print "Running kshtest"
    unfunction kshtest

(2) Run zsh -f and execute:

    setopt kshautoload
    autoload +X -k kshtest
    kshtest

} : (3) Observe the [correct] error:
} :
} : 	zsh: kshtest: function not defined by file
} 
} I don't understand why this is the correct error

Because files loaded with kshautoload have to contain the full function
definition, like so:

    kshtest() {
	print "Running kshtest"
	unfunction kshtest
    }

And the kshtest file as created in (1) does not do so.

} since you setopt kshautload

That's another critical bit in tripping the bug.  After "autoload +X -k"
of the "incorrect" kshtest file, the function kshtest is defined as:

    kshtest () {
        print "Running kshtest"
        unfunction kshtest
	kshtest "$@"
    }

Whereas zsh expects it to have been defined as:

    kshtest () {
	kshtest () {
	    print "Running kshtest"
	    unfunction kshtest
	}
	kshtest "$@"
    }

The extra layer of function wrapper plus the call with "$@" is added
by "autoload +X".

} Also, I haven't yet understood why it was run twice in my case (but
} I've been sick so maybe my brain isn't working properly yet).

Normally, the function redefines itself and then the new definition is
called.  When the file itself does not contain its own function wrapper,
it ends up calling itself -- but only once, because that call re-auto-
loads the file without the +X behavior.  When that happens after the
function has been "unfunction"d, something bad results.

(Normally zsh protects itself against functions that unfunction them-
selves, otherwise "compinit" wouldn't work properly.  Somehow having
kshautoload set is bypassing those defenses.)

} : (4) Memory is now corrupted.  At some point soon, usually but not always
} : during printing of the next prompt, the shell will crash in malloc().
} 
} I'm still going.  Haven't seen any memory problems yet.

The "unfunction" appears to be necessary.

Re: Crash of 4.2.0-dev-1

[Geoff Wing]

Bart Schaefer <schaefer@brasslantern.com> typed:
: On Apr 11,  8:09am, Geoff Wing wrote:
[...]
:     autoload +X -k kshtest
[...]
:} I don't understand why this is the correct error
:
: Because files loaded with kshautoload have to contain the full function
: definition, like so:
:
:     kshtest() {
: 	print "Running kshtest"
: 	unfunction kshtest
:     }

OK, according to the manual zshbuiltins(1) in autoload:

 ... With ksh-style autoloading, the contents of the file will not be
     executed immediately.  Instead, the function created will contain
     the contents of the file ...

I read this to mean the function created has as its body the contents of
the file and as such it wasn't strictly necessary to define the function
itself (guess I wasn't paying too much attention).  Oh, and now I've just
read further down (Oops).

: Whereas zsh expects it to have been defined as:
:
:     kshtest () {
: 	kshtest () {
: 	    print "Running kshtest"
: 	    unfunction kshtest
: 	}
: 	kshtest "$@"
:     }

This is what I get.

It's all working fine for me.  I'll try it without the zsh-mem stuff to
see if that's been avoiding the problem for me. 

Regards,
-- 
Geoff Wing

Re: Crash of 4.2.0-dev-1

[Bart Schaefer]

On Apr 11, 10:40pm, Geoff Wing wrote:
}
} : Because files loaded with kshautoload have to contain the full function
} : definition
} 
} OK, according to the manual zshbuiltins(1) in autoload:
} 
}  ... With ksh-style autoloading, the contents of the file will not be
}      executed immediately.  Instead, the function created will contain
}      the contents of the file ...

Note that this excerpt applies specifically to +X with either -k or with
ksh_autoload set.  The trailing part you've omitted goes on:

   ... plus a call to the function itself appended to it ...

You have to look in zshmisc(1) under "Autoloading functions" to find the
real explanation of ksh_autoload:

       If the KSH_AUTOLOAD option is set, or  the  file  contains
       only  a simple definition of the function, the file's con-
       tents will be executed.  This  will  normally  define  the
       function in question, but may also perform initialization,
       which is executed in the context of  the  function  execu-
       tion, and may therefore define local parameters.  It is an
       error if the function is not defined by loading the  file.

       Otherwise,  the  function body (with no surrounding `func-
       name() {...}') is taken to be the complete contents of the
       file.  ...

} : Whereas zsh expects it to have been defined as:
} :
} :     kshtest () {
} : 	kshtest () {
} : 	    print "Running kshtest"
} : 	    unfunction kshtest
} : 	}
} : 	kshtest "$@"
} :     }
} 
} This is what I get.

If in fact your file "kshtest" contains exactly the two lines
	print "Running kshtest"
	unfunction kshtest
and you ran
	setopt ksh_autoload
	autoload +X -k kshtest
then it should be impossible for you to have gotten the kshtest-within-
kshtest form you just quoted.  If you *do* somehow get the nested def'n
shown above, then the bug *won't* be tickled -- the but only happens if
the nested kshtest def'n does *not* appear, but both the "unfunction"
and the call with "$@" *do* appear.

Re: Crash of 4.2.0-dev-1

[Felix Rosencrantz]

I ran valgrind against the latest cvs zsh with the steps Bart listed and got
the following error:

Invalid read&write of size 4
 doshfunc (exec.c:3547)
 execshfunc (exec.c:3354)
 execcmd (exec.c:2403)
 execpline2 (exec.c:1276)
 execpline (exec.c:1066)
 execlist (exec.c:872)
 execode (exec.c:773)
 loop (init.c:165)
 zsh_main (init.c:1274)
 main (main.c:93)
Address 0x7E849E4 is 0 bytes inside a block of size 36 free'd
 free (vg_replace_malloc.c:231)
 zfree (mem.c:1391)
 freeeprog (parse.c:2153)
 execode (exec.c:775)
 runshfunc (exec.c:3651)


__________________________________
Do you Yahoo!?
Yahoo! Tax Center - File online by April 15th
http://taxes.yahoo.com/filing.html

Re: Crash of 4.2.0-dev-1

[Peter Stephenson]

Bart Schaefer wrote:
> I presume of 4.2.0 also.  Reproduce as follows:
>
> (1) Place the following in a file "kshtest" in a directory in fpath:
> 
>     print "Running kshtest"
>     unfunction kshtest
> 
> (2) Run zsh -f and execute:
> 
>     setopt kshautoload
>     autoload +X -k kshtest
>     kshtest
> 
> (3) Observe the [correct] error:
> 
> 	zsh: kshtest: function not defined by file

This must be from the EF_RUN test in doshfunc, right?  (Because of the
logic for autoload +X you can't get it the other way.)  That means you
see the message just after the outer kshtest is run, I think.

I believe (but haven't checked) we always duplicate a function to
execute so that it's safe to unfunction while it's being executed.
Could that be failing here?

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: Crash of 4.2.0-dev-1

[Bart Schaefer]

On Apr 15,  7:13pm, Peter Stephenson wrote:
> > 
> > 	zsh: kshtest: function not defined by file
> 
> This must be from the EF_RUN test in doshfunc, right?

I wasn't sure, but I just tried setting breakpoints on both places and
that's the one it hit.

> That means you see the message just after the outer kshtest is run,
> I think.

Right, except that there is no "inner" definition of kshtest in this
case -- there's an "inner" *call*:

zagzig% autoload +X -k kshtest
zagzig% functions kshtest
kshtest () {
        print "Running kshtest"
        unfunction kshtest
        kshtest "$@"
}
zagzig% kshtest
Running kshtest
zsh: kshtest: function not defined by file

> I believe (but haven't checked) we always duplicate a function to
> execute so that it's safe to unfunction while it's being executed.
> Could that be failing here?

Yes, that's what I think is happening.  It may be of interest to note
that the `kshtest "$@"' line either never executes, or does so without
producing any output or errors of any kind.  (I would have at least
expected it to say "command not found".)  There's no second call to
exec.c:loadautofn() which I at first suspected might be the reason for
the crash.  (The first call to loadautofn() is from `autoload -X'.)

Or at least the above is what happens when I run it in the debugger.
I suppose something different might be happening when not debugging.

Oliver's patch in 19767 doesn't seem to make any difference to this,
not that I expected it to.

PATCH: Re: Crash of 4.2.0-dev-1

[Peter Stephenson]

Bart Schaefer wrote:
> I presume of 4.2.0 also.  Reproduce as follows:
>
> (1) Place the following in a file "kshtest" in a directory in fpath:
> 
>     print "Running kshtest"
>     unfunction kshtest
> 
> (2) Run zsh -f and execute:
> 
>     setopt kshautoload
>     autoload +X -k kshtest
>     kshtest
> 
> (3) Observe the [correct] error:
> 
> 	zsh: kshtest: function not defined by file

I wonder if the following is advisable?  I haven't investigated in any
more detail whether it helps.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.60
diff -u -r1.60 exec.c
--- Src/exec.c	20 Apr 2004 12:11:16 -0000	1.60
+++ Src/exec.c	21 Apr 2004 15:07:31 -0000
@@ -3546,10 +3546,10 @@
     if (prog->flags & EF_RUN) {
 	Shfunc shf;
 
-	runshfunc(prog, NULL, fstack.name);
-
 	prog->flags &= ~EF_RUN;
 
+	runshfunc(prog, NULL, fstack.name);
+
 	if (!(shf = (Shfunc) shfunctab->getnode(shfunctab,
 						(name = fname)))) {
 	    zwarn("%s: function not defined by file", name, 0);

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: PATCH: Re: Crash of 4.2.0-dev-1

[Bart Schaefer]

On Apr 21,  4:11pm, Peter Stephenson wrote:
> I wonder if the following is advisable?  I haven't investigated in any
> more detail whether it helps.

I see you committed this with a remark about not having gotten feedback.

The ony feedback I can give is that I can't reproduce the crash any more.
However, I stopped being able to reproduce the crash _before_ this patch
was applied to the source, so I don't know whether this specific change
helps, either.