diff mbox series

[1/2] network_thread: block SIGPIPE in network thread

Message ID 20240321054146.2303245-1-dominique.martinet@atmark-techno.com
State Accepted
Headers show
Series [1/2] network_thread: block SIGPIPE in network thread | expand

Commit Message

Dominique Martinet March 21, 2024, 5:41 a.m. UTC
From: Shiita ISHIGAKI <shiita.ishigaki@atmark-techno.com>

SIGPIPE was blocked in the subprocess thread since commit a781be1d0b7a
("network_thread: Don't SIGPIPE-die on IPC client crash"), but the main
thread sometimes sends reply through write() directly (for e.g. STATUS)

In this case, swupdate crashes if the client is gone, which is not
something we want.

Since we block SIGPIPE before creating the subprocess thread we no
longer need to block it there.

(Add errno in write error message while we are here)

Signed-off-by: Shiita ISHIGAKI <shiita.ishigaki@atmark-techno.com>
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
We had a weird swupdate failure in "hawkbit mode" that turned out to be
two different problems, this appears to fix the issue and I checked
SIGPIPE is still blocked in the subprocess_thread but please
double-check.

 core/network_thread.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Stefano Babic March 21, 2024, 7:38 a.m. UTC | #1
On 21.03.24 06:41, Dominique Martinet wrote:
> From: Shiita ISHIGAKI <shiita.ishigaki@atmark-techno.com>
>
> SIGPIPE was blocked in the subprocess thread since commit a781be1d0b7a
> ("network_thread: Don't SIGPIPE-die on IPC client crash"), but the main
> thread sometimes sends reply through write() directly (for e.g. STATUS)
>
> In this case, swupdate crashes if the client is gone, which is not
> something we want.
>
> Since we block SIGPIPE before creating the subprocess thread we no
> longer need to block it there.
>
> (Add errno in write error message while we are here)
>
> Signed-off-by: Shiita ISHIGAKI <shiita.ishigaki@atmark-techno.com>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> We had a weird swupdate failure in "hawkbit mode" that turned out to be
> two different problems, this appears to fix the issue and I checked
> SIGPIPE is still blocked in the subprocess_thread but please
> double-check.
>
>   core/network_thread.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/core/network_thread.c b/core/network_thread.c
> index ca23908d0cda..d7b713fb2154 100644
> --- a/core/network_thread.c
> +++ b/core/network_thread.c
> @@ -337,11 +337,6 @@ static void *subprocess_thread (void *data)
>   	(void)data;
>   	thread_ready();
>
> -	sigset_t sigpipe_mask;
> -	sigemptyset(&sigpipe_mask);
> -	sigaddset(&sigpipe_mask, SIGPIPE);
> -	pthread_sigmask(SIG_BLOCK, &sigpipe_mask, NULL);
> -
>   	pthread_mutex_lock(&subprocess_msg_lock);
>
>   	while(1) {
> @@ -393,6 +388,11 @@ void *network_thread (void *data)
>   	SIMPLEQ_INIT(&subprocess_messages);
>   	register_notifier(network_notifier);
>
> +	sigset_t sigpipe_mask;
> +	sigemptyset(&sigpipe_mask);
> +	sigaddset(&sigpipe_mask, SIGPIPE);
> +	pthread_sigmask(SIG_BLOCK, &sigpipe_mask, NULL);
> +

I think it is ok and it is safe to block SIGPIPE directly in the main
thread.

Acked-by: Stefano Babic <stefano.babic@swupdate.org>

Best regards,
Stefano Babic

>   	subprocess_ipc_handler_thread_id = start_thread(subprocess_thread, NULL);
>
>   	/* Initialize and bind to UDS */
> @@ -644,7 +644,7 @@ void *network_thread (void *data)
>   		if (msg.type == ACK || msg.type == NACK) {
>   			ret = write(ctrlconnfd, &msg, sizeof(msg));
>   			if (ret < 0)
> -				ERROR("Error write on socket ctrl");
> +				ERROR("Error write on socket ctrl: %s", strerror(errno));
>
>   			if (should_close_socket == true)
>   				close(ctrlconnfd);
diff mbox series

Patch

diff --git a/core/network_thread.c b/core/network_thread.c
index ca23908d0cda..d7b713fb2154 100644
--- a/core/network_thread.c
+++ b/core/network_thread.c
@@ -337,11 +337,6 @@  static void *subprocess_thread (void *data)
 	(void)data;
 	thread_ready();
 
-	sigset_t sigpipe_mask;
-	sigemptyset(&sigpipe_mask);
-	sigaddset(&sigpipe_mask, SIGPIPE);
-	pthread_sigmask(SIG_BLOCK, &sigpipe_mask, NULL);
-
 	pthread_mutex_lock(&subprocess_msg_lock);
 
 	while(1) {
@@ -393,6 +388,11 @@  void *network_thread (void *data)
 	SIMPLEQ_INIT(&subprocess_messages);
 	register_notifier(network_notifier);
 
+	sigset_t sigpipe_mask;
+	sigemptyset(&sigpipe_mask);
+	sigaddset(&sigpipe_mask, SIGPIPE);
+	pthread_sigmask(SIG_BLOCK, &sigpipe_mask, NULL);
+
 	subprocess_ipc_handler_thread_id = start_thread(subprocess_thread, NULL);
 
 	/* Initialize and bind to UDS */
@@ -644,7 +644,7 @@  void *network_thread (void *data)
 		if (msg.type == ACK || msg.type == NACK) {
 			ret = write(ctrlconnfd, &msg, sizeof(msg));
 			if (ret < 0)
-				ERROR("Error write on socket ctrl");
+				ERROR("Error write on socket ctrl: %s", strerror(errno));
 
 			if (should_close_socket == true)
 				close(ctrlconnfd);