PATCH: Seg. fault in chpwd hook in a widget

PATCH: Seg. fault in chpwd hook in a widget

[Peter Stephenson]

Just had a look at the Sourforge bug tracker, which normally I don't
have time to do (please feel free to forward things to the list if you
notice anything there which appears to be reproducible and hasn't been
fixed); issue 2338948 is this:

https://sourceforge.net/tracker/index.php?func=detail&aid=2338948&group_id=4068&atid=104068

This is an interesting interaction of different levels of the shell.  I
think a key part of it is the "source".  It seems that hbegin() in here
is trashing the current history line chline.  It seems to me that we
should only be calling hbegin() after a lexsave(), to protect against
this, and there appears to be nothing in the call to loop() from
source() that does that.

I'm quite surpsised we've never seen anything like this before; there
seems to be no protection against chline being trashed by any old
"source" or "." that comes along.  As far as I can see, lexsave() is the
only way of doing this.  I think the answer is that most of the time the
history mechanism has exited by this time, so chline is already NULL and
its friends aren't being used.

Obviously any alternative opinions would be useful.  Currently that
means Bart, but both of us would be very interested in getting new
apprentice source code gurus...

(I wonder if there's a more efficient way of saving and restoring lexical
context?  It would imply lexical variables being accessed by pointers,
so the answer may well be "no".)

Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.99
diff -u -r1.99 init.c
--- Src/init.c	19 Feb 2009 10:12:39 -0000	1.99
+++ Src/init.c	9 Mar 2009 11:07:31 -0000
@@ -105,6 +105,8 @@
     Eprog prog;
 
     pushheap();
+    if (!toplevel)
+	lexsave();
     for (;;) {
 	freeheap();
 	if (stophist == 3)	/* re-entry via preprompt() */
@@ -199,6 +201,8 @@
 	if (justonce)
 	    break;
     }
+    if (!toplevel)
+	lexrestore();
     popheap();
 }
 


-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: PATCH: Seg. fault in chpwd hook in a widget

[Bart Schaefer]

On Mar 9, 11:22am, Peter Stephenson wrote:
} Subject: PATCH: Seg. fault in chpwd hook in a widget
}
} Just had a look at the Sourforge bug tracker, which normally I don't
} have time to do (please feel free to forward things to the list if you
} notice anything there which appears to be reproducible and hasn't been
} fixed); issue 2338948 is this:

This was originally on zsh-workers, thread starting at 26089.  You even
answered with a patch in 26091, which was applied 2008-11-25 or so says
the ChangeLog, but you followed *that* by saying it might need deeper
inspection.

} This is an interesting interaction of different levels of the shell.  I
} think a key part of it is the "source".  It seems that hbegin() in here
} is trashing the current history line chline.  It seems to me that we
} should only be calling hbegin() after a lexsave(), to protect against
} this, and there appears to be nothing in the call to loop() from
} source() that does that.

I think you're right about lexsave(), although history has never really
been my bit of the code [to the extent that I "have" any bit at all].
Wayne may have some input.
 
} I'm quite surpsised we've never seen anything like this before; there
} seems to be no protection against chline being trashed by any old
} "source" or "." that comes along.  As far as I can see, lexsave() is the
} only way of doing this.  I think the answer is that most of the time the
} history mechanism has exited by this time, so chline is already NULL and
} its friends aren't being used.

So why *hasn't* the history mechanism exited by the time chpwd is
called?  Aha; it's not direclty because of "source", it's because he's
calling "cd" from inside a ZLE widget.  The hooks are invoked in a
context from which they were never meant to be invoked.

So your lexsave()/lexrestore() is probably the correct thing, but there
may be other consequences of hook/widget interaction still waiting to
bite us for other hooks.
 
} Obviously any alternative opinions would be useful.  Currently that
} means Bart, but both of us would be very interested in getting new
} apprentice source code gurus...

Leak some of that sentiment onto zsh-users.  Maybe potential apprentices
just aren't reading -workers.

Re: PATCH: Seg. fault in chpwd hook in a widget

[Peter Stephenson]

On Mon, 09 Mar 2009 08:49:36 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Mar 9, 11:22am, Peter Stephenson wrote:
> } Subject: PATCH: Seg. fault in chpwd hook in a widget
> }
> } Just had a look at the Sourforge bug tracker, which normally I don't
> } have time to do (please feel free to forward things to the list if you
> } notice anything there which appears to be reproducible and hasn't been
> } fixed); issue 2338948 is this:
> 
> This was originally on zsh-workers, thread starting at 26089.  You even
> answered with a patch in 26091, which was applied 2008-11-25 or so says
> the ChangeLog, but you followed *that* by saying it might need deeper
> inspection.

Strange I obviously came to a different conclusion then forgot about it...

> I think you're right about lexsave(), although history has never really
> been my bit of the code [to the extent that I "have" any bit at all].

I've committed it---I think at the worst it can only be unnecessary
sometimes, but virtually all the other interaction with hbegin()/hend()
outside the main command loop has this anyway.

> So why *hasn't* the history mechanism exited by the time chpwd is
> called?  Aha; it's not direclty because of "source", it's because he's
> calling "cd" from inside a ZLE widget.  The hooks are invoked in a
> context from which they were never meant to be invoked.

The "cd -q" stuff is supposed to allow you to do this (and this is even in
use in some of the functions there), but yes, buried "chpwd" etc. calls
tend to cause trouble even when they don't trigger shell problems.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070