[PATCH BlueZ] core: Ignore SIGPIPE

[PATCH BlueZ] core: Ignore SIGPIPE

[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.

Ignoring the SIGPIPE will cause the write operation to return an
error which will be logged as "io_send: Broken pipe".
---
 src/main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/main.c b/src/main.c
index 4716f5388..c62886593 100644
--- a/src/main.c
+++ b/src/main.c
@@ -691,7 +691,7 @@ int main(int argc, char *argv[])
 	uint16_t sdp_mtu = 0;
 	uint32_t sdp_flags = 0;
 	int gdbus_flags = 0;
-	guint signal, watchdog;
+	guint signal_source, watchdog;
 	const char *watchdog_usec;
 
 	init_defaults();
@@ -721,7 +721,11 @@ int main(int argc, char *argv[])
 
 	event_loop = g_main_loop_new(NULL, FALSE);
 
-	signal = setup_signalfd();
+	signal_source = setup_signalfd();
+
+	/* Ignore SIGPIPE, a broken pipe error will be returned from write 
+	 * attempts to a pipe with no readers */
+	signal(SIGPIPE, SIG_IGN);
 
 	__btd_log_init(option_debug, option_detach);
 
@@ -809,7 +813,7 @@ int main(int argc, char *argv[])
 
 	sd_notify(0, "STATUS=Quitting");
 
-	g_source_remove(signal);
+	g_source_remove(signal_source);
 
 	plugin_cleanup();
 
-- 
2.19.1

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Luiz Augusto von Dentz]

Hi Gal,

On Fri, Nov 16, 2018 at 9:17 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.
>
> Ignoring the SIGPIPE will cause the write operation to return an
> error which will be logged as "io_send: Broken pipe".

Ive been suggesting using MSG_NOSIGNAL, do you have anything against it?

MSG_NOSIGNAL (since Linux 2.2)Requests not to send SIGPIPE on errors
on stream oriented sockets when the other end breaks the connection.
The EPIPE error is still returned.
(https://linux.die.net/man/2/send)

> ---
>  src/main.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/src/main.c b/src/main.c
> index 4716f5388..c62886593 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
>         uint16_t sdp_mtu = 0;
>         uint32_t sdp_flags = 0;
>         int gdbus_flags = 0;
> -       guint signal, watchdog;
> +       guint signal_source, watchdog;
>         const char *watchdog_usec;
>
>         init_defaults();
> @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
>
>         event_loop = g_main_loop_new(NULL, FALSE);
>
> -       signal = setup_signalfd();
> +       signal_source = setup_signalfd();
> +
> +       /* Ignore SIGPIPE, a broken pipe error will be returned from write
> +        * attempts to a pipe with no readers */
> +       signal(SIGPIPE, SIG_IGN);
>
>         __btd_log_init(option_debug, option_detach);
>
> @@ -809,7 +813,7 @@ int main(int argc, char *argv[])
>
>         sd_notify(0, "STATUS=Quitting");
>
> -       g_source_remove(signal);
> +       g_source_remove(signal_source);
>
>         plugin_cleanup();
>
> --
> 2.19.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Gal Ben Haim]

there's a suggestion in the thread about my other patch that uses poll
that the signal should be ignored anyway,
to prevent a scenario that the peer disconnects between poll or
checking for MSG_NOSIGNAL and the write attempt.
On Fri, Nov 16, 2018 at 9:30 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Gal,
>
> On Fri, Nov 16, 2018 at 9:17 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.
> >
> > Ignoring the SIGPIPE will cause the write operation to return an
> > error which will be logged as "io_send: Broken pipe".
>
> Ive been suggesting using MSG_NOSIGNAL, do you have anything against it?
>
> MSG_NOSIGNAL (since Linux 2.2)Requests not to send SIGPIPE on errors
> on stream oriented sockets when the other end breaks the connection.
> The EPIPE error is still returned.
> (https://linux.die.net/man/2/send)
>
> > ---
> >  src/main.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/main.c b/src/main.c
> > index 4716f5388..c62886593 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> >         uint16_t sdp_mtu = 0;
> >         uint32_t sdp_flags = 0;
> >         int gdbus_flags = 0;
> > -       guint signal, watchdog;
> > +       guint signal_source, watchdog;
> >         const char *watchdog_usec;
> >
> >         init_defaults();
> > @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> >
> >         event_loop = g_main_loop_new(NULL, FALSE);
> >
> > -       signal = setup_signalfd();
> > +       signal_source = setup_signalfd();
> > +
> > +       /* Ignore SIGPIPE, a broken pipe error will be returned from write
> > +        * attempts to a pipe with no readers */
> > +       signal(SIGPIPE, SIG_IGN);
> >
> >         __btd_log_init(option_debug, option_detach);
> >
> > @@ -809,7 +813,7 @@ int main(int argc, char *argv[])
> >
> >         sd_notify(0, "STATUS=Quitting");
> >
> > -       g_source_remove(signal);
> > +       g_source_remove(signal_source);
> >
> >         plugin_cleanup();
> >
> > --
> > 2.19.1
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Marcel Holtmann]

