[Bug 3173] forking async worker: interrupted pipe I/O

bk: 58368f6176gs0JKnLJLfagqAg4YtBw

diff --git a/ChangeLog b/ChangeLog
index 0cb8c4fb4779b249944c144baf63c35be953e531..b304ef838673422ab0cb307b244e3ef6d7c94e33 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+---
+* [Bug 3173] forking async worker: interrupted pipe I/O <perlinger@ntp.org>
+  - initial patch by Christos Zoulas
+
 ---
 (4.2.8p9) 2016/11/21 Released by Harlan Stenn <stenn@ntp.org>
 (4.2.8p9) 2016/MM/DD Released by Harlan Stenn <stenn@ntp.org>

diff --git a/libntp/work_fork.c b/libntp/work_fork.c
index 8223fdd2f9b29b5ce531422fc5d8b992ca9c2084..37caa1c582d64550c2a18f1b97b46d2d6cd361d5 100644 (file)
--- a/libntp/work_fork.c
+++ b/libntp/work_fork.c
@@ -31,6 +31,62 @@ static       RETSIGTYPE      worker_sighup(int);
 static void            send_worker_home_atexit(void);
 static void            cleanup_after_child(blocking_child *);
 
+/* === I/O helpers === */
+/* Since we have signals enabled, there's a good chance that blocking IO
+ * via pipe suffers from EINTR -- and this goes for both directions.
+ * The next two wrappers will loop until either all the data is written
+ * or read, plus handling the EOF condition on read. They may return
+ * zero if no data was transferred at all, and effectively every return
+ * value that differs from the given transfer length signifies an error
+ * condition.
+ */
+
+static size_t
+netread(
+       int             fd,
+       void *          vb,
+       size_t          l
+       )
+{
+       char *          b = vb;
+       ssize_t         r;
+
+       while (l) {
+               r = read(fd, b, l);
+               if (r > 0) {
+                       l -= r;
+                       b += r;
+               } else if (r == 0 || errno != EINTR) {
+                       l = 0;
+               }
+       }
+       return (size_t)(b - (char *)vb);
+}
+
+
+static size_t
+netwrite(
+       int             fd,
+       const void *    vb,
+       size_t          l
+       )
+{
+       const char *    b = vb;
+       ssize_t         w;
+
+       while (l) {
+               w = write(fd, b, l);
+               if (w > 0) {
+                       l -= w;
+                       b += w;
+               } else if (errno != EINTR) {
+                       l = 0;
+               }
+       }
+       return (size_t)(b - (const char *)vb);
+}
+
+
 /* === functions === */
 /*
  * exit_worker()
@@ -200,8 +256,8 @@ send_blocking_req_internal(
        void *                  data
        )
 {
-       int octets;
-       int rc;
+       size_t  octets;
+       size_t  rc;
 
        DEBUG_REQUIRE(hdr != NULL);
        DEBUG_REQUIRE(data != NULL);
@@ -213,23 +269,18 @@ send_blocking_req_internal(
        }
 
        octets = sizeof(*hdr);
-       rc = write(c->req_write_pipe, hdr, octets);
+       rc = netwrite(c->req_write_pipe, hdr, octets);
 
        if (rc == octets) {
                octets = hdr->octets - sizeof(*hdr);
-               rc = write(c->req_write_pipe, data, octets);
-
+               rc = netwrite(c->req_write_pipe, data, octets);
                if (rc == octets)
                        return 0;
        }
 
-       if (rc < 0)
-               msyslog(LOG_ERR,
-                       "send_blocking_req_internal: pipe write: %m");
-       else
-               msyslog(LOG_ERR,
-                       "send_blocking_req_internal: short write %d of %d",
-                       rc, octets);
+       msyslog(LOG_ERR,
+               "send_blocking_req_internal: short write (%zu of %zu), %m",
+               rc, octets);
 
        /* Fatal error.  Clean up the child process.  */
        req_child_exit(c);
