Avoid assuming that time_t can fit in an int.

We had several places that used cast-to-unsigned-int as a substitute
for properly checking for overflow.  Coverity has started objecting
to that practice as likely introducing Y2038 bugs.  An extra
comparison is surely not much compared to the cost of time(NULL), nor
is this coding practice particularly readable.  Let's do it honestly,
with explicit logic covering the cases of first-time-through and
clock-went-backwards.

I don't feel a need to back-patch though: our released versions
will be out of support long before 2038, and besides which I think
the code would accidentally work anyway for another 70 years or so.

diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 78e39e5f866a744b0d4b744eb33c735b9b3bd8c7..ce6b5299324725f191284e8f7bc5a42a15afe15e 100644 (file)
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -185,8 +185,8 @@ PgArchShmemInit(void)
 /*
  * PgArchCanRestart
  *
- * Return true and archiver is allowed to restart if enough time has
- * passed since it was launched last to reach PGARCH_RESTART_INTERVAL.
+ * Return true, indicating archiver is allowed to restart, if enough time has
+ * passed since it was last launched to reach PGARCH_RESTART_INTERVAL.
  * Otherwise return false.
  *
  * This is a safety valve to protect against continuous respawn attempts if the
@@ -201,15 +201,18 @@ PgArchCanRestart(void)
    time_t      curtime = time(NULL);
 
    /*
-    * Return false and don't restart archiver if too soon since last archiver
-    * start.
+    * If first time through, or time somehow went backwards, always update
+    * last_pgarch_start_time to match the current clock and allow archiver
+    * start.  Otherwise allow it only once enough time has elapsed.
     */
-   if ((unsigned int) (curtime - last_pgarch_start_time) <
-       (unsigned int) PGARCH_RESTART_INTERVAL)
-       return false;
-
-   last_pgarch_start_time = curtime;
-   return true;
+   if (last_pgarch_start_time == 0 ||
+       curtime < last_pgarch_start_time ||
+       curtime - last_pgarch_start_time >= PGARCH_RESTART_INTERVAL)
+   {
+       last_pgarch_start_time = curtime;
+       return true;
+   }
+   return false;
 }
 
 
@@ -332,7 +335,8 @@ pgarch_MainLoop(void)
         * SIGUSR2 arrives.  However, that means a random SIGTERM would
         * disable archiving indefinitely, which doesn't seem like a good
         * idea.  If more than 60 seconds pass since SIGTERM, exit anyway, so
-        * that the postmaster can start a new archiver if needed.
+        * that the postmaster can start a new archiver if needed.  Also exit
+        * if time unexpectedly goes backward.
         */
        if (ShutdownRequestPending)
        {
@@ -340,8 +344,8 @@ pgarch_MainLoop(void)
 
            if (last_sigterm_time == 0)
                last_sigterm_time = curtime;
-           else if ((unsigned int) (curtime - last_sigterm_time) >=
-                    (unsigned int) 60)
+           else if (curtime < last_sigterm_time ||
+                    curtime - last_sigterm_time >= 60)
                break;
        }
 

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e1d643b013d77caf3ac51a52311c0a561d981d8f..00de559ba8f41f489efabf93902290e6e6d3a78d 100644 (file)
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1557,13 +1557,21 @@ DetermineSleepTime(void)
    {
        if (AbortStartTime != 0)
        {
+           time_t      curtime = time(NULL);
            int         seconds;
 
-           /* time left to abort; clamp to 0 in case it already expired */
-           seconds = SIGKILL_CHILDREN_AFTER_SECS -
-               (time(NULL) - AbortStartTime);
+           /*
+            * time left to abort; clamp to 0 if it already expired, or if
+            * time goes backwards
+            */
+           if (curtime < AbortStartTime ||
+               curtime - AbortStartTime >= SIGKILL_CHILDREN_AFTER_SECS)
+               seconds = 0;
+           else
+               seconds = SIGKILL_CHILDREN_AFTER_SECS -
+                   (curtime - AbortStartTime);
 
-           return Max(seconds * 1000, 0);
+           return seconds * 1000;
        }
        else
            return 60 * 1000;

diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 8c061d55bdb51d9f236ad153afebd305ba09a229..b122d99b0097cf67ea7dbaef5e3366df1d49ca7e 100644 (file)
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1636,8 +1636,9 @@ ShutDownSlotSync(void)
 /*
  * SlotSyncWorkerCanRestart
  *
- * Returns true if enough time (SLOTSYNC_RESTART_INTERVAL_SEC) has passed
- * since it was launched last. Otherwise returns false.
+ * Return true, indicating worker is allowed to restart, if enough time has
+ * passed since it was last launched to reach SLOTSYNC_RESTART_INTERVAL_SEC.
+ * Otherwise return false.
  *
  * This is a safety valve to protect against continuous respawn attempts if the
  * worker is dying immediately at launch. Note that since we will retry to
@@ -1649,14 +1650,19 @@ SlotSyncWorkerCanRestart(void)
 {
    time_t      curtime = time(NULL);
 
-   /* Return false if too soon since last start. */
-   if ((unsigned int) (curtime - SlotSyncCtx->last_start_time) <
-       (unsigned int) SLOTSYNC_RESTART_INTERVAL_SEC)
-       return false;
-
-   SlotSyncCtx->last_start_time = curtime;
-
-   return true;
+   /*
+    * If first time through, or time somehow went backwards, always update
+    * last_start_time to match the current clock and allow worker start.
+    * Otherwise allow it only once enough time has elapsed.
+    */
+   if (SlotSyncCtx->last_start_time == 0 ||
+       curtime < SlotSyncCtx->last_start_time ||
+       curtime - SlotSyncCtx->last_start_time >= SLOTSYNC_RESTART_INTERVAL_SEC)
+   {
+       SlotSyncCtx->last_start_time = curtime;
+       return true;
+   }
+   return false;
 }
 
 /*