rc segfaults when SIGINT is received (FIX)

Save the value of `runq' at the start of the function, so that the `pc' update at the end does work on that original value, and not on a probably modified value of `runq'.

See http://code.swtch.com/plan9port/issue/14/rc-segfaults-when-sigint-is-received#comment-37316 for a more detailed description.

[russcox_gmail.com, 2009-08-08 20:17:23]

can you explain what's going on here?
why does this fix the problem?

[russcox_gmail.com, 2009-08-08 20:17:53]

On 2009/08/08 20:17:23, rsc wrote:
> can you explain what's going on here?
> why does this fix the problem?

hmm.  i wish the automail included the description.
i missed on the web page.  never mind

[russcox_gmail.com, 2009-08-08 20:20:15]

codebot: apply

rc: fix segfault when SIGINT is received

Save the value of `runq' at the start of the function, so that the `pc' update
at the end does work on that original value, and not on a probably modified
value of `runq'.

fixes #14 http://code.swtch.com/plan9port/issue/14/

[old.codebot, 2009-08-08 20:20:37]

# codebot: apply

codebot error: unknown full name for knieriem <mt4swm@googlemail.com>

knieriem, please add a note to this issue with
"codebot: name Your Full Name"
on a line by itself without the quotes.

thanks.
codebot


	Return-Path:
<3AN59SgcJB6YXaYYIUdMSGOR.IUSIUJKHUZXYI.YcZIN.IUS@apphosting.bounces.google.com>
	Received-SPF: Pass; receiver=holo.morphisms.net; client-ip=209.85.217.232;
	Date: Sat, 08 Aug 2009 20:20:16 +0000
	Subject: Re: rc segfaults when SIGINT is received (FIX)
	From: russcox@gmail.com
	
	codebot: apply
	
	rc: fix segfault when SIGINT is received
	
	Save the value of `runq' at the start of the function, so that the `pc'
	update
	at the end does work on that original value, and not on a probably
	modified
	value of `runq'.
	
	fixes #14 http://code.swtch.com/plan9port/issue/14/
	
	
	http://codereview.appspot.com/104066

[russcox_gmail.com, 2009-08-08 20:23:17]

I took care of codebit's whining

codebot: apply

rc: fix segfault when SIGINT is received

Save the value of `runq' at the start of the function, so that the `pc' update
at the end does work on that original value, and not on a probably modified
value of `runq'.

fixes #14 http://code.swtch.com/plan9port/issue/14/

[old.codebot, 2009-08-08 20:24:53]

# codebot: apply

applied.

[knieriem, 2009-08-19 16:46:24]

On 2009/08/08 20:24:53, codebot wrote:
> applied.

It turnes out that the previous patch only is a workaround
for part of the problem. It copes with the situation, when
a `fn sigint' has been defined, which was the context of
the original bug report.  But I does not cover the normal
situation, when no `fn sigint' is defined.

If you interrupt a command
	% echo `{read}
without having a fn sigint defined, still a segmentation
fault will occur.

Apparently p9p rc behaves like this since rev 122,
when as part of various fixes one line was added to
plan9ish.c:Read():
	if(ntrap) dotrap();

To summarize, from rev 122 on, and before the recent
workaround, the behaviour was:

1.	when a script or an interactive shell had a
	fn sigint defined, and was waiting for input in a
	`{} command, and a SIGINT occured, this resulted
	in a segmentation fault

2.	when an interactive shell had _no_ fn sigint
	defined, and was waiting for input in a `{}
	command, and a SIGINT occured, this resulted in
	a segmentation fault too

Now the situation is:

1.	ok, no segfault
2.	segfault

Probably the simple, and sufficient solution is to revert
the previous patch to Xbackq (as that would no longer be
neccessary), and to delete the call to dotrap() in Read().
Plan9ports rc then would have dotrap calls only in main()
and in flush(), like Plan 9's rc.

If you think the extra dotrap call in Read actually can be
abandoned, I can provide a patch for this. At least rc
seems to work properly without it (as it does on 9vx).


Why does the dotrap call in Read cause a segfault, in case
of an interactive shell, when no fn sigint is defined?

    When p9p's rc is blocking in Read within an Xbackq
    command, and a SIGINT occurs, the command executed
    will stop, and readnb() returns 0. Then, in Read, since
    ntrap is non-zero dotrap() will be called, which will
    walk up the rc-stack using Xreturn, freeing all the
    code, also that of the current Xbackq command. The Xbackq
    function - which still has to be completed -, when using
    its pointer to runq, works on memory that has been
    freed already, resulting in a segfault.

    Note that in case of a `fn sigint' defined,
    dotrap's action is different, and the memory is not
    freed. That's why the workaround was effective in this
    special case.

    Also the behaviour above only applies to interactive
    shells, not to scripts, as dotrap/Xreturn will exit
    if there is no interactive command reading loop,
    which will prevent Xbackq from proceeding after the
    dotrap call.

I only have a vague idea what the purpose of the change
in rev 122 could be. Read() is called by emptybuf() only,
which is called by rchr(), which again is used only in
	Xbackq, here commands, and lex' getnext function.

Perhaps the change was related to problems with the signal
handling (lib9/notify.c), which also was improved by rev
122 and later.  Before rev 122 SIGINT did not interrupt
system calls (SA_RESTART was the default), from then on
it does. Maybe the extra dotrap call in Read() had the
intention to improve a situation where signals were handled
to late, but I'm not sure in what concrete situation.