@@ -244,41 +295,32 @@ receive_blocking_req_internal(
 {
        blocking_pipe_header    hdr;
        blocking_pipe_header *  req;
-       int                     rc;
-       long                    octets;
+       size_t                  rc;
+       size_t                  octets;
 
        DEBUG_REQUIRE(-1 != c->req_read_pipe);
 
        req = NULL;
+       rc = netread(c->req_read_pipe, &hdr, sizeof(hdr));
 
-       do {
-               rc = read(c->req_read_pipe, &hdr, sizeof(hdr));
-       } while (rc < 0 && EINTR == errno);
-
-       if (rc < 0) {
-               msyslog(LOG_ERR,
-                       "receive_blocking_req_internal: pipe read %m");
-       } else if (0 == rc) {
+       if (0 == rc) {
                TRACE(4, ("parent closed request pipe, child %d terminating\n",
                          c->pid));
        } else if (rc != sizeof(hdr)) {
                msyslog(LOG_ERR,
-                       "receive_blocking_req_internal: short header read %d of %lu",
-                       rc, (u_long)sizeof(hdr));
+                       "receive_blocking_req_internal: short header read (%zu of %zu), %m",
+                       rc, sizeof(hdr));
        } else {
                INSIST(sizeof(hdr) < hdr.octets && hdr.octets < 4 * 1024);
                req = emalloc(hdr.octets);
                memcpy(req, &hdr, sizeof(*req));
                octets = hdr.octets - sizeof(hdr);
-               rc = read(c->req_read_pipe, (char *)req + sizeof(*req),
-                         octets);
+               rc = netread(c->req_read_pipe, (char *)(req + 1),
+                            octets);
 
-               if (rc < 0)
-                       msyslog(LOG_ERR,
-                               "receive_blocking_req_internal: pipe data read %m");
-               else if (rc != octets)
+               if (rc != octets)
                        msyslog(LOG_ERR,
-                               "receive_blocking_req_internal: short read %d of %ld",
+                               "receive_blocking_req_internal: short read (%zu of %zu), %m",
                                rc, octets);
                else if (BLOCKING_REQ_MAGIC != req->magic_sig)
                        msyslog(LOG_ERR,
@@ -301,24 +343,20 @@ send_blocking_resp_internal(
        blocking_pipe_header *  resp
        )
 {
-       long    octets;
-       int     rc;
+       size_t  octets;
+       size_t  rc;
 
        DEBUG_REQUIRE(-1 != c->resp_write_pipe);
 
        octets = resp->octets;
-       rc = write(c->resp_write_pipe, resp, octets);
+       rc = netwrite(c->resp_write_pipe, resp, octets);
        free(resp);
 
        if (octets == rc)
                return 0;
 
-       if (rc < 0)
-               TRACE(1, ("send_blocking_resp_internal: pipe write %m\n"));
-       else
-               TRACE(1, ("send_blocking_resp_internal: short write %d of %ld\n",
-                         rc, octets));
-
+       TRACE(1, ("send_blocking_resp_internal: short write (%zu of %zu), %m\n",
+                 rc, octets));
        return -1;
 }
 
@@ -330,21 +368,19 @@ receive_blocking_resp_internal(
 {
        blocking_pipe_header    hdr;
        blocking_pipe_header *  resp;
-       int                     rc;
-       long                    octets;
+       size_t                  rc;
+       size_t                  octets;
 
        DEBUG_REQUIRE(c->resp_read_pipe != -1);
 
        resp = NULL;
-       rc = read(c->resp_read_pipe, &hdr, sizeof(hdr));
+       rc = netread(c->resp_read_pipe, &hdr, sizeof(hdr));
 
-       if (rc < 0) {
-               TRACE(1, ("receive_blocking_resp_internal: pipe read %m\n"));
-       } else if (0 == rc) {
+       if (0 == rc) {
                /* this is the normal child exited indication */
        } else if (rc != sizeof(hdr)) {
-               TRACE(1, ("receive_blocking_resp_internal: short header read %d of %lu\n",
-                         rc, (u_long)sizeof(hdr)));
+               TRACE(1, ("receive_blocking_resp_internal: short header read (%zu of %zu), %m\n",
+                         rc, sizeof(hdr)));
        } else if (BLOCKING_RESP_MAGIC != hdr.magic_sig) {
                TRACE(1, ("receive_blocking_resp_internal: header mismatch (0x%x)\n",
                          hdr.magic_sig));
@@ -354,24 +390,21 @@ receive_blocking_resp_internal(
                resp = emalloc(hdr.octets);
                memcpy(resp, &hdr, sizeof(*resp));
                octets = hdr.octets - sizeof(hdr);
-               rc = read(c->resp_read_pipe,
-                         (char *)resp + sizeof(*resp),
-                         octets);
+               rc = netread(c->resp_read_pipe, (char *)(resp + 1),
+                            octets);
 
-               if (rc < 0)
-                       TRACE(1, ("receive_blocking_resp_internal: pipe data read %m\n"));
-               else if (rc < octets)
-                       TRACE(1, ("receive_blocking_resp_internal: short read %d of %ld\n",
+               if (rc != octets)
+                       TRACE(1, ("receive_blocking_resp_internal: short read (%zu of %zu), %m\n",
                                  rc, octets));
                else
                        return resp;
        }
 
        cleanup_after_child(c);
-
+       
        if (resp != NULL)
                free(resp);
-
+       
        return NULL;
 }
 

diff --git a/ntpd/ntp_proto.c b/ntpd/ntp_proto.c
index ad04ed4ac0d7f50d784c10802fc9cc2e43fe74da..80d2619979a878c1da41e6a05b25b05f2eabd79b 100644 (file)
--- a/ntpd/ntp_proto.c
+++ b/ntpd/ntp_proto.c
@@ -1691,7 +1691,7 @@ receive(
 
                if (0) {
                } else if (L_ISZERO(&p_org)) {
-                       char *action;
+                       const char *action;
 
                        L_CLR(&peer->aorg);
                        /**/
@@ -1712,6 +1712,7 @@ receive(
                                peer->flash |= TEST2;   /* bogus */
                                break;
                            default:
+                               action = "";    /* for cranky compilers / MSVC */
                                INSIST(!"receive(): impossible hismode");
                                break;
                        }