[PATCH BlueZ] shared/io-mainloop: Try to write to the pipe only if there are readers

[PATCH BlueZ] shared/io-mainloop: Try to write to the pipe only if there are readers

[Gal Ben-Haim]

bluetoothd receives a SIGPIPE and terminates if writing to a pipe that
was acquired by AcquireNotify and there are no readers. it can be
reproduced by terminating the reader process without closing the reader
end of the pipe.

poll is used to check for a POLLRDHUP event before write and an EPIPE
error is returned if the peer closed its end of the pipe.
---
 src/shared/io-mainloop.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
index 2306c3479..4a70449fc 100644
--- a/src/shared/io-mainloop.c
+++ b/src/shared/io-mainloop.c
@@ -27,6 +27,7 @@
 
 #include <unistd.h>
 #include <errno.h>
+#include <poll.h>
 #include <sys/socket.h>
 
 #include "src/shared/mainloop.h"
@@ -305,7 +306,14 @@ ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
 	if (!io || io->fd < 0)
 		return -ENOTCONN;
 
+	struct pollfd fds[1];
+	fds[0].fd = io->fd;
+	fds[0].events = POLLRDHUP;
 	do {
+		if (poll(fds, 1, 0) > 0 && fds[0].revents & POLLRDHUP > 0) {
+			return -EPIPE;
+		}
+
 		ret = writev(io->fd, iov, iovcnt);
 	} while (ret < 0 && errno == EINTR);
 
-- 
2.19.1

Re: [PATCH BlueZ] shared/io-mainloop: Try to write to the pipe only if there are readers

[Luiz Augusto von Dentz]

Hi Gal,
On Thu, Nov 15, 2018 at 1:55 PM Gal Ben-Haim <gbenhaim@augury.com> wrote:
>
> bluetoothd receives a SIGPIPE and terminates if writing to a pipe that
> was acquired by AcquireNotify and there are no readers. it can be
> reproduced by terminating the reader process without closing the reader
> end of the pipe.
>
> poll is used to check for a POLLRDHUP event before write and an EPIPE
> error is returned if the peer closed its end of the pipe.
> ---
>  src/shared/io-mainloop.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
> index 2306c3479..4a70449fc 100644
> --- a/src/shared/io-mainloop.c
> +++ b/src/shared/io-mainloop.c
> @@ -27,6 +27,7 @@
>
>  #include <unistd.h>
>  #include <errno.h>
> +#include <poll.h>
>  #include <sys/socket.h>
>
>  #include "src/shared/mainloop.h"
> @@ -305,7 +306,14 @@ ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
>         if (!io || io->fd < 0)
>                 return -ENOTCONN;
>
> +       struct pollfd fds[1];
> +       fds[0].fd = io->fd;
> +       fds[0].events = POLLRDHUP;
>         do {
> +               if (poll(fds, 1, 0) > 0 && fds[0].revents & POLLRDHUP > 0) {
> +                       return -EPIPE;
> +               }
> +
>                 ret = writev(io->fd, iov, iovcnt);
>         } while (ret < 0 && errno == EINTR);
>
> --
> 2.19.1

We normally do it a bit differently, we setup a write handler using
io_set_write_handler which informs when the fd is writable, otherwise
we maybe polling twice here since the user of the io may already be
using that mechanism. That said you are actually checking for HUP
which is something done io_set_disconnect_handler, which is already in
use. I suspect to really get this done cleanly we should perhaps use
MSG_NOSIGNAL when trying to write.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] shared/io-mainloop: Try to write to the pipe only if there are readers

[Gal Ben Haim]

doesn't my first patch of ignoring SIGPIPE simplify this? errno of the
write op will be EPIPE..
I can move it to main.c
On Thu, Nov 15, 2018 at 2:38 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Gal,
> On Thu, Nov 15, 2018 at 1:55 PM Gal Ben-Haim <gbenhaim@augury.com> wrote:
> >
> > bluetoothd receives a SIGPIPE and terminates if writing to a pipe that
> > was acquired by AcquireNotify and there are no readers. it can be
> > reproduced by terminating the reader process without closing the reader
> > end of the pipe.
> >
> > poll is used to check for a POLLRDHUP event before write and an EPIPE
> > error is returned if the peer closed its end of the pipe.
> > ---
> >  src/shared/io-mainloop.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/shared/io-mainloop.c b/src/shared/io-mainloop.c
> > index 2306c3479..4a70449fc 100644
> > --- a/src/shared/io-mainloop.c
> > +++ b/src/shared/io-mainloop.c
> > @@ -27,6 +27,7 @@
> >
> >  #include <unistd.h>
> >  #include <errno.h>
> > +#include <poll.h>
> >  #include <sys/socket.h>
> >
> >  #include "src/shared/mainloop.h"
> > @@ -305,7 +306,14 @@ ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> >         if (!io || io->fd < 0)
> >                 return -ENOTCONN;
> >
> > +       struct pollfd fds[1];
> > +       fds[0].fd = io->fd;
> > +       fds[0].events = POLLRDHUP;
> >         do {
> > +               if (poll(fds, 1, 0) > 0 && fds[0].revents & POLLRDHUP > 0) {
> > +                       return -EPIPE;
> > +               }
> > +
> >                 ret = writev(io->fd, iov, iovcnt);
> >         } while (ret < 0 && errno == EINTR);
> >
> > --
> > 2.19.1
>
> We normally do it a bit differently, we setup a write handler using
> io_set_write_handler which informs when the fd is writable, otherwise
> we maybe polling twice here since the user of the io may already be
> using that mechanism. That said you are actually checking for HUP
> which is something done io_set_disconnect_handler, which is already in
> use. I suspect to really get this done cleanly we should perhaps use
> MSG_NOSIGNAL when trying to write.
>
> --
> Luiz Augusto von Dentz