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
Hi,
On Thu, Nov 15, 2018 at 10:41 AM Gal <[email protected]> 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
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
<[email protected]> wrote:
>
> Hi,
> On Thu, Nov 15, 2018 at 10:41 AM Gal <[email protected]> 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
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
I can try to create another patch for it
On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <[email protected]> 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
>
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 <[email protected]> wrote:
>
> I can try to create another patch for it
>
> On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <[email protected]> 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
> >
Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <[email protected]>:
>
> 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 <[email protected]> wrote:
> >
> > I can try to create another patch for it
> >
> > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <[email protected]> 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
should it be added to setup_signalfd in main.c ?
On Thu, Nov 15, 2018 at 2:56 PM Emil Lenngren <[email protected]> wrote:
>
> Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <[email protected]>:
> >
> > 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 <[email protected]> wrote:
> > >
> > > I can try to create another patch for it
> > >
> > > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <[email protected]> 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
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 <[email protected]> wrote:
>
> should it be added to setup_signalfd in main.c ?
>
> On Thu, Nov 15, 2018 at 2:56 PM Emil Lenngren <[email protected]> wrote:
> >
> > Den tors 15 nov. 2018 kl 12:55 skrev Gal Ben Haim <[email protected]>:
> > >
> > > 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 <[email protected]> wrote:
> > > >
> > > > I can try to create another patch for it
> > > >
> > > > On Thu, Nov 15, 2018 at 12:18 PM Marcel Holtmann <[email protected]> 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