From: Peter Stephenson <p.stephenson@samsung.com>
To: Zsh hackers list <zsh-workers@zsh.org>
Subject: PATCH: Rimmerworld pipeline race
Date: Fri, 7 Sep 2018 15:01:40 +0100 [thread overview]
Message-ID: <20180907140142eucas1p235260aa99e9d5cf5e7d02bbf05067be4~SIv9WwLnT2225022250eucas1p2W@eucas1p2.samsung.com> (raw)
In-Reply-To: <20180907122145.2af5bcba@camnpupstephen.cam.scsc.local>
OK, this is *much* closer and possibly correct.
When adding a new process to the list in the main shell, we looked to
see if the group leader for that job was empty and if so marked the new
process as the group leader.
This conflicts with entersubsh(), where there are hairy list-pipe tests
to determine what will actually be used as the process group leader
(look in the block controlled by the ESUB_PGRP test --- that's where I
was fiddling in the last patch, but this bit isn't the one that's
wrong).
Most of the time this isn't a problem, but if we decide in the main
shell that that process exited while that group was in control of the terminal,
the main shell will think it's time for it to reclaim the terminal.
Whatever happens to be running in the foreground will then find it's
been pushed into the background.
This showed up in this case
echo help | { cat | more }
because
- The echo had set the process group in a list_pipe_job fashion.
- The cat was in that process group and the bogus logic made the parent
shell think it had its own process group.
- The more put itself in the same process group, too.
- The cat exited. The following logic
if (WIFEXITED(status) &&
pn->pid == jn->gleader &&
killpg(pn->pid, 0) == -1) {
jn->gleader = 0;
if (!(jn->stat & STAT_NOSTTY)) {
/*
* This PID was in control of the terminal;
* reclaim terminal now it has exited.
* It's still possible some future forked
* process of this job will become group
* leader, however.
*/
attachtty(mypgrp);
}
}
then triggered, grabbing back the terminal. The more found itself
without a terminal.
The fix is to second-guess the test the process itself makes when
setting the group leader and to add some clear comments. Then
jn->gleader in the hunk above correctly reflects the actual foreground
process group, so the shell doesn't reclaim the terminal yet.
Neither of the two related bits, in entersubsh() or addproc(), is new,
increasing my suspicion that the problem isn't fundamentally new but a
newly exposed race.
It's hard to think how fixing this can make things worse (although I
always say this). How about committing it, making a new test build,
encouraging EVERYONE to try it out, and seeing if that is releasable?
pws
diff --git a/Src/exec.c b/Src/exec.c
index 09ee132..8062f0c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -994,6 +994,31 @@ enum {
ESUB_JOB_CONTROL = 0x40
};
+
+/*
+ * If the list_pipe magic is in effect, its process group controls
+ * the terminal: return this. Otherwise, return -1.
+ *
+ * C.f. the test in entersubsh() for process group of forked subshell.
+ */
+
+/**/
+int
+get_list_pipe_process_group(void)
+{
+ int gleader;
+ if (!list_pipe && !list_pipe_child)
+ return -1;
+ gleader = jobtab[list_pipe_job].gleader;
+
+ if (gleader &&
+ killpg(gleader, 0) != -1 &&
+ gettygrp() == gleader)
+ return gleader;
+
+ return -1;
+}
+
/**/
static void
entersubsh(int flags)
diff --git a/Src/jobs.c b/Src/jobs.c
index 38b3d89..c841150 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1392,10 +1392,21 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime)
if (!aux)
{
pn->bgtime = *bgtime;
- /* if this is the first process we are adding to *
- * the job, then it's the group leader. */
- if (!jobtab[thisjob].gleader)
- jobtab[thisjob].gleader = pid;
+ /*
+ * If this is the first process we are adding to
+ * the job, then it's the group leader.
+ *
+ * Exception: if the list_pipe_job pipeline madness is in
+ * effect, we need to use the process group that's already
+ * controlling the terminal or we'll attach to something
+ * bogus when that process exits. This test mimics the
+ * test the process itself makes in entersubsh(), so the
+ * two should be kept in sync.
+ */
+ if (!jobtab[thisjob].gleader) {
+ int gleader = get_list_pipe_process_group();
+ jobtab[thisjob].gleader = (gleader == -1) ? pid : gleader;
+ }
/* attach this process to end of process list of current job */
pnlist = &jobtab[thisjob].procs;
}
next prev parent reply other threads:[~2018-09-07 14:02 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20180905200816epcas5p18ce6c49baa637e7d83a769e97c4364fb@epcas5p1.samsung.com>
2018-09-05 20:07 ` PATCH: job control debug Peter Stephenson
2018-09-06 8:09 ` Peter Stephenson
[not found] ` <20180906090902.1f344e9f@camnpupstephen.cam.scsc.local>
2018-09-06 9:22 ` Peter Stephenson
2018-09-07 3:18 ` Bart Schaefer
2018-09-07 9:18 ` Peter Stephenson
[not found] ` <20180907101852.62415ff9@camnpupstephen.cam.scsc.local>
2018-09-07 11:21 ` Peter Stephenson
[not found] ` <20180907122145.2af5bcba@camnpupstephen.cam.scsc.local>
2018-09-07 14:01 ` Peter Stephenson [this message]
[not found] ` <20180907150140.46a05880@camnpupstephen.cam.scsc.local>
2018-09-07 14:58 ` PATCH: Rimmerworld pipeline race Peter Stephenson
2018-09-07 16:46 ` Peter Stephenson
2018-09-07 23:40 ` Bart Schaefer
2018-09-08 17:37 ` Peter Stephenson
2018-09-08 18:40 ` Peter Stephenson
2018-09-10 22:55 ` Axel Beckert
2018-09-11 8:58 ` Peter Stephenson
2018-09-11 14:40 ` Vincent Lefevre
2018-09-11 15:03 ` Peter Stephenson
[not found] ` <20180911160326.33dfd575@camnpupstephen.cam.scsc.local>
2018-09-11 15:32 ` Peter Stephenson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='20180907140142eucas1p235260aa99e9d5cf5e7d02bbf05067be4~SIv9WwLnT2225022250eucas1p2W@eucas1p2.samsung.com' \
--to=p.stephenson@samsung.com \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
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).