fix latent bug in process.c

fix latent bug in process.c

[Tom Tromey]

While looking into making process.c thread-friendly, I found an oddity
in process.c.  It looks like a latent bug to me, but I was hoping that
somebody more familiar with the code could take a look.

The appended patch shows the issue.

First, we unconditionally set Writeok, even if SELECT_CANT_DO_WRITE_MASK
is set.  It seems to me that in one branch we ought to clear Writeok;
and only set it in the other.

Second, and more importantly, in the loop after the select completes, we
check if an fd is in write_mask.  But, I think this will always be true
for any fd for which add_write_fd has been called -- meaning that the
callback will be called even if the fd is not in fact available for
writing.

The fix here is to check Writeok rather than write_mask (and this is
what necessitated the first change -- to ensure Writeok is always set
correctly).

Comments?

Tom

    fix latent bug in process.c
    
    	* process.c (wait_reading_process_output): Handle Writeok
    	depending on SELECT_CANT_DO_WRITE_MASK.  Check Writeok bits,
    	not write_mask.

diff --git a/src/process.c b/src/process.c
index 0be624a..278d143 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4522,11 +4522,12 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	    Available = non_keyboard_wait_mask;
 	  else
 	    Available = input_wait_mask;
-          Writeok = write_mask;
 #ifdef SELECT_CANT_DO_WRITE_MASK
           check_write = 0;
+	  FD_ZERO (&Writeok);
 #else
           check_write = 1;
+          Writeok = write_mask;
 #endif
  	  check_delay = wait_channel >= 0 ? 0 : process_output_delay_count;
 	}
@@ -4792,7 +4793,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
               && d->func != 0
               && (d->condition & FOR_READ) != 0)
             d->func (channel, d->data, 1);
-          if (FD_ISSET (channel, &write_mask)
+          if (FD_ISSET (channel, &Writeok)
               && d->func != 0
               && (d->condition & FOR_WRITE) != 0)
             d->func (channel, d->data, 0);

Re: fix latent bug in process.c

[Andreas Schwab]

Tom Tromey <tromey@redhat.com> writes:

> First, we unconditionally set Writeok, even if SELECT_CANT_DO_WRITE_MASK
> is set.  It seems to me that in one branch we ought to clear Writeok;
> and only set it in the other.

It doesn't matter, since if SELECT_CANT_DO_WRITE_MASK is defined
write_mask will always be empty anyway (w32 does not support dbus).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: fix latent bug in process.c

[Tom Tromey]

>>>>> "Andreas" == Andreas Schwab <schwab@linux-m68k.org> writes:

Andreas> Tom Tromey <tromey@redhat.com> writes:
>> First, we unconditionally set Writeok, even if SELECT_CANT_DO_WRITE_MASK
>> is set.  It seems to me that in one branch we ought to clear Writeok;
>> and only set it in the other.

Andreas> It doesn't matter, since if SELECT_CANT_DO_WRITE_MASK is defined
Andreas> write_mask will always be empty anyway (w32 does not support dbus).

I suppose so, but I think it is more future-proof this way.

The second part is really the interesting one though.

Tom

Re: fix latent bug in process.c

[Andreas Schwab]

Tom Tromey <tromey@redhat.com> writes:

> The second part is really the interesting one though.

If select can't figure out whether an fd is writable, it must be assumed
to be always writable, otherwise it will never be written to.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: fix latent bug in process.c

[Michael Albinus]

Andreas Schwab <schwab@linux-m68k.org> writes:

> (w32 does not support dbus).

"Yet". There exists native D-Bus for Windows, and it could happen, that
somebody will improve dbusbind.c for that case.

(For the records, I won't).

> Andreas.

Best regards, Michael.

Re: fix latent bug in process.c

[Jan Djärv]

Hello. 

Doesn't async connect also use write mask on W32?

    Jan D.

14 aug 2012 kl. 08:05 skrev Michael Albinus <michael.albinus@gmx.de>:

> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
>> (w32 does not support dbus).
> 
> "Yet". There exists native D-Bus for Windows, and it could happen, that
> somebody will improve dbusbind.c for that case.
> 
> (For the records, I won't).
> 
>> Andreas.
> 
> Best regards, Michael.

Re: fix latent bug in process.c

[Tom Tromey]

>>>>> "Andreas" == Andreas Schwab <schwab@linux-m68k.org> writes:

Andreas> Tom Tromey <tromey@redhat.com> writes:
>> The second part is really the interesting one though.

Andreas> If select can't figure out whether an fd is writable, it must
Andreas> be assumed to be always writable, otherwise it will never be
Andreas> written to.

Ok, thanks.  That makes the first bit make sense.
However, I still don't understand why we'd bother selecting for write
and then ignore the results.  If we're going to do this it seems like we
might as well not pass these bits to select at all.

What do you think of the appended instead?
This should preserve the current behavior if SELECT_CANT_DO_WRITE_MASK;
but properly check the results from select elsewhere.

Tom

    	* process.c (wait_reading_process_output): Check Writeok bits,
    	not write_mask.

diff --git a/src/process.c b/src/process.c
index 0be624a..18775c5 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4792,7 +4792,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
               && d->func != 0
               && (d->condition & FOR_READ) != 0)
             d->func (channel, d->data, 1);
-          if (FD_ISSET (channel, &write_mask)
+          if (FD_ISSET (channel, &Writeok)
               && d->func != 0
               && (d->condition & FOR_WRITE) != 0)
             d->func (channel, d->data, 0);

Re: fix latent bug in process.c

[Andreas Schwab]

Tom Tromey <tromey@redhat.com> writes:

>     	* process.c (wait_reading_process_output): Check Writeok bits,
>     	not write_mask.
>
> diff --git a/src/process.c b/src/process.c
> index 0be624a..18775c5 100644
> --- a/src/process.c
> +++ b/src/process.c
> @@ -4792,7 +4792,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
>                && d->func != 0
>                && (d->condition & FOR_READ) != 0)
>              d->func (channel, d->data, 1);
> -          if (FD_ISSET (channel, &write_mask)
> +          if (FD_ISSET (channel, &Writeok)

Yes, that looks like a real bug.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."