zsh-workers
 help / color / Atom feed
* PATCH: bug fix: infinite loop in sysread
@ 2020-02-05 14:19 Roman Perepelitsa
  2020-02-05 20:55 ` Bart Schaefer
  2020-02-06 19:52 ` dana
  0 siblings, 2 replies; 7+ messages in thread
From: Roman Perepelitsa @ 2020-02-05 14:19 UTC (permalink / raw)
  To: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

The attached patch fixes a bug in sysread from zsh/system. The bug
triggers in the following case:

1. zsh has been compiled with HAVE_SELECT and without HAVE_POLL
2. sysread is called with timeout (-t)
3. the input file descriptor is valid but there is no data to read
4. errno happens to be EINTR prior to the call to sysread

This results in an infinite loop in sysread:

  while ((ret = select(infd+1, (SELECT_ARG_2_T) &fds,
                       NULL, NULL,&select_tv)) < 1) {
      if (errno != EINTR || errflag || retflag || breaks || contflag)
          break;
  }

Here select() keeps returning 0, indicating timeout. This is not an
error, so errno doesn't get set. If it was EINTR prior to the call,
it stays EINTR, and the loop keeps spinning.

The fix is to replace `< 1` with `< 0` in the loop condition.

On GitHub:
https://github.com/zsh-users/zsh/compare/master...romkatv:fix-sysread-tmout

Roman.

[-- Attachment #2: fix-sysread-tmout.patch.txt --]
[-- Type: text/plain, Size: 456 bytes --]

diff --git a/Src/Modules/system.c b/Src/Modules/system.c
index 50de59cf9..fb3d80773 100644
--- a/Src/Modules/system.c
+++ b/Src/Modules/system.c
@@ -174,7 +174,7 @@ bin_sysread(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	while ((ret = select(infd+1, (SELECT_ARG_2_T) &fds,
-			     NULL, NULL,&select_tv)) < 1) {
+			     NULL, NULL,&select_tv)) < 0) {
 	    if (errno != EINTR || errflag || retflag || breaks || contflag)
 		break;
 	}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-05 14:19 PATCH: bug fix: infinite loop in sysread Roman Perepelitsa
@ 2020-02-05 20:55 ` Bart Schaefer
  2020-02-05 21:04   ` Roman Perepelitsa
  2020-02-06 19:52 ` dana
  1 sibling, 1 reply; 7+ messages in thread
From: Bart Schaefer @ 2020-02-05 20:55 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

On Wed, Feb 5, 2020 at 6:20 AM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> Here select() keeps returning 0, indicating timeout. This is not an
> error, so errno doesn't get set. If it was EINTR prior to the call,
> it stays EINTR, and the loop keeps spinning.
>
> The fix is to replace `< 1` with `< 0` in the loop condition.

Wouldn't a better fix be to set errno = 0 before the call?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-05 20:55 ` Bart Schaefer
@ 2020-02-05 21:04   ` Roman Perepelitsa
  0 siblings, 0 replies; 7+ messages in thread
From: Roman Perepelitsa @ 2020-02-05 21:04 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

On Wed, Feb 5, 2020 at 9:55 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Wed, Feb 5, 2020 at 6:20 AM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > Here select() keeps returning 0, indicating timeout. This is not an
> > error, so errno doesn't get set. If it was EINTR prior to the call,
> > it stays EINTR, and the loop keeps spinning.
> >
> > The fix is to replace `< 1` with `< 0` in the loop condition.
>
> Wouldn't a better fix be to set errno = 0 before the call?

That would be incorrect. select() returns -1 on error and in general
functions must set errno on error and may set it on success. When
select() returns 0, errno can be anything.

Before sending this patch I've also checked if there are similar bugs
in other places. There are none. The result of select() is always
tested with `< 0` everywhere in Zsh expect this one place.

The function I'm modifying has a very similar branch that calls poll()
instead of select. That one compares with `< 0`.

Some quotes:

From `man select`:

       On success, select() and pselect() return the number of file
       descriptors  contained in the three returned descriptor sets
       (that is, the total number of bits that are set in  readfds,
       writefds,  exceptfds)  which  may  be  zero  if  the timeout
       expires before anything interesting happens.  On  error,  -1
       is  returned,  and  errno  is set to indicate the error; the
       file descriptor sets are  unmodified,  and  timeout  becomes
       undefined.

From `man errno`:

       The value in errno is significant only when the return value
       of the call indicated an error (i.e., -1  from  most  system
       calls;  -1  or NULL from most library functions); a function
       that succeeds is allowed to  change  errno.   The  value  of
       errno  is  never  set  to zero by any system call or library
       function.

Roman.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-05 14:19 PATCH: bug fix: infinite loop in sysread Roman Perepelitsa
  2020-02-05 20:55 ` Bart Schaefer
