* [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread
@ 2023-09-14 7:42 Tomáš Golembiovský
2023-09-14 9:38 ` Alejandro Colomar
0 siblings, 1 reply; 3+ messages in thread
From: Tomáš Golembiovský @ 2023-09-14 7:42 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: linux-man, Tomáš Golembiovský
This is another take on the ancient saga of closing sockets from one
thread while another thread is blocked on recv(2). It all started with
[1,2] and continued by [3]. It was established that this is expected and
long term behavior or Linux and the issue was closed by Michael Kerrisk
by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.
This is all fine and the patch covered the issue in general terms.
However, it does not mention the specific case of sockets and shutdown,
where the issue can be (at least for the read case) mitigated by proper
shutdown. While one may argue that such information may be implied by
other man pages (perhaps return value of recv(2)) and thus is redundant,
it seems only fair to mention shutdown(2) here as it is only rarely
noted in Linux documentation that properly shutting down both side of
the socket is a good programming practice when dealing with sockets.
As a test program I am attaching the program originally written by Lukas
Czerner. Only with small fixes here and there.
[1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
[2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
[3] https://bugzilla.kernel.org/show_bug.cgi?id=53781
```c
/**
* Copyright Red Hat
* SPDX-License-Identifier: GPL-2.0-or-later
*/
void *close_socket(void *arg) {
int sockfd = *(int *)arg;
sleep(3);
printf("Thread: closing socket %d\n", sockfd);
// shutdown(sockfd, SHUT_RDWR);
close(sockfd);
return NULL;
}
int client(void) {
int sockfd;
int len;
struct sockaddr_un address;
int ret;
char *buffer=malloc(BUFSIZE);
pthread_t thread;
sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
address.sun_family = AF_UNIX;
strcpy(address.sun_path, "server_socket");
len = sizeof(address);
ret = connect(sockfd, (struct sockaddr *)&address, len);
if (ret == -1) {
perror("connect");
return 1;
}
printf("client connected\n");
ret = pthread_create(&thread, NULL, close_socket, (void *)&sockfd);
if (ret != 0) {
perror("Creating thread failed");
return 1;
}
while (1) {
ret = recv(sockfd,buffer,BUFSIZE,0);
if (ret < 0) {
perror("recv");
return 1;
}
printf("Data received: %s\n", buffer);
sleep(1);
}
close(sockfd);
return 0;
}
int server(void) {
char *message="This is the message I am sending to you";
struct sockaddr_un server_addr, client_addr;
int server_sockfd, client_sockfd;
socklen_t server_len, client_len;
int ret;
unlink("server_socket");
server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
server_addr.sun_family = AF_UNIX;
strcpy(server_addr.sun_path, "server_socket");
server_len = sizeof(server_addr);
bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);
listen(server_sockfd, 5);
client_len = sizeof(client_addr);
client_sockfd = accept(server_sockfd,
(struct sockaddr *)&client_addr, &client_len);
printf("Server: sending data...\n");
ret = send(client_sockfd ,message,strlen(message),0);
if (ret < 0) {
perror("send");
return 1;
}
/* simulate running server by not closing the client_socket socket */
return 0;
}
int main() {
pid_t pid;
pid = fork();
if (pid < 0) {
perror("fork failed");
exit(1);
}
if (pid > 0) {
printf(" - starting server\n");
server();
printf(" - exiting server\n");
wait(NULL);
} else {
sleep(1);
printf(" - starting client\n");
client();
printf(" - exiting client\n");
}
}
```
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
v2: updated test file header
man2/close.2 | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/man2/close.2 b/man2/close.2
index 68211bc58..08c6a0839 100644
--- a/man2/close.2
+++ b/man2/close.2
@@ -145,6 +145,11 @@ Thus, the blocking system call in the first thread may successfully
complete after the
.BR close ()
in the second thread.
+.PP
+When dealing with sockets,
+blocking forever in another thread may be prevented by using
+.BR shutdown (2)
+to shut down both parts of the connection before closing the socket.
.\"
.SS Dealing with error returns from close()
A careful programmer will check the return value of
--
2.42.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread
2023-09-14 7:42 [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread Tomáš Golembiovský
@ 2023-09-14 9:38 ` Alejandro Colomar
2023-09-14 9:49 ` Alejandro Colomar
0 siblings, 1 reply; 3+ messages in thread
From: Alejandro Colomar @ 2023-09-14 9:38 UTC (permalink / raw)
To: Tomáš Golembiovský; +Cc: linux-man
[-- Attachment #1.1: Type: text/plain, Size: 7555 bytes --]
On 2023-09-14 09:42, Tomáš Golembiovský wrote:
> This is another take on the ancient saga of closing sockets from one
> thread while another thread is blocked on recv(2). It all started with
> [1,2] and continued by [3]. It was established that this is expected and
> long term behavior or Linux and the issue was closed by Michael Kerrisk
> by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.
>
> This is all fine and the patch covered the issue in general terms.
> However, it does not mention the specific case of sockets and shutdown,
> where the issue can be (at least for the read case) mitigated by proper
> shutdown. While one may argue that such information may be implied by
> other man pages (perhaps return value of recv(2)) and thus is redundant,
> it seems only fair to mention shutdown(2) here as it is only rarely
> noted in Linux documentation that properly shutting down both side of
> the socket is a good programming practice when dealing with sockets.
>
> As a test program I am attaching the program originally written by Lukas
> Czerner. Only with small fixes here and there.
>
> [1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
> [3] https://bugzilla.kernel.org/show_bug.cgi?id=53781
>
> ```c
> /**
> * Copyright Red Hat
> * SPDX-License-Identifier: GPL-2.0-or-later
> */
>
> void *close_socket(void *arg) {
> int sockfd = *(int *)arg;
>
> sleep(3);
> printf("Thread: closing socket %d\n", sockfd);
> // shutdown(sockfd, SHUT_RDWR);
> close(sockfd);
> return NULL;
> }
>
> int client(void) {
> int sockfd;
> int len;
> struct sockaddr_un address;
> int ret;
> char *buffer=malloc(BUFSIZE);
> pthread_t thread;
>
> sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> address.sun_family = AF_UNIX;
> strcpy(address.sun_path, "server_socket");
> len = sizeof(address);
>
> ret = connect(sockfd, (struct sockaddr *)&address, len);
> if (ret == -1) {
> perror("connect");
> return 1;
> }
> printf("client connected\n");
>
> ret = pthread_create(&thread, NULL, close_socket, (void *)&sockfd);
> if (ret != 0) {
> perror("Creating thread failed");
> return 1;
> }
>
> while (1) {
> ret = recv(sockfd,buffer,BUFSIZE,0);
> if (ret < 0) {
> perror("recv");
> return 1;
> }
> printf("Data received: %s\n", buffer);
> sleep(1);
> }
>
> close(sockfd);
> return 0;
> }
>
> int server(void) {
> char *message="This is the message I am sending to you";
> struct sockaddr_un server_addr, client_addr;
> int server_sockfd, client_sockfd;
> socklen_t server_len, client_len;
> int ret;
>
> unlink("server_socket");
> server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> server_addr.sun_family = AF_UNIX;
> strcpy(server_addr.sun_path, "server_socket");
> server_len = sizeof(server_addr);
> bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);
>
> listen(server_sockfd, 5);
>
> client_len = sizeof(client_addr);
> client_sockfd = accept(server_sockfd,
> (struct sockaddr *)&client_addr, &client_len);
>
> printf("Server: sending data...\n");
> ret = send(client_sockfd ,message,strlen(message),0);
> if (ret < 0) {
> perror("send");
> return 1;
> }
>
> /* simulate running server by not closing the client_socket socket */
> return 0;
> }
>
> int main() {
> pid_t pid;
>
> pid = fork();
> if (pid < 0) {
> perror("fork failed");
> exit(1);
> }
> if (pid > 0) {
> printf(" - starting server\n");
> server();
> printf(" - exiting server\n");
> wait(NULL);
> } else {
> sleep(1);
> printf(" - starting client\n");
> client();
> printf(" - exiting client\n");
> }
> }
> ```
Here are some fixes to the program:
$ diff -u old.c new.c
--- old.c 2023-09-14 11:35:06.556184223 +0200
+++ new.c 2023-09-14 11:35:33.109203789 +0200
@@ -3,6 +3,16 @@
* SPDX-License-Identifier: GPL-2.0-or-later
*/
+ #include <pthread.h>
+ #include <stddef.h>
+ #include <stdio.h>
+ #include <stdlib.h>
+ #include <string.h>
+ #include <sys/socket.h>
+ #include <sys/un.h>
+ #include <sys/wait.h>
+ #include <unistd.h>
+
void *close_socket(void *arg) {
int sockfd = *(int *)arg;
@@ -15,10 +25,11 @@
int client(void) {
int sockfd;
- int len;
+ socklen_t len;
struct sockaddr_un address;
int ret;
- char *buffer=malloc(BUFSIZE);
+ ssize_t r;
+ char *buffer=malloc(BUFSIZ);
pthread_t thread;
sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -41,8 +52,8 @@
}
while (1) {
- ret = recv(sockfd,buffer,BUFSIZE,0);
- if (ret < 0) {
+ r = recv(sockfd, buffer, BUFSIZ, 0);
+ if (r < 0) {
perror("recv");
return 1;
}
@@ -59,7 +70,7 @@
struct sockaddr_un server_addr, client_addr;
int server_sockfd, client_sockfd;
socklen_t server_len, client_len;
- int ret;
+ ssize_t ret;
unlink("server_socket");
server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -76,7 +87,7 @@
(struct sockaddr *)&client_addr, &client_len);
printf("Server: sending data...\n");
- ret = send(client_sockfd ,message,strlen(message),0);
+ ret = send(client_sockfd, message, strlen(message), 0);
if (ret < 0) {
perror("send");
return 1;
After those fixes, I still get a warning of dead code:
$ clang -Weverything -Wno-missing-prototypes new.c
new.c:64:9: warning: code will never be executed [-Wunreachable-code]
close(sockfd);
^~~~~
1 warning generated.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>
> v2: updated test file header
>
> man2/close.2 | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/man2/close.2 b/man2/close.2
> index 68211bc58..08c6a0839 100644
> --- a/man2/close.2
> +++ b/man2/close.2
> @@ -145,6 +145,11 @@ Thus, the blocking system call in the first thread may successfully
> complete after the
> .BR close ()
> in the second thread.
> +.PP
> +When dealing with sockets,
> +blocking forever in another thread may be prevented by using
> +.BR shutdown (2)
> +to shut down both parts of the connection before closing the socket.
> .\"
> .SS Dealing with error returns from close()
> A careful programmer will check the return value of
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread
2023-09-14 9:38 ` Alejandro Colomar
@ 2023-09-14 9:49 ` Alejandro Colomar
0 siblings, 0 replies; 3+ messages in thread
From: Alejandro Colomar @ 2023-09-14 9:49 UTC (permalink / raw)
To: Tomáš Golembiovský; +Cc: linux-man
[-- Attachment #1.1: Type: text/plain, Size: 8808 bytes --]
On 2023-09-14 11:38, Alejandro Colomar wrote:
> On 2023-09-14 09:42, Tomáš Golembiovský wrote:
>> This is another take on the ancient saga of closing sockets from one
>> thread while another thread is blocked on recv(2). It all started with
>> [1,2] and continued by [3]. It was established that this is expected and
>> long term behavior or Linux and the issue was closed by Michael Kerrisk
>> by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.
>>
>> This is all fine and the patch covered the issue in general terms.
>> However, it does not mention the specific case of sockets and shutdown,
>> where the issue can be (at least for the read case) mitigated by proper
>> shutdown. While one may argue that such information may be implied by
>> other man pages (perhaps return value of recv(2)) and thus is redundant,
>> it seems only fair to mention shutdown(2) here as it is only rarely
>> noted in Linux documentation that properly shutting down both side of
>> the socket is a good programming practice when dealing with sockets.
>>
>> As a test program I am attaching the program originally written by Lukas
>> Czerner. Only with small fixes here and there.
>>
>> [1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
>> [3] https://bugzilla.kernel.org/show_bug.cgi?id=53781
>>
>> ```c
>> /**
>> * Copyright Red Hat
>> * SPDX-License-Identifier: GPL-2.0-or-later
>> */
>>
>> void *close_socket(void *arg) {
>> int sockfd = *(int *)arg;
>>
>> sleep(3);
>> printf("Thread: closing socket %d\n", sockfd);
>> // shutdown(sockfd, SHUT_RDWR);
>> close(sockfd);
>> return NULL;
>> }
>>
>> int client(void) {
>> int sockfd;
>> int len;
>> struct sockaddr_un address;
>> int ret;
>> char *buffer=malloc(BUFSIZE);
>> pthread_t thread;
>>
>> sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
>>
>> address.sun_family = AF_UNIX;
>> strcpy(address.sun_path, "server_socket");
>> len = sizeof(address);
>>
>> ret = connect(sockfd, (struct sockaddr *)&address, len);
>> if (ret == -1) {
>> perror("connect");
>> return 1;
>> }
>> printf("client connected\n");
>>
>> ret = pthread_create(&thread, NULL, close_socket, (void *)&sockfd);
>> if (ret != 0) {
>> perror("Creating thread failed");
>> return 1;
>> }
>>
>> while (1) {
>> ret = recv(sockfd,buffer,BUFSIZE,0);
>> if (ret < 0) {
>> perror("recv");
>> return 1;
>> }
>> printf("Data received: %s\n", buffer);
>> sleep(1);
>> }
>>
>> close(sockfd);
>> return 0;
>> }
>>
>> int server(void) {
>> char *message="This is the message I am sending to you";
>> struct sockaddr_un server_addr, client_addr;
>> int server_sockfd, client_sockfd;
>> socklen_t server_len, client_len;
>> int ret;
>>
>> unlink("server_socket");
>> server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
>>
>> server_addr.sun_family = AF_UNIX;
>> strcpy(server_addr.sun_path, "server_socket");
>> server_len = sizeof(server_addr);
>> bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);
>>
>> listen(server_sockfd, 5);
>>
>> client_len = sizeof(client_addr);
>> client_sockfd = accept(server_sockfd,
>> (struct sockaddr *)&client_addr, &client_len);
>>
>> printf("Server: sending data...\n");
>> ret = send(client_sockfd ,message,strlen(message),0);
>> if (ret < 0) {
>> perror("send");
>> return 1;
>> }
>>
>> /* simulate running server by not closing the client_socket socket */
>> return 0;
>> }
>>
>> int main() {
>> pid_t pid;
>>
>> pid = fork();
>> if (pid < 0) {
>> perror("fork failed");
>> exit(1);
>> }
>> if (pid > 0) {
>> printf(" - starting server\n");
>> server();
>> printf(" - exiting server\n");
>> wait(NULL);
>> } else {
>> sleep(1);
>> printf(" - starting client\n");
>> client();
>> printf(" - exiting client\n");
>> }
>> }
>> ```
>
> Here are some fixes to the program:
>
> $ diff -u old.c new.c
> --- old.c 2023-09-14 11:35:06.556184223 +0200
> +++ new.c 2023-09-14 11:35:33.109203789 +0200
> @@ -3,6 +3,16 @@
> * SPDX-License-Identifier: GPL-2.0-or-later
> */
>
> + #include <pthread.h>
> + #include <stddef.h>
> + #include <stdio.h>
> + #include <stdlib.h>
> + #include <string.h>
> + #include <sys/socket.h>
> + #include <sys/un.h>
> + #include <sys/wait.h>
> + #include <unistd.h>
> +
> void *close_socket(void *arg) {
> int sockfd = *(int *)arg;
>
> @@ -15,10 +25,11 @@
>
> int client(void) {
> int sockfd;
> - int len;
> + socklen_t len;
> struct sockaddr_un address;
> int ret;
> - char *buffer=malloc(BUFSIZE);
> + ssize_t r;
> + char *buffer=malloc(BUFSIZ);
> pthread_t thread;
>
> sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> @@ -41,8 +52,8 @@
> }
>
> while (1) {
> - ret = recv(sockfd,buffer,BUFSIZE,0);
> - if (ret < 0) {
> + r = recv(sockfd, buffer, BUFSIZ, 0);
> + if (r < 0) {
> perror("recv");
> return 1;
> }
> @@ -59,7 +70,7 @@
> struct sockaddr_un server_addr, client_addr;
> int server_sockfd, client_sockfd;
> socklen_t server_len, client_len;
> - int ret;
> + ssize_t ret;
>
> unlink("server_socket");
> server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> @@ -76,7 +87,7 @@
> (struct sockaddr *)&client_addr, &client_len);
>
> printf("Server: sending data...\n");
> - ret = send(client_sockfd ,message,strlen(message),0);
> + ret = send(client_sockfd, message, strlen(message), 0);
> if (ret < 0) {
> perror("send");
> return 1;
>
>
> After those fixes, I still get a warning of dead code:
>
> $ clang -Weverything -Wno-missing-prototypes new.c
> new.c:64:9: warning: code will never be executed [-Wunreachable-code]
> close(sockfd);
> ^~~~~
> 1 warning generated.
And one from clang-tidy(1):
/home/alx/tmp/new.c:81:9: error: the value returned by this function should be used [bugprone-unused-return-value,-warnings-as-errors]
bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It seems it didn't realize about the ignored return value of listen(2),
but it seems there are a few functions whose error shouldn't be ignored.
And a couple from cppcheck(1):
$ cppcheck --enable=all --error-exitcode=2 --inconclusive --quiet \
--suppressions-list=./etc/cppcheck/cppcheck.suppress \
/home/alx/tmp/new.c;
/home/alx/tmp/new.c:44:17: error: Memory leak: buffer [memleak]
return 1;
^
/home/alx/tmp/new.c:51:17: error: Memory leak: buffer [memleak]
return 1;
^
Cheers,
Alex
>
>>
>> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
>> ---
>>
>> v2: updated test file header
>>
>> man2/close.2 | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/man2/close.2 b/man2/close.2
>> index 68211bc58..08c6a0839 100644
>> --- a/man2/close.2
>> +++ b/man2/close.2
>> @@ -145,6 +145,11 @@ Thus, the blocking system call in the first thread may successfully
>> complete after the
>> .BR close ()
>> in the second thread.
>> +.PP
>> +When dealing with sockets,
>> +blocking forever in another thread may be prevented by using
>> +.BR shutdown (2)
>> +to shut down both parts of the connection before closing the socket.
>> .\"
>> .SS Dealing with error returns from close()
>> A careful programmer will check the return value of
>
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-14 9:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-14 7:42 [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread Tomáš Golembiovský
2023-09-14 9:38 ` Alejandro Colomar
2023-09-14 9:49 ` Alejandro Colomar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).