Re: bin/54514: script(1) sometimes swallows last line(s) of output

[Harold Gutch <logix%foobar.franken.de@localhost>]

The following reply was made to PR bin/54514; it has been noted by GNATS.

From: Harold Gutch <logix%foobar.franken.de@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: bin/54514: script(1) sometimes swallows last line(s) of output
Date: Sun, 1 Sep 2019 21:42:52 +0200

 Hi,
 
 On Sat, Aug 31, 2019 at 02:00:02AM +0000, Robert Elz wrote:
 > The following reply was made to PR bin/54514; it has been noted by GNATS.
 > 
 > From: Robert Elz <kre%munnari.OZ.AU@localhost>
 > To: gnats-bugs%netbsd.org@localhost
 > Cc: 
 > Subject: Re: bin/54514: script(1) sometimes swallows last line(s) of output
 > Date: Sat, 31 Aug 2019 08:58:18 +0700
 > 
 >  I don't believe that is the correct fix.   Your first change
 >  (moving the signal(SIGCHLD, finish) later) opens a possible
 >  (if unlikely) race if the child exits very quickly (before the
 >  signal is established) - which would result in the parent
 >  hanging until it reads EOF from stdin (which for your tests would
 >  be essentially forever - until manually killed).
 
 Good catch, you're right - thanks!
 
 
 >  The correct way to handle this is to leave the initial signal() call
 >  catching SIGCHLD where it is currently, and insert
 >  	(void)signal(SIGCHLD, SIG_DFL);
 >  in the
 >  	if (child == 0) {
 >  case, before the subsequent (second) fork().   (An alternative would be
 >  to save the result of the initial signal call, and use that instead of
 >  SIG_DFL in the one to be added - but that should make no real difference.)
 >  
 >  Your second change is wrong (setting SIGCHLD to SIG_IGN is almost
 >  never the right thing to do), and in any case unnecessary, both
 >  because after your first change (or after the version of it I
 >  suggest above) when dooutput() is called the state of SIGCHLD will
 >  be SIG_DFL (which results in what I suspect you wanted to achieve
 >  by setting it to SIG_IGN) and also because dooutput() is called in the
 >  grandchild process, which never forks, so never has any children
 >  and so will never receive a SIGCHLD in any case.   This change can
 >  simply be deleted.
 
 Thanks for pointing this out.  I wasn't aware of the difference
 between SIG_IGN and SIG_DFL for the SIGCHILD case.
 
 
 >  Please try again with the changes as suggested, and let us know
 >  if my suggested variation for your patch fixes the problem.
 
 Yes, your suggestion fixes the problem for me.
 
 --- src/usr.bin/script/script.c	2011-09-06 18:29:56.000000000 +0000
 +++ src/usr.bin/script/script.c	2019-08-31 12:41:26.368033644 +0000
 @@ -173,6 +173,7 @@
  		fail();
  	}
  	if (child == 0) {
 +		(void)signal(SIGCHLD, SIG_DFL);
  		subchild = child = fork();
  		if (child < 0) {
  			warn("fork");
 
 
 >  kre
 >  
 >  ps: this has most likely never been a problem, as script is almost
 >  always used interactively, and only exits when the user enters ^D
 >  to the terminal, which they typically don't do until all the output
 >  that is expected has been seen.   For the kind of usage in your test.
 >  the input is already known (it is in the test script) so isn't needed,
 >  to capture output simple shell redirection is enough.   I am not sure
 >  why anyone would ever normally run script as it is being run here.
 
 I actually use script(1) that way somewhat regularly if I want to log
 the output of a longer job.  The alternative would be to redirect stdout
 and stderr, open a second terminal and "tail -f" the log file (but
 very occasionally I also want timestamping via "script -t").  I find
 it simpler to run "script ./blah logfile".  And sure, I could do it in
 two steps, call script and then call whatever I want interactively.
 So yes, I really did stumble over this issue a while ago in a real
 situation.  And of course the test case is contrived and just
 illustrates the issue.
 
 
 Thanks for your input!
   Harold