bug#6074: 24.0.50; accept-process-output on listening sockets not incorruptible

bug#6074: 24.0.50; accept-process-output on listening sockets not incorruptible

[Helmut Eller]

With this code Emacs seems to be stuck in an endless loop and is not
incorruptible with C-g:

(let ((proc (make-network-process :name "foo" :server t :noquery t 
				  :family 'local :service "/tmp/foo.socket")))
  (accept-process-output proc))

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Pip Cet]

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

> With this code Emacs seems to be stuck in an endless loop and is not
> incorruptible with C-g:

> (let ((proc (make-network-process :name "foo" :server t :noquery t
>  :family 'local :service "/tmp/foo.socket")))
>   (accept-process-output proc))

On Linux, the problem appears to be that we don't abort this infloop
in process.c if a read () returns EINVAL:

          while (true)
        {
          int nread = read_process_output (proc, wait_proc->infd);
          if (nread < 0)
            {
              if (errno == EIO || would_block (errno))
            break;
            }
          else
            {
              if (got_some_output < nread)
            got_some_output = nread;
              if (nread == 0)
            break;
              read_some_bytes = true;
            }
        }

That seems problematic to me, since we might get non-EIO errors for
other reasons. I'm attaching a patch that appears to fix the issue.

[-- Attachment #2: 0001-Don-t-retry-reading-after-receiving-EINVAL-bug-6074.patch --]
[-- Type: text/x-patch, Size: 849 bytes --]

From ad294bb1e7ff7012dca3345a8ca2046e0f9dc8fb Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 22 Jul 2019 03:49:54 +0000
Subject: [PATCH] Don't retry reading after receiving EINVAL (bug#6074)

* src/process.c (wait_reading_process_output): Don't retry reading
from an fd after an unknown error.
---
 src/process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index abadabe77e..1311409274 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5277,7 +5277,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 		  int nread = read_process_output (proc, wait_proc->infd);
 		  if (nread < 0)
 		    {
-		      if (errno == EIO || would_block (errno))
+		      if (errno == EINTR)
+			continue;
+		      else
 			break;
 		    }
 		  else
-- 
2.22.0

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Mon, 22 Jul 2019 03:52:33 +0000
> 
> diff --git a/src/process.c b/src/process.c
> index abadabe77e..1311409274 100644
> --- a/src/process.c
> +++ b/src/process.c
> @@ -5277,7 +5277,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
>  		  int nread = read_process_output (proc, wait_proc->infd);
>  		  if (nread < 0)
>  		    {
> -		      if (errno == EIO || would_block (errno))
> +		      if (errno == EINTR)
> +			continue;
> +		      else
>  			break;
>  		    }
>  		  else

Isn't it better to simply call rarely_quit inside the loop?

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Pip Cet]

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

On Mon, Jul 22, 2019 at 2:26 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Mon, 22 Jul 2019 03:52:33 +0000
> >
> > diff --git a/src/process.c b/src/process.c
> > index abadabe77e..1311409274 100644
> > --- a/src/process.c
> > +++ b/src/process.c
> > @@ -5277,7 +5277,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
> >                 int nread = read_process_output (proc, wait_proc->infd);
> >                 if (nread < 0)
> >                   {
> > -                   if (errno == EIO || would_block (errno))
> > +                   if (errno == EINTR)
> > +                     continue;
> > +                   else
> >                       break;
> >                   }
> >                 else
>
> Isn't it better to simply call rarely_quit inside the loop?

Why would we try again after receiving EINVAL? I believe the right
behavior is to return immediately in this case, just as we do for EIO.

However, we should probably call rarely_quit inside the loop, anyway,
to catch the case of a kernel bug keeping us busy with EINTRs. How's
this?

[-- Attachment #2: 0001-Don-t-retry-reading-after-receiving-EINVAL-bug-6074.patch --]
[-- Type: text/x-patch, Size: 1060 bytes --]

From a8991ccc1e846aed9a8f1ba0ebcd2771b623f241 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 22 Jul 2019 03:49:54 +0000
Subject: [PATCH] Don't retry reading after receiving EINVAL (bug#6074)

* src/process.c (wait_reading_process_output): Don't retry reading
from an fd after an unknown error.
---
 src/process.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index abadabe77e..9104a392c8 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5270,14 +5270,16 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	  /* If data can be read from the process, do so until exhausted.  */
 	  if (wait_proc->infd >= 0)
 	    {
+	      unsigned int count = 0;
 	      XSETPROCESS (proc, wait_proc);
 
 	      while (true)
 		{
 		  int nread = read_process_output (proc, wait_proc->infd);
+		  rarely_quit (++count);
 		  if (nread < 0)
 		    {
-		      if (errno == EIO || would_block (errno))
+		      if (errno != EINTR)
 			break;
 		    }
 		  else
-- 
2.22.0

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

> Why would we try again after receiving EINVAL? I believe the right
> behavior is to return immediately in this case, just as we do for EIO.
>
> However, we should probably call rarely_quit inside the loop, anyway,
> to catch the case of a kernel bug keeping us busy with EINTRs. How's
> this?

[...]

>  	    {
> +	      unsigned int count = 0;
>  	      XSETPROCESS (proc, wait_proc);
>  
>  	      while (true)
>  		{
>  		  int nread = read_process_output (proc, wait_proc->infd);
> +		  rarely_quit (++count);
>  		  if (nread < 0)
>  		    {
> -		      if (errno == EIO || would_block (errno))
> +		      if (errno != EINTR)
>  			break;
>  		    }
>  		  else

There was no followup on this patch at the time (except Richard chiding
you for using forbidden terminology), but I tried the patch now, and it
seems to fix the issue, so I've applied it to the trunk now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > On Linux, the problem appears to be that

Those words lead me to think that you're running GNU Emacs as
part of the GNU operating system -- a version with Linx as the kernel.
One of the obstacles that we GNU developers have to face
is that people that use GNU don't know it is GNU.  They think
the system is "Linux".

Thanks for reporting a bug in one part of the GNU system.  While
you're at it, could you please call the system "GNU/Linux", so as to
help correct the misinformation of what it is, where it comes from,
and why we developed it?

See https://gnu.org/gnu/linux-and-gnu.html and
https://gnu.org/gnu/gnu-linux-faq.html, plus the history in
https://gnu.org/gnu/the-gnu-project.html.


-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Pip Cet]

On Tue, Jul 23, 2019 at 2:46 AM Richard Stallman <rms@gnu.org> wrote:
>   > On Linux, the problem appears to be that
>
> Those words lead me to think that you're running GNU Emacs as
> part of the GNU operating system -- a version with Linx as the kernel.

I am, indeed, but in this particular case, it is the Linux kernel
doing something problematic: returning -EINVAL, which glibc merely
stores in errno.  But I agree it would have been more accurate to say
something like "On this GNU/Linux system, the problem appears to be
that the Linux kernel..."

> Thanks for reporting a bug in one part of the GNU system.  While

(I didn't report it, I merely reproduced a very old bug report and
suggested a fix.)

> you're at it, could you please call the system "GNU/Linux", so as to
> help correct the misinformation of what it is, where it comes from,
> and why we developed it?

Thanks for taking the time to remind me of that.

bug#6074: accept-process-output on listening sockets cause non-interruptible infloop

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Happy hacking!

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)