Merge branch '1600-g_timeout_add_seconds-overflow' into 'master'

Resolve "g_timeout_source_new_seconds overflows when given interval > (G_MAXUINT / 1000)"

Closes #1600

See merge request GNOME/glib!496

glib/gmain.c

@@ -315,6 +315,7 @@ struct _GMainLoop
 struct _GTimeoutSource
 {
   GSource     source;
+  /* Measured in seconds if 'seconds' is TRUE, or milliseconds otherwise. */
   guint       interval;
   gboolean    seconds;
 };
@@ -3524,13 +3525,13 @@ g_main_context_prepare (GMainContext *context,
                 }
               else
                 {
-                  gint timeout;
+                  gint64 timeout;
 
                   /* rounding down will lead to spinning, so always round up */
                   timeout = (source->priv->ready_time - context->time + 999) / 1000;
 
                   if (source_timeout < 0 || timeout < source_timeout)
-                    source_timeout = timeout;
+                    source_timeout = MIN (timeout, G_MAXINT);
                 }
             }
 
@@ -4615,8 +4616,6 @@ g_timeout_set_expiration (GTimeoutSource *timeout_source,
 {
   gint64 expiration;
 
-  expiration = current_time + (guint64) timeout_source->interval * 1000;
-
   if (timeout_source->seconds)
     {
       gint64 remainder;
@@ -4638,6 +4637,8 @@ g_timeout_set_expiration (GTimeoutSource *timeout_source,
             timer_perturb = 0;
         }
 
+      expiration = current_time + (guint64) timeout_source->interval * 1000 * 1000;
+
       /* We want the microseconds part of the timeout to land on the
        * 'timer_perturb' mark, but we need to make sure we don't try to
        * set the timeout in the past.  We do this by ensuring that we
@@ -4653,6 +4654,10 @@ g_timeout_set_expiration (GTimeoutSource *timeout_source,
       expiration -= remainder;
       expiration += timer_perturb;
     }
+  else
+    {
+      expiration = current_time + (guint64) timeout_source->interval * 1000;
+    }
 
   g_source_set_ready_time ((GSource *) timeout_source, expiration);
 }
@@ -4735,7 +4740,7 @@ g_timeout_source_new_seconds (guint interval)
   GSource *source = g_source_new (&g_timeout_funcs, sizeof (GTimeoutSource));
   GTimeoutSource *timeout_source = (GTimeoutSource *)source;
 
-  timeout_source->interval = 1000 * interval;
+  timeout_source->interval = interval;
   timeout_source->seconds = TRUE;
 
   g_timeout_set_expiration (timeout_source, g_get_monotonic_time ());

glib/tests/timeout.c

@@ -14,7 +14,7 @@ stop_waiting (gpointer data)
 }
 
 static gboolean
-function (gpointer data)
+unreachable_callback (gpointer data)
 {
   g_assert_not_reached ();
 
@@ -38,13 +38,14 @@ test_seconds (void)
    * then we have trouble (since it ran in less than 2 seconds).
    *
    * We need a timeout of at least 2 seconds because
-   * g_timeout_add_second can add as much as an additional second of
+   * g_timeout_add_seconds() can add as much as an additional second of
    * latency.
    */
+  g_test_bug ("https://bugzilla.gnome.org/show_bug.cgi?id=642052");
   loop = g_main_loop_new (NULL, FALSE);
 
   g_timeout_add (2100, stop_waiting, NULL);
-  id = g_timeout_add_seconds (21475, function, NULL);
+  id = g_timeout_add_seconds (21475, unreachable_callback, NULL);
 
   g_main_loop_run (loop);
   g_main_loop_unref (loop);
@@ -52,6 +53,91 @@ test_seconds (void)
   g_source_remove (id);
 }
 
+static void
+test_weeks_overflow (void)
+{
+  guint id;
+  guint interval_seconds;
+
+  /* Internally, the guint interval (in seconds) was converted to milliseconds
+   * then stored in a guint variable. This meant that any interval larger than
+   * G_MAXUINT / 1000 would overflow.
+   *
+   * On a system with 32-bit guint, the interval (G_MAXUINT / 1000) + 1 seconds
+   * (49.7 days) would end wrapping to 704 milliseconds.
+   *
+   * Test that that isn't true anymore by scheduling two jobs:
+   *   - one, as above
+   *   - another that runs in 2100ms
+   *
+   * If everything is working properly, the 2100ms one should run first
+   * (and exit the mainloop).  If we ever see the other job run
+   * then we have trouble (since it ran in less than 2 seconds).
+   *
+   * We need a timeout of at least 2 seconds because
+   * g_timeout_add_seconds() can add as much as an additional second of
+   * latency.
+   */
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/1600");
+  loop = g_main_loop_new (NULL, FALSE);
+
+  g_timeout_add (2100, stop_waiting, NULL);
+  interval_seconds = 1 + G_MAXUINT / 1000;
+  id = g_timeout_add_seconds (interval_seconds, unreachable_callback, NULL);
+
+  g_main_loop_run (loop);
+  g_main_loop_unref (loop);
+
+  g_source_remove (id);
+}
+
+/* The ready_time for a GSource is stored as a gint64, as an absolute monotonic
+ * time in microseconds. To call poll(), this must be converted to a relative
+ * timeout, in milliseconds, as a gint. If the ready_time is sufficiently far
+ * in the future, the timeout will not fit. Previously, it would be narrowed in
+ * an implementation-defined way; if this gave a negative result, poll() would
+ * block forever.
+ *
+ * This test creates a GSource with the largest possible ready_time (a little
+ * over 292 millennia, assuming g_get_monotonic_time() starts from near 0 when
+ * the system boots), adds it to a GMainContext, queries it for the parameters
+ * to pass to poll() -- essentially the first half of
+ * g_main_context_iteration() -- and checks that the timeout is a large
+ * positive number.
+ */
+static void
+test_far_future_ready_time (void)
+{
+  GSourceFuncs source_funcs = { 0 };
+  GMainContext *context = g_main_context_new ();
+  GSource *source = g_source_new (&source_funcs, sizeof (GSource));
+  gboolean acquired, ready;
+  gint priority, timeout_, n_fds;
+
+  g_source_set_ready_time (source, G_MAXINT64);
+  g_source_attach (source, context);
+
+  acquired = g_main_context_acquire (context);
+  g_assert_true (acquired);
+
+  ready = g_main_context_prepare (context, &priority);
+  g_assert_false (ready);
+
+  n_fds = 0;
+  n_fds = g_main_context_query (context, priority, &timeout_, NULL, n_fds);
+
+  /* The true timeout in milliseconds doesn't fit into a gint. We definitely
+   * don't want poll() to block forever:
+   */
+  g_assert_cmpint (timeout_, >=, 0);
+  /* Instead, we want it to block for as long as possible: */
+  g_assert_cmpint (timeout_, ==, G_MAXINT);
+
+  g_main_context_release (context);
+  g_main_context_unref (context);
+  g_source_unref (source);
+}
+
 static gint64 last_time;
 static gint count;
 
@@ -102,8 +188,11 @@ int
 main (int argc, char *argv[])
 {
   g_test_init (&argc, &argv, NULL);
+  g_test_bug_base ("");
 
   g_test_add_func ("/timeout/seconds", test_seconds);
+  g_test_add_func ("/timeout/weeks-overflow", test_weeks_overflow);
+  g_test_add_func ("/timeout/far-future-ready-time", test_far_future_ready_time);
   g_test_add_func ("/timeout/rounding", test_rounding);
 
   return g_test_run ();

tests/gio-test.c

@@ -113,13 +113,11 @@ recv_message (GIOChannel  *channel,
   gint fd = g_io_channel_unix_get_fd (channel);
   gboolean retval = TRUE;
 
-#ifdef VERBOSE
+  g_debug ("gio-test: ...from %d:%s%s%s%s", fd,
-  g_print ("gio-test: ...from %d:%s%s%s%s\n", fd,
 	   (cond & G_IO_ERR) ? " ERR" : "",
 	   (cond & G_IO_HUP) ? " HUP" : "",
 	   (cond & G_IO_IN)  ? " IN"  : "",
 	   (cond & G_IO_PRI) ? " PRI" : "");
-#endif
 
   if (cond & (G_IO_ERR | G_IO_HUP))
     {
@@ -140,9 +138,7 @@ recv_message (GIOChannel  *channel,
 	{
 	  if (nb == 0)
 	    {
-#ifdef VERBOSE
+	      g_debug ("gio-test: ...from %d: EOF", fd);
-	      g_print ("gio-test: ...from %d: EOF\n", fd);
-#endif
 	      shutdown_source (data);
 	      return FALSE;
 	    }
@@ -165,9 +161,7 @@ recv_message (GIOChannel  *channel,
       
       if (nb == 0)
 	{
-#ifdef VERBOSE
+	  g_debug ("gio-test: ...from %d: EOF", fd);
-	  g_print ("gio-test: ...from %d: EOF\n", fd);
-#endif
 	  shutdown_source (data);
 	  return FALSE;
 	}
@@ -176,9 +170,7 @@ recv_message (GIOChannel  *channel,
 
       g_assert_cmpint (nbytes, <, BUFSIZE);
       g_assert (nbytes >= 0 && nbytes < BUFSIZE);
-#ifdef VERBOSE      
+      g_debug ("gio-test: ...from %d: %d bytes", fd, nbytes);
-      g_print ("gio-test: ...from %d: %d bytes\n", fd, nbytes);
-#endif      
       if (nbytes > 0)
 	{
 	  error = read_all (fd, channel, buf, nbytes, &nb);
@@ -188,18 +180,14 @@ recv_message (GIOChannel  *channel,
 
 	  if (nb == 0)
 	    {
-#ifdef VERBOSE
+	      g_debug ("gio-test: ...from %d: EOF", fd);
-	      g_print ("gio-test: ...from %d: EOF\n", fd);
-#endif
 	      shutdown_source (data);
 	      return FALSE;
 	    }
       
 	  for (j = 0; j < nbytes; j++)
             g_assert (buf[j] == ' ' + ((nbytes + j) % 95));
-#ifdef VERBOSE
+	  g_debug ("gio-test: ...from %d: OK", fd);
-	  g_print ("gio-test: ...from %d: OK\n", fd);
-#endif
 	}
     }
   return retval;
@@ -266,7 +254,6 @@ main (int    argc,
       
       GIOChannel *my_read_channel;
       gchar *cmdline;
-      guint *id;
       int i;
 #ifdef G_OS_WIN32
       GTimeVal start, end;
@@ -316,6 +303,7 @@ main (int    argc,
       for (i = 0; i < nkiddies; i++)
 	{
 	  int pipe_to_sub[2], pipe_from_sub[2];
+	  guint *id;
 	  
 	  if (pipe (pipe_to_sub) == -1 ||
 	      pipe (pipe_from_sub) == -1)
@@ -328,10 +316,11 @@ main (int    argc,
 	  
 	  id = g_new (guint, 1);
 	  *id =
-	    g_io_add_watch (my_read_channel,
+	    g_io_add_watch_full (my_read_channel,
+				 G_PRIORITY_DEFAULT,
 				 G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP,
 				 recv_message,
-			    id);
+				 id, g_free);
 	  
 	  nrunning++;
 	  
@@ -372,7 +361,6 @@ main (int    argc,
 
       g_main_loop_unref (main_loop);
       g_free (seqtab);
-      g_free (id);
     }
   else if (argc == 3)
     {
@@ -404,10 +392,8 @@ main (int    argc,
 	  buflen = rand() % BUFSIZE;
 	  for (j = 0; j < buflen; j++)
 	    buf[j] = ' ' + ((buflen + j) % 95);
-#ifdef VERBOSE
+	  g_debug ("gio-test: child writing %d+%d bytes to %d",
-	  g_print ("gio-test: child writing %d+%d bytes to %d\n",
 		   (int)(sizeof(i) + sizeof(buflen)), buflen, writefd);
-#endif
 	  write (writefd, &i, sizeof (i));
 	  write (writefd, &buflen, sizeof (buflen));
 	  write (writefd, buf, buflen);
@@ -424,9 +410,7 @@ main (int    argc,
 	    }
 #endif
 	}
-#ifdef VERBOSE
+      g_debug ("gio-test: child exiting, closing %d", writefd);
-      g_print ("gio-test: child exiting, closing %d\n", writefd);
-#endif
       close (writefd);
     }
   else