[PATCH BlueZ] gatt: Ignore SIGPIPE

[PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal]

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.

Ignoring the SIGPIPE will cause the write operation to return a
"io_send: Broken pipe" error which will be logged.
---
 src/gatt-client.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 234f46ed7..236f38ad5 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1137,6 +1137,9 @@ static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,
 	if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
 		return btd_error_failed(msg, strerror(errno));
 
+	/* Ignore SIGPIPE, a broken pipe error will be returned if the pipe has no readers */
+	signal(SIGPIPE, SIG_IGN);
+
 	dir = dbus_message_has_member(msg, "AcquireWrite");
 
 	io = io_new(pipefd[!dir]);
-- 
2.19.1

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Luiz Augusto von Dentz]

Hi,
On Thu, Nov 15, 2018 at 10:41 AM Gal <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.
>
> Ignoring the SIGPIPE will cause the write operation to return a
> "io_send: Broken pipe" error which will be logged.
> ---
>  src/gatt-client.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 234f46ed7..236f38ad5 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -1137,6 +1137,9 @@ static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,
>         if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
>                 return btd_error_failed(msg, strerror(errno));
>
> +       /* Ignore SIGPIPE, a broken pipe error will be returned if the pipe has no readers */
> +       signal(SIGPIPE, SIG_IGN);
> +
>         dir = dbus_message_has_member(msg, "AcquireWrite");
>
>         io = io_new(pipefd[!dir]);
> --
> 2.19.1

I wonder if there is a way to detect that on bluetoothd actually, that
way even if the client don't set it properly it would not make
bluetoothd to exit.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal Ben Haim]

io_send can use poll and check for POLLOUT and POLLRDHUP before
writing to the pipe

On Thu, Nov 15, 2018 at 12:12 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
> On Thu, Nov 15, 2018 at 10:41 AM Gal <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.
> >
> > Ignoring the SIGPIPE will cause the write operation to return a
> > "io_send: Broken pipe" error which will be logged.
> > ---
> >  src/gatt-client.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > index 234f46ed7..236f38ad5 100644
> > --- a/src/gatt-client.c
> > +++ b/src/gatt-client.c
> > @@ -1137,6 +1137,9 @@ static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,
> >         if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
> >                 return btd_error_failed(msg, strerror(errno));
> >
> > +       /* Ignore SIGPIPE, a broken pipe error will be returned if the pipe has no readers */
> > +       signal(SIGPIPE, SIG_IGN);
> > +
> >         dir = dbus_message_has_member(msg, "AcquireWrite");
> >
> >         io = io_new(pipefd[!dir]);
> > --
> > 2.19.1
>
> I wonder if there is a way to detect that on bluetoothd actually, that
> way even if the client don't set it properly it would not make
> bluetoothd to exit.
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Marcel Holtmann]

Hi Gal,

> io_send can use poll and check for POLLOUT and POLLRDHUP before
> writing to the pipe

that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.

Regards

Marcel

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal Ben Haim]

I can try to create another patch for it

On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Gal,
>
> > io_send can use poll and check for POLLOUT and POLLRDHUP before
> > writing to the pipe
>
> that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.
>
> Regards
>
> Marcel
>

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal Ben Haim]

I submitted a different patch -
https://marc.info/?l=linux-bluetooth&m=154228281730667&w=2
On Thu, Nov 15, 2018 at 12:20 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
>
> I can try to create another patch for it
>
> On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Gal,
> >
> > > io_send can use poll and check for POLLOUT and POLLRDHUP before
> > > writing to the pipe
> >
> > that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.
> >
> > Regards
> >
> > Marcel
> >

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Emil Lenngren]

Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <gbenhaim@augury.com>:
>
> I submitted a different patch -
> https://marc.info/?l=linux-bluetooth&m=154228281730667&w=2
> On Thu, Nov 15, 2018 at 12:20 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> >
> > I can try to create another patch for it
> >
> > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > >
> > > Hi Gal,
> > >
> > > > io_send can use poll and check for POLLOUT and POLLRDHUP before
> > > > writing to the pipe
> > >
> > > that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.
> > >
> > > Regards
> > >
> > > Marcel
> > >

Even if you first check for writability with poll, you still need to
also ignore the SIGPIPE signal, since the connection could be reset
after the check is made but before the write is executed, I guess. But
the "signal(SIGPIPE, SIG_IGN)" should usually be made once at
application initialization.

/Emil

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal Ben Haim]

should it be added to setup_signalfd in main.c ?

On Thu, Nov 15, 2018 at 2:56 PM Emil Lenngren <emil.lenngren@gmail.com> wrote:
>
> Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <gbenhaim@augury.com>:
> >
> > I submitted a different patch -
> > https://marc.info/?l=linux-bluetooth&m=154228281730667&w=2
> > On Thu, Nov 15, 2018 at 12:20 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > >
> > > I can try to create another patch for it
> > >
> > > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > > >
> > > > Hi Gal,
> > > >
> > > > > io_send can use poll and check for POLLOUT and POLLRDHUP before
> > > > > writing to the pipe
> > > >
> > > > that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.
> > > >
> > > > Regards
> > > >
> > > > Marcel
> > > >
>
> Even if you first check for writability with poll, you still need to
> also ignore the SIGPIPE signal, since the connection could be reset
> after the check is made but before the write is executed, I guess. But
> the "signal(SIGPIPE, SIG_IGN)" should usually be made once at
> application initialization.
>
> /Emil

Re: [PATCH BlueZ] gatt: Ignore SIGPIPE

[Gal Ben Haim]

I submitted another patch that does it in src/main.c -
https://marc.info/?l=linux-bluetooth&m=154239570400752&w=2
On Thu, Nov 15, 2018 at 5:43 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
>
> should it be added to setup_signalfd in main.c ?
>
> On Thu, Nov 15, 2018 at 2:56 PM Emil Lenngren <emil.lenngren@gmail.com> wrote:
> >
> > Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <gbenhaim@augury.com>:
> > >
> > > I submitted a different patch -
> > > https://marc.info/?l=linux-bluetooth&m=154228281730667&w=2
> > > On Thu, Nov 15, 2018 at 12:20 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > > >
> > > > I can try to create another patch for it
> > > >
> > > > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > > > >
> > > > > Hi Gal,
> > > > >
> > > > > > io_send can use poll and check for POLLOUT and POLLRDHUP before
> > > > > > writing to the pipe
> > > > >
> > > > > that should be done anyway since code that writes to a pipe descriptor (or socket for that matter) without checking that it is still valid is broken code.
> > > > >
> > > > > Regards
> > > > >
> > > > > Marcel
> > > > >
> >
> > Even if you first check for writability with poll, you still need to
> > also ignore the SIGPIPE signal, since the connection could be reset
> > after the check is made but before the write is executed, I guess. But
> > the "signal(SIGPIPE, SIG_IGN)" should usually be made once at
> > application initialization.
> >
> > /Emil