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@xxxxxxxxxx/ >> [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@xxxxxxxxxx> >> --- >> >> 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:
OpenPGP_signature.asc
Description: OpenPGP digital signature
