From 85edd07c3958a6344384a239935f0ddea9760795 Mon Sep 17 00:00:00 2001 From: "gu.martinm@gmail.com" Date: Sun, 23 Feb 2014 17:25:12 +0100 Subject: [PATCH] Avoid SIGPIPE and polite kill child process. SIGPIPE when using send method and remote side closed connection. Portable alternatives: sighandler for SIGPIPE or block/ignore SIGPIPE. In some situations we may not ignore SIGPIPE (if we are writing some API we may not just swallow SIGPIPE) because of that depending o the situation we could ignore or block SIGPIPE. There is SIGPIPE JUST when using send/write (no with read/recv) and remote side closed connection. --- Daemon/javafork.c | 104 +++++++++++++++++++++++++++++++++++++++--------------- Daemon/javafork.h | 4 +++ 2 files changed, 80 insertions(+), 28 deletions(-) diff --git a/Daemon/javafork.c b/Daemon/javafork.c index 1ae2139..5fdcd75 100644 --- a/Daemon/javafork.c +++ b/Daemon/javafork.c @@ -615,6 +615,48 @@ end: +int polite_kill_and_wait(pid_t pid) +{ + if (kill(pid, SIGTERM) < 0) { + syslog (LOG_ERR, "error while sending SIGTERM to child process: %m"); + return -1; + } + + sleep(5); + + // It might be interesting to use if/ else if/ else if/ to write error log when waitpid returns -1 and 0. + // In this case when -1 or 0 I am using the same LOG_ERR (see the next call to syslog just below) + // the problem is, when -1 the error is different than when 0. Perhaps it would be interesting to show a different + // error log when -1 and when 0. + if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, WNOHANG)) > 0) { + // Child process is dead. + return 0; + } + + syslog (LOG_ERR, "waitpid after SIGTERM did not work, next step trying with SIGKILL: %m"); + + if (kill(pid, SIGKILL) < 0) { + syslog (LOG_ERR, "error while sending SIGKILL to child process: %m"); + return -1; + } + + sleep(5); + + if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, WNOHANG)) > 0) { + // Child process is dead. + return 0; + } + + /*We are not sure if the child process is dead. In this case, */ + /*probably the child process is going to be an orphan and its */ + /*system process (if there is one) as well*/ + syslog (LOG_ERR, "waitpid after SIGKILL did not work either: %m"); + + return -1; +} + + + int fork_system(int socket, unsigned char *command, int *returnstatus) { pid_t pid; /*Child or parent PID.*/ @@ -734,8 +776,15 @@ int fork_system(int socket, unsigned char *command, int *returnstatus) //and the client application must know what it has to do with them. //TODO: my own protocol to make the client independent of the ENDIANNESS and character set used //by the machine running this server. See comments in the TCPForkDaemon.java code about this. - if (TEMP_FAILURE_RETRY(send(socket, buf, n+sizeof(struct tcpforkhdr), 0)) < 0) + + // Avoid SIGPIPE with MSG_NOSIGNAL flag. It just works on Linux OS (non portable code) + // Portable alternatives: sighandler for SIGPIPE or block/ignore SIGPIPE. + // There is SIGPIPE JUST when using send/write (no with read/recv) and remote side closed connection. + if (TEMP_FAILURE_RETRY(send(socket, buf, n+sizeof(struct tcpforkhdr), MSG_NOSIGNAL)) < 0) { syslog (LOG_INFO, "error while sending stdout: %m"); + polite_kill_and_wait(pid); + goto err; + } } if(polls[1].revents && POLLIN) { @@ -743,54 +792,50 @@ int fork_system(int socket, unsigned char *command, int *returnstatus) n=TEMP_FAILURE_RETRY(read(err[0], &buf[sizeof(struct tcpforkhdr)], 2000-sizeof(struct tcpforkhdr))); header->type = htonl(2); header->length = htonl(n); - if (TEMP_FAILURE_RETRY(send(socket, buf, n+sizeof(struct tcpforkhdr), 0)) < 0) + + // Avoid SIGPIPE with MSG_NOSIGNAL flag. It just works on Linux OS (non portable code) + // Portable alternatives: sighandler for SIGPIPE or block/ignore SIGPIPE. + // There is SIGPIPE JUST when using send/write (no with read/recv) and remote side closed connection. + if (TEMP_FAILURE_RETRY(send(socket, buf, n+sizeof(struct tcpforkhdr), MSG_NOSIGNAL)) < 0) { syslog (LOG_INFO, "error while sending stderr: %m"); + polite_kill_and_wait(pid); + goto err; + } } if(!(polls[0].revents && POLLIN) && !(polls[1].revents && POLLIN)) { syslog (LOG_ERR, "parent error polling pipes: %m"); - - if (kill(pid, SIGKILL /*should I use SIGTERM and my own handler?*/) < 0) { - /*We are not sure if the child process will die. In this case, */ - /*probably the child process is going to be an orphan and its */ - /*system process (if there is one) as well*/ - syslog (LOG_ERR, "error while killing child process: %m"); - goto err; - } - - if (TEMP_FAILURE_RETRY(waitpid(pid, NULL, 0)) < 0) { - /*We are not sure if the child process is dead. In this case, */ - /*probably the child process is going to be an orphan and its */ - /*system process (if there is one) as well*/ - syslog (LOG_ERR, "error while waiting for killed child process: %m"); - } - + polite_kill_and_wait(pid); /*In the Java code, the client will get an error as the return status from the exec method.*/ goto err; } } else if (pollReturn == 0) { /*When timeout*/ - if(TEMP_FAILURE_RETRY(waitpid(pid, &childreturnstatus, WNOHANG))) { - //TODO: treating errors in waitpid function + int waitpidReturn; + int status; + waitpidReturn = TEMP_FAILURE_RETRY(waitpid(pid, &status, WNOHANG)); + if(waitpidReturn > 0) { /*Child is dead, we can finish the connection*/ /*First of all, we check the exit status of our child process*/ /*In case of error send an error status to the remote calling process*/ - if (WIFEXITED(childreturnstatus) != 1) + if (WIFEXITED(status) != 1) goto err; break; } + else if(waitpidReturn < 0) { + // Error when using waitpid function + syslog (LOG_ERR, "waitpid error after poll timeout from child process: %m"); + polite_kill_and_wait(pid); + goto err; + } + /*The child process is not dead, keep polling more data from stdout or stderr streams*/ } else { /*Return with error from poll*/ syslog (LOG_ERR, "poll error: %m"); - - /*Trying to kill child process*/ - if (kill (pid, SIGTERM) == -1) { - syslog (LOG_ERR, "kill error: %m"); - } - + polite_kill_and_wait(pid); goto err; } } @@ -803,7 +848,10 @@ end: bzero(buf,2000); header->type = htonl(3); header->length = htonl((childreturnstatus)); - if (TEMP_FAILURE_RETRY(send(socket, buf, sizeof(struct tcpforkhdr), 0)) < 0) + // Avoid SIGPIPE with MSG_NOSIGNAL flag. It just works on Linux OS (non portable code) + // Portable alternatives: sighandler for SIGPIPE or block/ignore SIGPIPE. + // There is SIGPIPE JUST when using send/write (no with read/recv) and remote side closed connection. + if (TEMP_FAILURE_RETRY(send(socket, buf, sizeof(struct tcpforkhdr), MSG_NOSIGNAL)) < 0) syslog (LOG_INFO, "error while sending return status: %m"); closeSafely (out[0]); diff --git a/Daemon/javafork.h b/Daemon/javafork.h index 54377ed..df9f18b 100644 --- a/Daemon/javafork.h +++ b/Daemon/javafork.h @@ -103,3 +103,7 @@ int is_closed_socket (int socket); int wait_for_closed_socket (int socket, long timeout); + + + +int polite_kill_and_wait(pid_t pid); -- 2.1.4