@ 2020-02-06 19:52 ` dana
  2020-02-06 20:01   ` Peter Stephenson
  2020-02-07 11:26   ` Roman Perepelitsa
  1 sibling, 2 replies; 7+ messages in thread
From: dana @ 2020-02-06 19:52 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

On 5 Feb 2020, at 08:19, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
> The attached patch fixes a bug in sysread from zsh/system.

I've merged this because it makes sense to me and i want to do another 5.8
test release. If there are any issues we can back it out obv. Thanks

dana


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-06 19:52 ` dana
@ 2020-02-06 20:01   ` Peter Stephenson
  2020-02-07 11:26   ` Roman Perepelitsa
  1 sibling, 0 replies; 7+ messages in thread
From: Peter Stephenson @ 2020-02-06 20:01 UTC (permalink / raw)
  To: zsh-workers

On Thu, 2020-02-06 at 13:52 -0600, dana wrote:
> On 5 Feb 2020, at 08:19, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
> > The attached patch fixes a bug in sysread from zsh/system.
> 
> I've merged this because it makes sense to me and i want to do another 5.8
> test release. If there are any issues we can back it out obv. Thanks

It's hard to see how it could be wrong.  0 does mean a timeout,
so no point looping, and errno is irrelevant.

pws



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-06 19:52 ` dana
  2020-02-06 20:01   ` Peter Stephenson
@ 2020-02-07 11:26   ` Roman Perepelitsa
  2020-02-07 16:49     ` dana
  1 sibling, 1 reply; 7+ messages in thread
From: Roman Perepelitsa @ 2020-02-07 11:26 UTC (permalink / raw)
  To: dana; +Cc: Zsh hackers list

On Thu, Feb 6, 2020 at 8:52 PM dana <dana@dana.is> wrote:
>
> On 5 Feb 2020, at 08:19, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
> > The attached patch fixes a bug in sysread from zsh/system.
>
> I've merged this because it makes sense to me and i want to do another 5.8
> test release. If there are any issues we can back it out obv. Thanks

Thanks, dana.

FYI: I have commit rights. I'm new here, so I would need someone to
give me a go ahead before I merge anything into master. Your merging
my patch is more convenient for me of course (less work). You can push
this work to me by telling me to merge.

Roman.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: PATCH: bug fix: infinite loop in sysread
  2020-02-07 11:26   ` Roman Perepelitsa
@ 2020-02-07 16:49     ` dana
  0 siblings, 0 replies; 7+ messages in thread
From: dana @ 2020-02-07 16:49 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

On 7 Feb 2020, at 05:26, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
> FYI: I have commit rights. I'm new here, so I would need someone to
> give me a go ahead before I merge anything into master. Your merging
> my patch is more convenient for me of course (less work). You can push
> this work to me by telling me to merge.

Oh, i didn't realise. That works though, thanks

dana


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 14:19 PATCH: bug fix: infinite loop in sysread Roman Perepelitsa
2020-02-05 20:55 ` Bart Schaefer
2020-02-05 21:04   ` Roman Perepelitsa
2020-02-06 19:52 ` dana
2020-02-06 20:01   ` Peter Stephenson
2020-02-07 11:26   ` Roman Perepelitsa
2020-02-07 16:49     ` dana

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git