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

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6432 bytes --]

Hi Tomáš,

On 2023-08-22 17:19, 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 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
>  * Copyright 2023 (C) Red Hat, Inc., Tomas Golembiovsky <tgolembi@redhat.com>
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License as
>  * published by the Free Software Foundation.

No version of the GPL may be understood as GPL-1.0-or-later.  I suggest
specifying.  But rather, we prefer SPDX license tags in this project,
so could you please specify the license as something SPDX?

Thanks,
Alex

>  *
>  * This program is distributed in the hope that it would be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, write the Free Software Foundation,
>  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>  */
> 
> 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>
> ---
>  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.

LGTM.


>  .\"
>  .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 --]