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