This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [RFC] Fix expect leaky pipes


On 29 May 2006 01:06, Christopher Faylor wrote:

> On Mon, May 29, 2006 at 12:00:05AM +0100, Dave Korn wrote:

>> int
>> fhandler_pty_master::close ()
>> {
>>  if (get_ttyp ()->master_alive ())
>>    fhandler_tty_common::close ();
>>  else
>>    {
>>      termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
>>      if (get_ttyp ()->from_master)
>> 	CloseHandle (get_ttyp ()->from_master);
>>      if (get_ttyp ()->to_master)
>> 	CloseHandle (get_ttyp ()->to_master);
>>      fhandler_tty_common::close ();
>>      if (!hExeced)
>> 	get_ttyp ()->init ();
>>    }
>>  return 0;
>> }
>> 
>> and I've changed that to
>> 
>> int
>> fhandler_pty_master::close ()
>> {
>>  if (get_ttyp ()->master_alive ())
>>    fhandler_tty_common::close ();
>>  /* That could have closed the last master, so re-check.  */
>>  if (!get_ttyp ()->master_alive ())
>>    {
>>      termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
>>      if (get_ttyp ()->from_master)
>> 	CloseHandle (get_ttyp ()->from_master);
>>      if (get_ttyp ()->to_master)
>> 	CloseHandle (get_ttyp ()->to_master);
>>      fhandler_tty_common::close ();
>>      if (!hExeced)
>> 	get_ttyp ()->init ();
>>    }
>>  return 0;
>> }
> 
> I think it should be ok to just do:
> 
>   fhandler_tty_common::close ();
> 
>   if (!get_ttyp ()->master_alive ())
>       {
> 	termios_printf ("freeing tty%d (%d)", get_unit (), get_ttyp ()->ntty);
> 	if (get_ttyp ()->from_master)
> 	 CloseHandle (get_ttyp ()->from_master);
> 	if (get_ttyp ()->to_master)
> 	 CloseHandle (get_ttyp ()->to_master);
>         ...
> 
> cgf

  Hmm, it looks nice and neat, but there's one possibility that worries me: perhaps the ordering in the else clause is deliberate to
avoid some potential race or error condition?  I haven't been able to see it yet, but the ChangeLog says

2005-08-17  Pavel Tsekov  <ptsekov@gmx.PCMM!>

	* fhandler_tty.cc (fhandler_tty_common::close): Rearrange the code so
	that the master end of the input and output pipes is closed before
	signalling an EOF event to the slave.
	(fhandler_pty_master::close): Likewise.

which kind of suggests it's important.  And in the way I've rewritten it, that's not going to happen: the EOF will be sent first.
Perhaps what Pavel really wanted to write was "if (one last reference remaining)" rather than "if (already closed)".  Ah: gottit:

http://cygwin.com/ml/cygwin-patches/2005-q3/msg00081.html 

  Yep.  So it's vital that the master pipes are closed before the 'input available' event is set so that any slave still alive
encounters an EOF on the pipe (rather than just 'no data') and translates it to SIGHUP; as it stands, my patch would simply
reintroduce the race.  Rightyo, but we can't know if we need to close them or not until we know if closing the inuse handle deletes
the master-alive event owing to it being the last open handle to it.  So I guess we'll just have to pass the extra handles into
common::close and teach it to close them if the inuse goes away.

  I'll prepare a revised patch on those lines unless anyone suggests a neater way to do it (hmmm suppose I could split common::close
up into a pre- and post- and wrap them around the code that needs to go in the middle... or I could define a callback hook passed
into common::close that gets called before eof is sent... or ...?)


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]