Re: [PATCH v2] close.2: Suggest shutdown(2) when closing socket from another thread

[Alejandro Colomar <alx@kernel.org>]

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