Bug 1333829

Summary: ksh waits for a command run on background to finish
Product: Red Hat Enterprise Linux 6 Reporter: Zdenek Pytela <zpytela>
Component: kshAssignee: Siteshwar Vashisht <svashisht>
Status: CLOSED WONTFIX QA Contact: BaseOS QE - Apps <qe-baseos-apps>
Severity: medium Docs Contact:
Priority: medium    
Version: 6.7CC: fkrska, kdudka, mhlavink, svashisht, thozza, tlavigne
Target Milestone: rcKeywords: Regression
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1419996 (view as bug list) Environment:
Last Closed: 2017-05-18 12:54:06 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1269194    
Attachments:
Description Flags
[PATCH] work in progress none

Description Zdenek Pytela 2016-05-06 13:48:44 UTC
Description of problem:
After updating the ksh package, one of the functions changed its behaviour: Under some circumstances, ksh waits for a command which has been set to run on background to finish as if it was on foreground.

Version-Release number of selected component (if applicable):
ksh-20120801-28.el6_7.3.x86_64

How reproducible:
always

Steps to Reproduce:
1. Run a simple reproducer script
=====
#!/bin/ksh
set -x
function SubJobReel
{
eval "sleep 30" >/dev/null &
echo $!
}

PID=$(SubJobReel)
echo "PID=$PID"
=====
2. yum downgrade ksh-20120801-10.el6.x86_64
3. Run the script again

Actual results:
different behaviour

Expected results:
the same behaviour

Additional info:
In bash any version the script works fine.

Comment 13 Kamil Dudka 2017-04-26 11:13:07 UTC
Let me summarize the bug.  The use of eval is unnecessary and has no influence on the bug.  The actual problem is that ksh waits for data produced on stdout of the background process despite stdout of the background process is redirected.

If the >/dev/null redirection was not used, ksh should indeed block on reading the output from the background process.  And that is exactly what other shells do.  The only problem is that ksh tries to read stdout of the background process when there is no (connected) descriptor to read from.

Comment 14 Kamil Dudka 2017-05-04 17:33:14 UTC
The following change makes this bug go away (and most likely (re)introduces another one):

--- a/src/cmd/ksh93/sh/xec.c
+++ b/src/cmd/ksh93/sh/xec.c
@@ -1638,8 +1638,6 @@ int sh_exec(register const Shnode_t *t, int flags)
                                        shp->comsub = 2;
                                sh_subtmpfile(shp);
                                shp->comsub = comsubsave;
-                               if(shp->comsub==1 && (!(shp->fdstatus[1]&IONOSEEK)))
-                                       unpipe = iousepipe(shp);

                                if((type&(FAMP|TFORK))==(FAMP|TFORK))
                                {

So I guess that the condition for running iousepipe() is incorrect but I have not understood the code properly yet.  shp->fdstatus[1] seems to be set to IOWRITE|IOSEEK, no matter if stdout of the background process is redirected or not.

The code causing us problems comes from ksh-20140415-hokaido.patch so it could be related to bug #1062296 and bug #1138751.

Comment 15 Kamil Dudka 2017-05-04 18:06:29 UTC
Created attachment 1276404 [details]
[PATCH] work in progress

Comment 16 Kamil Dudka 2017-05-04 18:17:17 UTC
Comment on attachment 1276404 [details]
[PATCH] work in progress

Ooops, I see some "standard error output lost with command substitution" errors in the upstream test-suite.  I need to have a closer look at them...

Comment 17 Kamil Dudka 2017-05-04 18:20:38 UTC
Comment on attachment 1276404 [details]
[PATCH] work in progress

I see those test-cases were already failing in the build of ksh-20120801-33.el6 but with a different error.  This will need to be further analyzed...

Comment 18 Kamil Dudka 2017-05-05 08:58:29 UTC
Comment on attachment 1276404 [details]
[PATCH] work in progress

Nope.  It is the other way around.  The proposed patch makes the upstream test suite pass again.  My previous testing was buggy because I had ksh-20120801-fd2lost.patch disabled by mistake.  Unfortunately, it only works for the `...` command substitution but not for $(...) as used in comment #0.

Comment 19 Kamil Dudka 2017-05-10 15:36:28 UTC
Comment on attachment 1276404 [details]
[PATCH] work in progress

The reason why this patch fixes the bug for the `...` syntax is that it reorders the conditions such that they are independent with each other.  Without the patch, the condition !(shp->fdstatus[1]&IONOSEEK) could be invalidated by calling iousepipe(shp), which would prevent sh_subfork() from being called.

Unfortunately, it does not fix the $(...) syntax because it uses a temporary file to transfer output data from the subshell unlike `...`, which uses a pipe.

If `...` is used, redirecting stdout to /dev/null in the background process closes the writing descriptor of the pipe, which unblocks read() from the pipe in the parent process.

If $(...) is used, a pipe is used just for synchronization but the actual data are transferred using a temporary file.  Redirecting redirecting stdout to /dev/null in the background process just closes the file but does not unblock read() from pipe in the parent process.  Consequently, the parent waits till the background process exits, which closes the writing descriptor of the pipe implicitly.