From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: zsh-workers-return-43408-ml=inbox.vuxu.org@zsh.org X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,T_DKIMWL_WL_HIGH autolearn=ham autolearn_force=no version=3.4.1 Received: from primenet.com.au (ns1.primenet.com.au [203.24.36.2]) by inbox.vuxu.org (OpenSMTPD) with ESMTP id e22b8ad6 for ; Fri, 7 Sep 2018 14:02:06 +0000 (UTC) Received: (qmail 19531 invoked by alias); 7 Sep 2018 14:01:51 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 43408 Received: (qmail 8002 invoked by uid 1010); 7 Sep 2018 14:01:51 -0000 X-Qmail-Scanner-Diagnostics: from mailout2.w1.samsung.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(210.118.77.12):SA:0(-6.9/5.0):. Processed in 2.068099 secs); 07 Sep 2018 14:01:51 -0000 X-Envelope-From: p.stephenson@samsung.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20180907140144euoutp0233f5ce4ceaef2866a58b2f443ab5f0ea~SIv_0q-690558405584euoutp02K DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1536328904; bh=GeOKKmJnd7w5vPvjntmUKHhy1FNKtqz89k+Nkc//AEE=; h=Date:From:To:Subject:In-Reply-To:References:From; b=JSf4/j2G3REYwSVnA4qJp8kVnzoZJ5AoyoLQlnBcXFxoEPiAnf/uYvXUrI8IzyDFD dxAxlnKTsMLxqrj7xq8Lq8MTsVFxaXOFxHp3eU2fIlLSTHNSA0+asFpG7xszPTHeZa Cf6wgITafYUxTiuWsrk9InefkVl2EAg2M7mrUQIY= X-AuditID: cbfec7f2-5e3ff70000001159-ed-5b9284c7601b Date: Fri, 7 Sep 2018 15:01:40 +0100 From: Peter Stephenson To: Zsh hackers list Subject: PATCH: Rimmerworld pipeline race In-Reply-To: <20180907122145.2af5bcba@camnpupstephen.cam.scsc.local> Organization: SCSC X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrHIsWRmVeSWpSXmKPExsWy7djPc7rHWyZFG/y6J21xsPkhkwOjx6qD H5gCGKO4bFJSczLLUov07RK4MrqPrmQpmK9Ycf7HVqYGxs2SXYycHBICJhJvJx1m7mLk4hAS WMEosXvzSiYIp49J4tvPySwgVUICvUwSx/66dDFygHW83q8MUbOcUWLO/X+MEA5QzaltS6C6 TzNKTL49EWrueUaJp2t+sIGMYhFQkbj9fzcjiM0mYCgxddNsMFtEQEtix8mTTCC2sICmxNfF +8FsTgEXiTsTfjCBrOYXEJK40GwLcbe9xNE9EOW8AoISJ2c+AbuUWUBeYvvbOWB7JQQ+s0lc +3mYEaKoTGLvjvlMEM0uEkv/bWOGsIUlXh3fwg5hy0j83zmfCaK5nVFizaTX7BBOD6PEpqN3 GCGqrCX6bl9kBLmIGejS9bv0IcKOEjdfHmOBhBGfxI23ghAH8UlM2jadGSLMK9HRJgRRrSax o2kr4wRG5VlIXpiF5IVZCPMXMDKvYhRPLS3OTU8tNsxLLdcrTswtLs1L10vOz93ECEwFp/8d /7SD8eulpEOMAhyMSjy8AWWTooVYE8uKK3MPMUpwMCuJ8LrWAYV4UxIrq1KL8uOLSnNSiw8x SnOwKInz8mmlRQsJpCeWpGanphakFsFkmTg4pRoYNXpK/K+KNvDHJIidnXI7WM7k4AQm1plV S4OnuWZr1N/4vPvEFIt+sSdzaq7MScs99sf4QE6paOJSlYDI8zf+SmYvY6/ec5Dx0ylLDiHJ HwF1sp8ux5+pk9GYmdGuN91hkeuhpb8urH20qmRXvzQba9zbk2e+pX+/FLt05bx/7puzF0dP brKap8RSnJFoqMVcVJwIAEU5jf8BAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFuphkeLIzCtJLcpLzFFi42I5/e/4Xd1jLZOiDbbfULE42PyQyYHRY9XB D0wBjFF6NkX5pSWpChn5xSW2StGGFkZ6hpYWekYmlnqGxuaxVkamSvp2NimpOZllqUX6dgl6 Gd1HV7IUzFesOP9jK1MD42bJLkYODgkBE4nX+5W7GDk5hASWMkp87EoGsSUEZCQ+XfnIDmEL S/y51sXWxcgFVNPNJLHh1Dwo5zSjxJSGy+wQznlGiQ3NHWAtLAIqErf/72YEsdkEDCWmbpoN ZosIaEnsOHmSCcQWFtCU+Lp4P5jNK+As8WrHezYQm1PAReLOhB9MEEPfMUv8vHeTEeRUfgEh iQvNthAn2Usc3XMSqldQ4uTMJywgNrOAjsSJVceYIWx5ie1v5zBPYBSehaRsFpKyWUjKFjAy r2IUSS0tzk3PLTbSK07MLS7NS9dLzs/dxAiMiW3Hfm7Zwdj1LvgQowAHoxIPb0DZpGgh1sSy 4srcQ4wSHMxKIryudUAh3pTEyqrUovz4otKc1OJDjKbAwJjILCWanA+M17ySeENTQ3MLS0Nz Y3NjMwslcd7zBpVRQgLpiSWp2ampBalFMH1MHJxSDYytZuGtHUbiXz8Ws/w5fEXFZo4h+2yV K6atlbIy9yYtEnNP/tB44e+mtjfm8Up6k2pfnGoonDihrGyN76O+qlNF56c2rX8l1/20qvy1 raVf2/7Ky3GLksTZCzulZcxfONm8mM4mvWzt29wr/De/vOrNN5e6H/BaRfBN1Nqcg76ykxx5 umMu2SmxFGckGmoxFxUnAgBtk998nwIAAA== Message-Id: <20180907140142eucas1p235260aa99e9d5cf5e7d02bbf05067be4~SIv9WwLnT2225022250eucas1p2W@eucas1p2.samsung.com> X-CMS-MailID: 20180907140142eucas1p235260aa99e9d5cf5e7d02bbf05067be4 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180905200816epcas5p18ce6c49baa637e7d83a769e97c4364fb X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180905200816epcas5p18ce6c49baa637e7d83a769e97c4364fb References: <20180905210740.5a6aec15@pws-HP.localdomain> <20180906090902.1f344e9f@camnpupstephen.cam.scsc.local> <20180906092250eucas1p13d651e07ae627d179dd0701e65f912d6~RxTLztJrP2263722637eucas1p12@eucas1p1.samsung.com> <20180907101852.62415ff9@camnpupstephen.cam.scsc.local> <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; }