From f34e8a4dc1a8a0ce3528dc60653b1670e4ff993f Mon Sep 17 00:00:00 2001 From: Gerhard Rieger Date: Mon, 28 Dec 2020 11:10:03 +0100 Subject: [PATCH] Mitigated race condition of SYSTEM,EXEC child processes --- CHANGES | 3 +++ socat.c | 74 ++++++++++++++++++++-------------------------------- test.sh | 18 +++++++------ xio.h | 1 + xiosigchld.c | 4 ++- 5 files changed, 45 insertions(+), 55 deletions(-) diff --git a/CHANGES b/CHANGES index 580a120..123347f 100644 --- a/CHANGES +++ b/CHANGES @@ -18,6 +18,9 @@ Corrections: building failed on Solaris 9. Thanks to Greg Earle for reporting this issue and providing a patch. + Mitigated race condition of quickly terminating SYSTEM or EXEC child + processes. + Porting: In gcc version 10 the default changed from -fcommon to -fno-common. Consequently, linking filan and procan failed with error diff --git a/socat.c b/socat.c index c772c93..f6fe0fc 100644 --- a/socat.c +++ b/socat.c @@ -597,29 +597,20 @@ int socat(const char *address1, const char *address2) { (XIO_RDSTREAM(sock1)->howtoend == END_KILL || XIO_RDSTREAM(sock1)->howtoend == END_CLOSE_KILL || XIO_RDSTREAM(sock1)->howtoend == END_SHUTDOWN_KILL)) { - if (XIO_RDSTREAM(sock1)->para.exec.pid == diedunknown1) { - /* child has alread died... but it might have put regular data into - the communication channel, so continue */ - Info1("child "F_pid" has already died (diedunknown1)", - XIO_RDSTREAM(sock1)->para.exec.pid); - diedunknown1 = 0; - XIO_RDSTREAM(sock1)->para.exec.pid = 0; - /* return STAT_RETRYLATER; */ - } else if (XIO_RDSTREAM(sock1)->para.exec.pid == diedunknown2) { - Info1("child "F_pid" has already died (diedunknown2)", - XIO_RDSTREAM(sock1)->para.exec.pid); - diedunknown2 = 0; - XIO_RDSTREAM(sock1)->para.exec.pid = 0; - } else if (XIO_RDSTREAM(sock1)->para.exec.pid == diedunknown3) { - Info1("child "F_pid" has already died (diedunknown3)", - XIO_RDSTREAM(sock1)->para.exec.pid); - diedunknown3 = 0; - XIO_RDSTREAM(sock1)->para.exec.pid = 0; - } else if (XIO_RDSTREAM(sock1)->para.exec.pid == diedunknown4) { - Info1("child "F_pid" has already died (diedunknown4)", - XIO_RDSTREAM(sock1)->para.exec.pid); - diedunknown4 = 0; - XIO_RDSTREAM(sock1)->para.exec.pid = 0; + int i; + for (i = 0; i < NUMUNKNOWN; ++i) { + if (XIO_RDSTREAM(sock1)->para.exec.pid == diedunknown[i]) { + /* child has alread died... but it might have put regular data into + the communication channel, so continue */ + Info2("child "F_pid" has already died with status %d", + XIO_RDSTREAM(sock1)->para.exec.pid, statunknown[i]); + if (statunknown[i] != 0) { + return 1; + } + diedunknown[i] = 0; + XIO_RDSTREAM(sock1)->para.exec.pid = 0; + /* return STAT_RETRYLATER; */ + } } } #endif @@ -648,29 +639,20 @@ int socat(const char *address1, const char *address2) { (XIO_RDSTREAM(sock2)->howtoend == END_KILL || XIO_RDSTREAM(sock2)->howtoend == END_CLOSE_KILL || XIO_RDSTREAM(sock2)->howtoend == END_SHUTDOWN_KILL)) { - if (XIO_RDSTREAM(sock2)->para.exec.pid == diedunknown1) { - /* child has alread died... but it might have put regular data into - the communication channel, so continue */ - Info1("child "F_pid" has already died (diedunknown1)", - XIO_RDSTREAM(sock2)->para.exec.pid); - diedunknown1 = 0; - XIO_RDSTREAM(sock2)->para.exec.pid = 0; - /* return STAT_RETRYLATER; */ - } else if (XIO_RDSTREAM(sock2)->para.exec.pid == diedunknown2) { - Info1("child "F_pid" has already died (diedunknown2)", - XIO_RDSTREAM(sock2)->para.exec.pid); - diedunknown2 = 0; - XIO_RDSTREAM(sock2)->para.exec.pid = 0; - } else if (XIO_RDSTREAM(sock2)->para.exec.pid == diedunknown3) { - Info1("child "F_pid" has already died (diedunknown3)", - XIO_RDSTREAM(sock2)->para.exec.pid); - diedunknown3 = 0; - XIO_RDSTREAM(sock2)->para.exec.pid = 0; - } else if (XIO_RDSTREAM(sock2)->para.exec.pid == diedunknown4) { - Info1("child "F_pid" has already died (diedunknown4)", - XIO_RDSTREAM(sock2)->para.exec.pid); - diedunknown4 = 0; - XIO_RDSTREAM(sock2)->para.exec.pid = 0; + int i; + for (i = 0; i < NUMUNKNOWN; ++i) { + if (XIO_RDSTREAM(sock2)->para.exec.pid == diedunknown[i]) { + /* child has alread died... but it might have put regular data into + the communication channel, so continue */ + Info2("child "F_pid" has already died with status %d", + XIO_RDSTREAM(sock2)->para.exec.pid, statunknown[i]); + if (statunknown[i] != 0) { + return 1; + } + diedunknown[i] = 0; + XIO_RDSTREAM(sock2)->para.exec.pid = 0; + /* return STAT_RETRYLATER; */ + } } } #endif diff --git a/test.sh b/test.sh index 618a8df..feb79f2 100755 --- a/test.sh +++ b/test.sh @@ -8520,16 +8520,17 @@ esac N=$((N+1)) -# test the shut-null and null-eof options +# Test the shut-null and null-eof options NAME=SHUTNULLEOF case "$TESTS" in *%$N%*|*%functions%*|*%socket%*|*%$NAME%*) TEST="$NAME: options shut-null and null-eof" -# run a receiving background process with option null-eof. -# start a sending process with option shut-null that sends a test record to the +# Run a receiving background process with option null-eof. +# Start a sending process with option shut-null that sends a test record to the # receiving process and then terminates. -# send another test record. -# whe the receiving process just got the first test record the test succeeded +# Send another test record. +# When the receiving process only received and stored the first test record the +# test succeeded if ! eval $NUMCOND; then :; else tf="$td/test$N.stout" te="$td/test$N.stderr" @@ -8541,9 +8542,9 @@ printf "test $F_n $TEST... " $N $CMD0 >/dev/null 2>"${te}0" & pid0=$! waitudp4port $PORT 1 -echo "$da" |$CMD1 >"${tf}1" 2>"${te}1" +{ echo "$da"; sleep 0.1; } |$CMD1 >"${tf}1" 2>"${te}1" rc1=$? -echo "xyz" |$CMD1 >"${tf}2" 2>"${te}2" +{ echo "xyz"; sleep 0.1; } |$CMD1 >"${tf}2" 2>"${te}2" rc2=$? kill $pid0 2>/dev/null; wait if [ $rc1 != 0 -o $rc2 != 0 ]; then @@ -12161,7 +12162,7 @@ case "$TESTS" in TEST="$NAME: correct evaluation of range mask 0" if ! eval $NUMCOND; then :; elif [ -z "$SECONDADDR" ]; then - # we need access to a second addresses + # we need access to a second address $PRINTF "test $F_n $TEST... ${YELLOW}need a second IPv4 address${NORMAL}\n" $N numCANT=$((numCANT+1)) listCANT="$listCANT $N" @@ -12179,6 +12180,7 @@ pid0=$! waittcp4port $PORT 1 echo "$da" |$CMD1 >"${tf}1" 2>"${te}1" rc1=$? +sleep 1 kill $pid0 2>/dev/null; wait if [ $rc1 != 0 ]; then $PRINTF "${YELLOW}invocation failed${NORMAL}\n" diff --git a/xio.h b/xio.h index 0b917d8..79a31f0 100644 --- a/xio.h +++ b/xio.h @@ -416,6 +416,7 @@ extern pid_t diedunknown[NUMUNKNOWN]; /* child died before it is registered */ #define diedunknown2 (diedunknown[1]) #define diedunknown3 (diedunknown[2]) #define diedunknown4 (diedunknown[3]) +extern int statunknown[NUMUNKNOWN]; /* exit state of unknown dead child */ extern int xiosetsigchild(xiofile_t *xfd, int (*callback)(struct single *)); extern int xiosetchilddied(void); diff --git a/xiosigchld.c b/xiosigchld.c index 8edb25c..d09947f 100644 --- a/xiosigchld.c +++ b/xiosigchld.c @@ -11,6 +11,7 @@ /*!! with socat, at most 4 exec children exist */ pid_t diedunknown[NUMUNKNOWN]; /* children that died before they were registered */ +int statunknown[NUMUNKNOWN]; /* exit state of unknown dead child */ size_t nextunknown; @@ -120,7 +121,8 @@ void childdied(int signum) { if (nextunknown == NUMUNKNOWN) { nextunknown = 0; } - diedunknown[nextunknown++] = pid; + diedunknown[nextunknown] = pid; + statunknown[nextunknown++] = WEXITSTATUS(status); Debug1("saving pid in diedunknown"F_Zu, nextunknown/*sic, for compatibility*/); }