From b2bda6eb6e8782446c05145fb525cdadcf49465e Mon Sep 17 00:00:00 2001
From: Davin McCall <davmac@davmac.org>
Date: Mon, 26 Jun 2017 19:26:46 +0100
Subject: [PATCH] service: improve robustness of read_pid_file().

Cleans up some TODOs.
---
 src/dinit-util.h | 35 ++++++++++++++++++++++++
 src/service.cc   | 70 +++++++++++++++++++++++++++++-------------------
 2 files changed, 78 insertions(+), 27 deletions(-)
 create mode 100644 src/dinit-util.h

diff --git a/src/dinit-util.h b/src/dinit-util.h
new file mode 100644
index 0000000..3b4c42c
--- /dev/null
+++ b/src/dinit-util.h
@@ -0,0 +1,35 @@
+#ifndef DINIT_UTIL_H_INCLUDED
+#define DINIT_UTIL_H_INCLUDED 1
+
+#include <cerrno>
+
+// Signal-safe read. Read and re-try if interrupted by signal (EINTR).
+// *May* affect errno even on a successful read (when the return is less than n).
+inline int ss_read(int fd, void * buf, size_t n)
+{
+    char * cbuf = static_cast<char *>(buf);
+    int r = 0;
+    while (r < n) {
+        int res = read(fd, cbuf + r, n - r);
+        if (res == 0) {
+            return r;
+        }
+        if (res < 0) {
+            if (res == EINTR) {
+                continue;
+            }
+
+            // If any other error, and we have successfully read some, return it:
+            if (r == 0) {
+                return -1;
+            }
+            else {
+                return r;
+            }
+        }
+        r += res;
+    }
+    return n;
+}
+
+#endif
diff --git a/src/service.cc b/src/service.cc
index cda831e..4a57bad 100644
--- a/src/service.cc
+++ b/src/service.cc
@@ -17,6 +17,7 @@
 #include "service.h"
 #include "dinit-log.h"
 #include "dinit-socket.h"
+#include "dinit-util.h"
 
 /*
  * service.cc - Service management.
@@ -704,7 +705,7 @@ bgproc_service::read_pid_file(int *exit_status) noexcept
     }
 
     char pidbuf[21]; // just enough to hold any 64-bit integer
-    int r = read(fd, pidbuf, 20); // TODO signal-safe read
+    int r = ss_read(fd, pidbuf, 20);
     if (r < 0) {
         // Could not read from PID file
         log(LogLevel::ERROR, service_name, ": could not read from pidfile; ", strerror(errno));
@@ -714,37 +715,52 @@ bgproc_service::read_pid_file(int *exit_status) noexcept
 
     close(fd);
     pidbuf[r] = 0; // store nul terminator
-    // TODO may need stoull; what if int isn't big enough...
-    pid = std::atoi(pidbuf);
-    pid_t wait_r = waitpid(pid, exit_status, WNOHANG);
-    if (wait_r == -1 && errno == ECHILD) {
-        // We can't track this child - check process exists:
-        if (kill(pid, 0) == 0) {
-            tracking_child = false;
-            return pid_result_t::OK;
-        }
-        else {
-            log(LogLevel::ERROR, service_name, ": pid read from pidfile (", pid, ") is not valid");
-            pid = -1;
-            return pid_result_t::FAILED;
+
+    bool valid_pid = false;
+    try {
+        unsigned long long v = std::stoull(pidbuf, nullptr, 0);
+        if (v <= std::numeric_limits<pid_t>::max()) {
+            pid = (pid_t) v;
+            valid_pid = true;
         }
     }
-    else if (wait_r == pid) {
-        pid = -1;
-        return pid_result_t::TERMINATED;
+    catch (std::out_of_range &exc) {
+        // Too large?
     }
-    else if (wait_r == 0) {
-        // We can track the child
-        child_listener.add_reserved(eventLoop, pid, DEFAULT_PRIORITY - 10);
-        tracking_child = true;
-        reserved_child_watch = true;
-        return pid_result_t::OK;
+    catch (std::invalid_argument &exc) {
+        // Ok, so it doesn't look like a number: proceed...
     }
-    else {
-        log(LogLevel::ERROR, service_name, ": pid read from pidfile (", pid, ") is not valid");
-        pid = -1;
-        return pid_result_t::FAILED;
+
+    if (valid_pid) {
+        pid_t wait_r = waitpid(pid, exit_status, WNOHANG);
+        if (wait_r == -1 && errno == ECHILD) {
+            // We can't track this child - check process exists:
+            if (kill(pid, 0) == 0) {
+                tracking_child = false;
+                return pid_result_t::OK;
+            }
+            else {
+                log(LogLevel::ERROR, service_name, ": pid read from pidfile (", pid, ") is not valid");
+                pid = -1;
+                return pid_result_t::FAILED;
+            }
+        }
+        else if (wait_r == pid) {
+            pid = -1;
+            return pid_result_t::TERMINATED;
+        }
+        else if (wait_r == 0) {
+            // We can track the child
+            child_listener.add_reserved(eventLoop, pid, DEFAULT_PRIORITY - 10);
+            tracking_child = true;
+            reserved_child_watch = true;
+            return pid_result_t::OK;
+        }
     }
+
+    log(LogLevel::ERROR, service_name, ": pid read from pidfile (", pid, ") is not valid");
+    pid = -1;
+    return pid_result_t::FAILED;
 }
 
 void service_record::started() noexcept
-- 
2.25.1