From 142169da59518ee17bbf885dfbf42525fd2c1d0b Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 31 Jan 2017 20:06:21 +0100

Subject: [PATCH 1/2] Low: libservices (sync): ensure no zombie is left behind

It could happen because this parent that is to wait for its

not-well-behaved child is knowlingly not blocking any signal (beside

SIGCHLD explicitly in case of using signalfd facility) and delivery of

such signal can interrupt waitpid, at least on paper, so protect

against this accordingly.

Speaking of SIGCHLD in plain self-pipe (not signalfd one) context, while

there's no clash with other synchronous actions, it may be the case with

asynchronous ones (or for that matter, arbitrary other fork-related

activities in the main program a library can have no idea about), and

this is exactly what could interrupt waitpid for real.

---

 lib/services/services_linux.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c

index cd7fd3f..ffb74ef 100644

--- a/lib/services/services_linux.c

+++ b/lib/services/services_linux.c

@@ -517,7 +517,7 @@ action_synced_wait(svc_action_t * op, sigset_t *mask)

                 if (1) {

                     /* Clear out the sigchld pipe. */

                     char ch;

-                    while (read(sfd, &ch, 1) == 1);

+                    while (read(sfd, &ch, 1) == 1) /*omit*/;

 #endif

                     wait_rc = waitpid(op->pid, &status, WNOHANG);

@@ -567,7 +567,7 @@ action_synced_wait(svc_action_t * op, sigset_t *mask)

          *

          * This makes it safe to skip WNOHANG here

          */

-        waitpid(op->pid, &status, 0);

+        while (waitpid(op->pid, &status, 0) == (pid_t) -1 && errno == EINTR) /*omit*/;

     } else if (WIFEXITED(status)) {

         op->status = PCMK_LRM_OP_DONE;

-- 

1.8.3.1

From 9acc32260562df262a768bd2259372d16d90283b Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 31 Jan 2017 20:49:04 +0100

Subject: [PATCH 2/2] Low: libservices(sync): partially prevent killing foreign

 process

Do not attempt sending SIGKILL to process for which previous

waitpid(pid, &st, WNOHANG) indicated ECHILD issue.  This might

be a result of clashing with another thread or an orthogonal

signal handler getting to reap the child first.

For good measure, repeat non-hanging waitpid test right before

killing -- it won't prevent illustrated race, but will limit it

a bit more.  That's unlikely, sad path, hence without regularly

imposed performance penalty.  And when we can pay more attention

to prevent killing innocent processes (the code in question is

commonly run as root so nothing will prevent that), we should.

---

 lib/services/services_linux.c | 36 +++++++++++++++++++++---------------

 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c

index ffb74ef..16f25f3 100644

--- a/lib/services/services_linux.c

+++ b/lib/services/services_linux.c

@@ -521,11 +521,19 @@ action_synced_wait(svc_action_t * op, sigset_t *mask)

 #endif

                     wait_rc = waitpid(op->pid, &status, WNOHANG);

-                    if (wait_rc < 0){

-                        crm_perror(LOG_ERR, "waitpid() for %d failed", op->pid);

-

-                    } else if (wait_rc > 0) {

+                    if (wait_rc > 0) {

                         break;

+

+                    } else if (wait_rc < 0){

+                        if (errno == ECHILD) {

+                                /* Here, don't dare to kill and bail out... */

+                                break;

+

+                        } else {

+                                /* ...otherwise pretend process still runs. */

+                                wait_rc = 0;

+                        }

+                        crm_perror(LOG_ERR, "waitpid() for %d failed", op->pid);

                     }

                 }

             }

@@ -547,9 +555,8 @@ action_synced_wait(svc_action_t * op, sigset_t *mask)

     crm_trace("Child done: %d", op->pid);

     if (wait_rc <= 0) {

-        int killrc = kill(op->pid, SIGKILL);

-

         op->rc = PCMK_OCF_UNKNOWN_ERROR;

+

         if (op->timeout > 0 && timeout <= 0) {

             op->status = PCMK_LRM_OP_TIMEOUT;

             crm_warn("%s:%d - timed out after %dms", op->id, op->pid, op->timeout);

@@ -558,16 +565,15 @@ action_synced_wait(svc_action_t * op, sigset_t *mask)

             op->status = PCMK_LRM_OP_ERROR;

         }

-        if (killrc && errno != ESRCH) {

-            crm_err("kill(%d, KILL) failed: %d", op->pid, errno);

+        /* If only child hasn't been successfully waited for, yet.

+           This is to limit killing wrong target a bit more. */

+        if (wait_rc == 0 && waitpid(op->pid, &status, WNOHANG) == 0) {

+            if (kill(op->pid, SIGKILL)) {

+                crm_err("kill(%d, KILL) failed: %d", op->pid, errno);

+            }

+            /* Safe to skip WNOHANG here as we sent non-ignorable signal. */

+            while (waitpid(op->pid, &status, 0) == (pid_t) -1 && errno == EINTR) /*omit*/;

         }

-        /*

-         * From sigprocmask(2):

-         * It is not possible to block SIGKILL or SIGSTOP.  Attempts to do so are silently ignored.

-         *

-         * This makes it safe to skip WNOHANG here

-         */

-        while (waitpid(op->pid, &status, 0) == (pid_t) -1 && errno == EINTR) /*omit*/;

     } else if (WIFEXITED(status)) {

         op->status = PCMK_LRM_OP_DONE;

-- 

1.8.3.1