Hi 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 an
> error which will be logged as "io_send: Broken pipe".
> ---
> src/main.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index 4716f5388..c62886593 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> 	uint16_t sdp_mtu = 0;
> 	uint32_t sdp_flags = 0;
> 	int gdbus_flags = 0;
> -	guint signal, watchdog;
> +	guint signal_source, watchdog;
> 	const char *watchdog_usec;
> 
> 	init_defaults();
> @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> 
> 	event_loop = g_main_loop_new(NULL, FALSE);
> 
> -	signal = setup_signalfd();
> +	signal_source = setup_signalfd();
> +
> +	/* Ignore SIGPIPE, a broken pipe error will be returned from write 
> +	 * attempts to a pipe with no readers */
> +	signal(SIGPIPE, SIG_IGN);

if we decide not to use MSG_NOSIGNAL, then everything has to go via signalfd and not just by hacking in signal(SIG_IGN).

Regards

Marcel

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Gal Ben Haim]

what should signal_handler do in case of SIGPIPE? will failed write
return EPIPE if the signal is catched?

On Fri, Nov 16, 2018 at 10:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi 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 an
> > error which will be logged as "io_send: Broken pipe".
> > ---
> > src/main.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/main.c b/src/main.c
> > index 4716f5388..c62886593 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> >       uint16_t sdp_mtu = 0;
> >       uint32_t sdp_flags = 0;
> >       int gdbus_flags = 0;
> > -     guint signal, watchdog;
> > +     guint signal_source, watchdog;
> >       const char *watchdog_usec;
> >
> >       init_defaults();
> > @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> >
> >       event_loop = g_main_loop_new(NULL, FALSE);
> >
> > -     signal = setup_signalfd();
> > +     signal_source = setup_signalfd();
> > +
> > +     /* Ignore SIGPIPE, a broken pipe error will be returned from write
> > +      * attempts to a pipe with no readers */
> > +     signal(SIGPIPE, SIG_IGN);
>
> if we decide not to use MSG_NOSIGNAL, then everything has to go via signalfd and not just by hacking in signal(SIG_IGN).
>
> Regards
>
> Marcel
>

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Gal Ben Haim]

I tried MSG_NOSIGNAL, see my other patch. the error is io_send: Socket
operation on non-socket.

please suggest an acceptable solution for this issue
On Fri, Nov 16, 2018 at 10:11 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
>
> what should signal_handler do in case of SIGPIPE? will failed write
> return EPIPE if the signal is catched?
>
> On Fri, Nov 16, 2018 at 10:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi 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 an
> > > error which will be logged as "io_send: Broken pipe".
> > > ---
> > > src/main.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/main.c b/src/main.c
> > > index 4716f5388..c62886593 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> > >       uint16_t sdp_mtu = 0;
> > >       uint32_t sdp_flags = 0;
> > >       int gdbus_flags = 0;
> > > -     guint signal, watchdog;
> > > +     guint signal_source, watchdog;
> > >       const char *watchdog_usec;
> > >
> > >       init_defaults();
> > > @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> > >
> > >       event_loop = g_main_loop_new(NULL, FALSE);
> > >
> > > -     signal = setup_signalfd();
> > > +     signal_source = setup_signalfd();
> > > +
> > > +     /* Ignore SIGPIPE, a broken pipe error will be returned from write
> > > +      * attempts to a pipe with no readers */
> > > +     signal(SIGPIPE, SIG_IGN);
> >
> > if we decide not to use MSG_NOSIGNAL, then everything has to go via signalfd and not just by hacking in signal(SIG_IGN).
> >
> > Regards
> >
> > Marcel
> >

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Luiz Augusto von Dentz]

Hi Gal,

