headers
Anders Kaseorg | 26 Jun 2012 14:04
Picon
Favicon

[PATCH] fifo: Do not restart open() if it already found a partner

If a parent and child process open the two ends of a fifo, and the
child immediately exits, the parent may receive a SIGCHLD before its
open() returns.  In that case, we need to make sure that open() will
return successfully after the SIGCHLD handler returns, instead of
throwing EINTR or being restarted.  Otherwise, the restarted open()
would incorrectly wait for a second partner on the other end.

The following test demonstrates the EINTR that was wrongly thrown from
the parent’s open().  Change .sa_flags = 0 to .sa_flags = SA_RESTART
to see a deadlock instead, in which the restarted open() waits for a
second reader that will never come.

  #include <sys/stat.h>
  #include <sys/types.h>
  #include <sys/wait.h>
  #include <fcntl.h>
  #include <signal.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <unistd.h>

  #define CHECK(x) do {if ((x) == -1) {perror(#x); abort();}} while(0)

  void handler(int signum) {}

  int main()
  {
      struct sigaction act = {.sa_handler = handler, .sa_flags = 0};
      CHECK(sigaction(SIGCHLD, &act, NULL));
      CHECK(mknod("fifo", S_IFIFO | S_IRWXU, 0));
(Continue reading)

Permalink | Reply |
headers
Jonathan Nieder | 26 Jun 2012 22:20
Picon

Re: [PATCH] fifo: Do not restart open() if it already found a partner

Hi,

Anders Kaseorg wrote:

> The following test demonstrates the EINTR that was wrongly thrown from
> the parent’s open().  Change .sa_flags = 0 to .sa_flags = SA_RESTART
> to see a deadlock instead, in which the restarted open() waits for a
> second reader that will never come.

Nice.

To recap, reading a fifo without a writer (resp. when writing a fifo
without a reader), fifo_open() without O_NONBLOCK waits for the other
end to be opened:

	if (!pipe->writers) {
		if ((filp->f_flags & O_NONBLOCK)) {
			...
		} else {
			wait_for_partner(inode, &pipe->w_counter);

The wait_for_partner() function waits for the pipe to be opened.
It is interruptible.  Inlining a little for clarity[*]:

	int cur = pipe->w_counter;

	while (cur == pipe->w_counter) {
		DEFINE_WAIT(wait);

		prepare_to_wait(&pipe->wait, &wait, TASK_INTERRUPTIBLE);
(Continue reading)

Permalink | Reply |
headers
Anders Kaseorg | 26 Jun 2012 22:59
Picon
Favicon

[PATCH v2] fifo: Do not restart open() if it already found a partner

If a parent and child process open the two ends of a fifo, and the
child immediately exits, the parent may receive a SIGCHLD before its
open() returns.  In that case, we need to make sure that open() will
return successfully after the SIGCHLD handler returns, instead of
throwing EINTR or being restarted.  Otherwise, the restarted open()
would incorrectly wait for a second partner on the other end.

The following test demonstrates the EINTR that was wrongly thrown from
the parent’s open().  Change .sa_flags = 0 to .sa_flags = SA_RESTART
to see a deadlock instead, in which the restarted open() waits for a
second reader that will never come.  (On my systems, this happens
pretty reliably within about 5 to 500 iterations.  Others report that
it manages to loop ~forever sometimes; YMMV.)

  #include <sys/stat.h>
  #include <sys/types.h>
  #include <sys/wait.h>
  #include <fcntl.h>
  #include <signal.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <unistd.h>

  #define CHECK(x) do if ((x) == -1) {perror(#x); abort();} while(0)

  void handler(int signum) {}

  int main()
  {
      struct sigaction act = {.sa_handler = handler, .sa_flags = 0};
(Continue reading)

Permalink | Reply |
headers
Jonathan Nieder | 26 Jun 2012 23:58
Picon

Re: [PATCH v2] fifo: Do not restart open() if it already found a partner

Anders Kaseorg wrote:

> Signed-off-by: Anders Kaseorg <andersk <at> mit.edu>
> ---
> Changes from v1:
> • Change wait_for_partner return from true/false to 0/-ERESTARTSYS.
> • Mention that the demo program doesn’t always work for some.
> • Remove some unneeded braces from the demo program’s CHECK() macro.

Yep, I don't have anything left to complain about.  Looks obviously
correct to my inexpert eyes.

Reviewed-by: Jonathan Nieder <jrnieder <at> gmail.com>
Permalink | Reply |
headers
Anders Kaseorg | 15 Jul 2012 23:14
Picon
Favicon

[PATCH v2] fifo: Do not restart open() if it already found a partner

If a parent and child process open the two ends of a fifo, and the
child immediately exits, the parent may receive a SIGCHLD before its
open() returns.  In that case, we need to make sure that open() will
return successfully after the SIGCHLD handler returns, instead of
throwing EINTR or being restarted.  Otherwise, the restarted open()
would incorrectly wait for a second partner on the other end.

The following test demonstrates the EINTR that was wrongly thrown from
the parent’s open().  Change .sa_flags = 0 to .sa_flags = SA_RESTART
to see a deadlock instead, in which the restarted open() waits for a
second reader that will never come.  (On my systems, this happens
pretty reliably within about 5 to 500 iterations.  Others report that
it manages to loop ~forever sometimes; YMMV.)

  #include <sys/stat.h>
  #include <sys/types.h>
  #include <sys/wait.h>
  #include <fcntl.h>
  #include <signal.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <unistd.h>

  #define CHECK(x) do if ((x) == -1) {perror(#x); abort();} while(0)

  void handler(int signum) {}

  int main()
  {
      struct sigaction act = {.sa_handler = handler, .sa_flags = 0};
(Continue reading)

Permalink | Reply |

Gmane