[PATCH] af_unix: closed SOCK_SEQPACKET socketpair must get SIGPIPE

According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

I used the following program to check the kernel behavior:

/*****************/
#include <signal.h>
#include <stdio.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

void PipeHandler(int sig) {
  fprintf(stderr, "Killed by SIGPIPE\n");
  _exit(1);
}

int main(int argc, char *argv[]) {
  if (argc < 2) return 0;
  struct sigaction act;
  act.sa_handler = PipeHandler;
  sigaction(SIGPIPE, &act, NULL);
  int fds[2];
  if (strcmp(argv[1], "seqpacket") == 0) {
    if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) == -1) return -1;
  } else {
    if (strcmp(argv[1], "stream") == 0) {
      if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == -1) return -1;
    } else {
      return -1;
    }
  }
  int flags = 0;
  if ((argc > 2) && (strcmp(argv[2], "nosignal") == 0))
    flags = MSG_NOSIGNAL;

  struct msghdr msg;
  memset(&msg, 0, sizeof(msg));
  close(fds[0]);
  const ssize_t r = sendmsg(fds[1], &msg, flags);
  if (r == -1) perror("sendmsg");
  return 0;
}
/*****************/

Without the below patch the behavior is as follows:

$ ./sock seqpacket
sendmsg: Broken pipe
$ ./sock stream
Killed by SIGPIPE
$ ./sock seqpacket nosignal
sendmsg: Broken pipe
$ ./sock stream nosignal
sendmsg: Broken pipe

The behavior of the patched kernel complies with POSIX:

$  ./sock seqpacket
Killed by SIGPIPE
$ ./sock stream
Killed by SIGPIPE
$ ./sock seqpacket nosignal
sendmsg: Broken pipe
$ ./sock stream nosignal
sendmsg: Broken pipe


Signed-off-by: Alexander Potapenko <glider@google.com>
---
 net/unix/af_unix.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8fbe6d7..ba34c73 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -1645,6 +1645,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	struct sock *other = NULL;
 	int namelen = 0; /* fake GCC */
 	int err;
+	bool send_sigpipe = false;
 	unsigned int hash;
 	struct sk_buff *skb;
 	long timeo;

@@ -1675,6 +1676,12 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			goto out;
 	}
 
+	if (sk->sk_shutdown & SEND_SHUTDOWN && sock->type == SOCK_SEQPACKET) {
+		send_sigpipe = true;
+		err = -EPIPE;
+		goto out;
+	}
+
 	if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr
 	    && (err = unix_autobind(sock)) != 0)
 		goto out;

@@ -1769,8 +1776,11 @@ restart_locked:
 	}
 
 	err = -EPIPE;
-	if (other->sk_shutdown & RCV_SHUTDOWN)
+	if (other->sk_shutdown & RCV_SHUTDOWN) {
+		if (sock->type == SOCK_SEQPACKET)
+			send_sigpipe = true;
 		goto out_unlock;
+	}
 
 	if (sk->sk_type != SOCK_SEQPACKET) {
 		err = security_unix_may_send(sk->sk_socket, other->sk_socket);

@@ -1837,6 +1847,8 @@ out:
 	if (other)
 		sock_put(other);
 	scm_destroy(&scm);
+	if (send_sigpipe && !(msg->msg_flags & MSG_NOSIGNAL))
+		send_sig(SIGPIPE, current, 0);
 	return err;
 }
 

-- 
2.7.0.rc3.207.g0ac5344

On Tue, 2016-03-15 at 10:03 +0100, Alexander Potapenko wrote:

According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

I find this sentence slightly confusing ?

If MSG_NOSIGNAL is set, then SIGPIPE should not be generated.

s/with/without/ maybe ?

From: Alexander Potapenko

Sent: 15 March 2016 09:04
According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

...

Without the below patch the behavior is as follows:

$ ./sock seqpacket
sendmsg: Broken pipe

...

The behavior of the patched kernel complies with POSIX:

$  ./sock seqpacket
Killed by SIGPIPE

...

While POSIX might specify this behaviour, changing the behaviour
could easily break applications.
Basically this change (more or less) require every application that
uses SOCK_SEQPACKED to be audited to ensure that MSG_NOSIGNAL is set
on ever send/write to the socket.

Personally I think the whole SIGPIPE on sockets should never have been
allowed to get into the standard.
I don't remember MSG_NOSIGNAL being present in SYSV.

The only time you want a write into a pipe to generate SIGPIPE is
for pipes generates by the shell that feed stdout to stdin of the
next process in the pipeline.
If pipes are implemented as unix-domain socketpairs (no one does that
any more) then it would require the SIGPIPE for write().

	David

On Tue, Mar 15, 2016 at 2:20 PM, Eric Dumazet [off-list ref] wrote:

On Tue, 2016-03-15 at 10:03 +0100, Alexander Potapenko wrote:

quoted

According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

I find this sentence slightly confusing ?

If MSG_NOSIGNAL is set, then SIGPIPE should not be generated.

s/with/without/ maybe ?

Agreed. I've rewritten this a couple of times and failed to proof-read
it for the last time.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

On Tue, Mar 15, 2016 at 2:46 PM, David Laight [off-list ref] wrote:

From: Alexander Potapenko

quoted

Sent: 15 March 2016 09:04
According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

...

quoted

Without the below patch the behavior is as follows:

$ ./sock seqpacket
sendmsg: Broken pipe

...

quoted

The behavior of the patched kernel complies with POSIX:

$  ./sock seqpacket
Killed by SIGPIPE

...

While POSIX might specify this behaviour, changing the behaviour
could easily break applications.
Basically this change (more or less) require every application that
uses SOCK_SEQPACKED to be audited to ensure that MSG_NOSIGNAL is set
on ever send/write to the socket.

This is true, but the drawback of maintaining a non-standard behavior
is that people can't write portable code.
I couldn't find the exact place where the bug has been introduced, but
according to http://lxr.free-electrons.com/ SOCK_SEQPACKET sockets did
not respect MSG_NOSIGNAL from the very beginning
(http://lxr.free-electrons.com/source/net/unix/af_unix.c?v=3.8#L1723,
there was no such thing as SOCK_SEQPACKET in AF_UNIX in 2.4.37)
Unfortunately I don't know how to estimate the number of existing
users depending on this oddity, as well as the number of people that
have to work around it, so leaving it up to maintainers to decide
whether the fix is needed.

Personally I think the whole SIGPIPE on sockets should never have been
allowed to get into the standard.
I don't remember MSG_NOSIGNAL being present in SYSV.

I think it was not.

The only time you want a write into a pipe to generate SIGPIPE is
for pipes generates by the shell that feed stdout to stdin of the
next process in the pipeline.
If pipes are implemented as unix-domain socketpairs (no one does that
any more) then it would require the SIGPIPE for write().

        David

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

From: Alexander Potapenko <glider@google.com>
Date: Tue, 15 Mar 2016 10:03:44 +0100

According to IEEE Std 1003.1, 2013, sending data to a SOCK_SEQPACKET
socketpair with MSG_NOSIGNAL flag set must result in a SIGPIPE if the
socket is no longer connected.

I used the following program to check the kernel behavior:

 ...

Signed-off-by: Alexander Potapenko <glider@google.com>

In the final analysis I don't think we can do this.

Properly working programs will start to mysteriously receive
signals in this situation and fail.

I'm sorry but I think we're stuck with this behavior.