Weird I thought send/sendmsg works with pipes, anyway I think we can
apply the handling of SIGPIPE with signalfd then.
On Sat, Nov 17, 2018 at 1:47 AM Gal Ben Haim <gbenhaim@augury.com> wrote:
>
> I tried MSG_NOSIGNAL, see my other patch. the error is io_send: Socket
> operation on non-socket.
>
> please suggest an acceptable solution for this issue
> On Fri, Nov 16, 2018 at 10:11 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> >
> > what should signal_handler do in case of SIGPIPE? will failed write
> > return EPIPE if the signal is catched?
> >
> > On Fri, Nov 16, 2018 at 10:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > >
> > > Hi 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 an
> > > > error which will be logged as "io_send: Broken pipe".
> > > > ---
> > > > src/main.c | 10 +++++++---
> > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/main.c b/src/main.c
> > > > index 4716f5388..c62886593 100644
> > > > --- a/src/main.c
> > > > +++ b/src/main.c
> > > > @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> > > >       uint16_t sdp_mtu = 0;
> > > >       uint32_t sdp_flags = 0;
> > > >       int gdbus_flags = 0;
> > > > -     guint signal, watchdog;
> > > > +     guint signal_source, watchdog;
> > > >       const char *watchdog_usec;
> > > >
> > > >       init_defaults();
> > > > @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> > > >
> > > >       event_loop = g_main_loop_new(NULL, FALSE);
> > > >
> > > > -     signal = setup_signalfd();
> > > > +     signal_source = setup_signalfd();
> > > > +
> > > > +     /* Ignore SIGPIPE, a broken pipe error will be returned from write
> > > > +      * attempts to a pipe with no readers */
> > > > +     signal(SIGPIPE, SIG_IGN);
> > >
> > > if we decide not to use MSG_NOSIGNAL, then everything has to go via signalfd and not just by hacking in signal(SIG_IGN).
> > >
> > > Regards
> > >
> > > Marcel
> > >



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] core: Ignore SIGPIPE

[Gal Ben Haim]

submitted - https://marc.info/?l=linux-bluetooth&m=154243443910013&w=2
On Sat, Nov 17, 2018 at 11:29 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Gal,
>
> Weird I thought send/sendmsg works with pipes, anyway I think we can
> apply the handling of SIGPIPE with signalfd then.
> On Sat, Nov 17, 2018 at 1:47 AM Gal Ben Haim <gbenhaim@augury.com> wrote:
> >
> > I tried MSG_NOSIGNAL, see my other patch. the error is io_send: Socket
> > operation on non-socket.
> >
> > please suggest an acceptable solution for this issue
> > On Fri, Nov 16, 2018 at 10:11 PM Gal Ben Haim <gbenhaim@augury.com> wrote:
> > >
> > > what should signal_handler do in case of SIGPIPE? will failed write
> > > return EPIPE if the signal is catched?
> > >
> > > On Fri, Nov 16, 2018 at 10:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > > >
> > > > Hi 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 an
> > > > > error which will be logged as "io_send: Broken pipe".
> > > > > ---
> > > > > src/main.c | 10 +++++++---
> > > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/src/main.c b/src/main.c
> > > > > index 4716f5388..c62886593 100644
> > > > > --- a/src/main.c
> > > > > +++ b/src/main.c
> > > > > @@ -691,7 +691,7 @@ int main(int argc, char *argv[])
> > > > >       uint16_t sdp_mtu = 0;
> > > > >       uint32_t sdp_flags = 0;
> > > > >       int gdbus_flags = 0;
> > > > > -     guint signal, watchdog;
> > > > > +     guint signal_source, watchdog;
> > > > >       const char *watchdog_usec;
> > > > >
> > > > >       init_defaults();
> > > > > @@ -721,7 +721,11 @@ int main(int argc, char *argv[])
> > > > >
> > > > >       event_loop = g_main_loop_new(NULL, FALSE);
> > > > >
> > > > > -     signal = setup_signalfd();
> > > > > +     signal_source = setup_signalfd();
> > > > > +
> > > > > +     /* Ignore SIGPIPE, a broken pipe error will be returned from write
> > > > > +      * attempts to a pipe with no readers */
> > > > > +     signal(SIGPIPE, SIG_IGN);
> > > >
> > > > if we decide not to use MSG_NOSIGNAL, then everything has to go via signalfd and not just by hacking in signal(SIG_IGN).
> > > >
> > > > Regards
> > > >
> > > > Marcel
> > > >
>
>
>
> --
> Luiz Augusto von Dentz