From b45d911cc631a2922a4290e1b070de657b5cd3e5 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 15:36:09 +0000 Subject: [PATCH 1/6] gio-test: fix leak & maybe-uninitialized warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GCC 8 on F29 complains: ../tests/gio-test.c: In function ‘main’: ../tests/gio-test.c:375:7: warning: ‘id’ may be used uninitialized in this function [-Wmaybe-uninitialized] g_free (id); ^~~~~~~~~~~ In the normal case, when run without arguments, 'id' will be assigned exactly once, so all is fine. If run with argument '0', 'id' will never be assigned, so the warning is legit; but in that case the test program will never exit. If run with any argument greater than 1, 'id' will be assigned more than once but only the last incarnation will be freed. Tweak the scope of the variable to match its use, and arrange for it to be freed when its watch is destroyed. --- tests/gio-test.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/gio-test.c b/tests/gio-test.c index d9bd7e357..79b53e294 100644 --- a/tests/gio-test.c +++ b/tests/gio-test.c @@ -266,7 +266,6 @@ main (int argc, GIOChannel *my_read_channel; gchar *cmdline; - guint *id; int i; #ifdef G_OS_WIN32 GTimeVal start, end; @@ -316,6 +315,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 +328,11 @@ main (int argc, id = g_new (guint, 1); *id = - g_io_add_watch (my_read_channel, - G_IO_IN | G_IO_PRI | G_IO_ERR | G_IO_HUP, - recv_message, - id); + 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, g_free); nrunning++; @@ -372,7 +373,6 @@ main (int argc, g_main_loop_unref (main_loop); g_free (seqtab); - g_free (id); } else if (argc == 3) { From 6debbbfd733c8af8ba63a9a901299671875d9108 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 15:47:14 +0000 Subject: [PATCH 2/6] gio-test: use g_debug rather than #define VERBOSE --- tests/gio-test.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/tests/gio-test.c b/tests/gio-test.c index 79b53e294..03472ac3d 100644 --- a/tests/gio-test.c +++ b/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_print ("gio-test: ...from %d:%s%s%s%s\n", fd, + g_debug ("gio-test: ...from %d:%s%s%s%s", 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_print ("gio-test: ...from %d: EOF\n", fd); -#endif + g_debug ("gio-test: ...from %d: EOF", fd); shutdown_source (data); return FALSE; } @@ -165,9 +161,7 @@ recv_message (GIOChannel *channel, if (nb == 0) { -#ifdef VERBOSE - g_print ("gio-test: ...from %d: EOF\n", fd); -#endif + g_debug ("gio-test: ...from %d: EOF", fd); 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_print ("gio-test: ...from %d: %d bytes\n", fd, nbytes); -#endif + g_debug ("gio-test: ...from %d: %d bytes", fd, nbytes); 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_print ("gio-test: ...from %d: EOF\n", fd); -#endif + g_debug ("gio-test: ...from %d: EOF", fd); shutdown_source (data); return FALSE; } for (j = 0; j < nbytes; j++) g_assert (buf[j] == ' ' + ((nbytes + j) % 95)); -#ifdef VERBOSE - g_print ("gio-test: ...from %d: OK\n", fd); -#endif + g_debug ("gio-test: ...from %d: OK", fd); } } return retval; @@ -404,10 +392,8 @@ main (int argc, buflen = rand() % BUFSIZE; for (j = 0; j < buflen; j++) buf[j] = ' ' + ((buflen + j) % 95); -#ifdef VERBOSE - g_print ("gio-test: child writing %d+%d bytes to %d\n", + g_debug ("gio-test: child writing %d+%d bytes to %d", (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_print ("gio-test: child exiting, closing %d\n", writefd); -#endif + g_debug ("gio-test: child exiting, closing %d", writefd); close (writefd); } else From 4ff3734ff5748cbc53f7b89c7148a4e3daba3230 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 16:04:10 +0000 Subject: [PATCH 3/6] g_timeout_*_seconds: don't overflow for large intervals Previously, the `guint interval` parameter, measured in seconds, was multiplied by 1000 and stored in another `guint` field. For intervals greater than (G_MAXUINT / 1000) seconds, this would overflow; the timeout would fire much sooner than intended. Since GTimeoutSource already keeps track of whether it was created at millisecond or second resolution, always store the passed interval directly. We later convert the interval to microseconds, stored in a gint64, so can move the `* 1000` to there. The eagle-eyed reader might notice that there is no obvious guarantee that the source's expiration time in microseconds won't overflow the gint64, but I don't think this is a new problem. Previously, the monotonic time would have to reach (2 ** 63 - 2 ** 32) microseconds for this overflow to occur; now it would have to reach approximately (2 ** 63 - 2 ** 42) microseconds. Both of these are 292.47 millennia to 5 significant figures. Fixes #1600. --- glib/gmain.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 5e849b9ac..347882551 100644 --- a/glib/gmain.c +++ b/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; }; @@ -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 ()); From 6490fe7fe8d8b6f9b5f05fcc1660deef429772d0 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 16:49:04 +0000 Subject: [PATCH 4/6] g_timeout_*_seconds: test an interval that overflowed This is essentially a C version of the reproducer on #1600. It is based on the existing test_seconds(), which relates to a similar but distinct overflow. I've only actually run this on a system with 32-bit ints, it should work regardless of the width of an int, since the remainder after wrapping will by construction be less than 1 second. --- glib/tests/timeout.c | 47 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/glib/tests/timeout.c b/glib/tests/timeout.c index ce7dd0965..cbbab7ff2 100644 --- a/glib/tests/timeout.c +++ b/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,52 @@ 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); + + 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); @@ -102,8 +141,10 @@ 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/rounding", test_rounding); return g_test_run (); From cdc2dd8eb15627a6c52724dcc2930300153fbfd4 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 20:38:55 +0000 Subject: [PATCH 5/6] gmain: clamp over-large timeouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit g_main_context_prepare() needs to calculate the timeout to pass to poll(), expressed in milliseconds as a gint. But since the ready time for a GSource is represented by gint64 microseconds, it's possible that it could be more than G_MAXINT * 1000 microseconds in the future, and so can't be represented as a gint. This conversion to a narrower signed type is implementation-defined, but there are two possible outcomes: * the result is >= 0, in which case poll() will time out earlier than we might hope (with no adverse consequences besides an unwanted wakeup) * the result is < 0, in which case, if there are no other sources, poll() will block forever This is extremely unlikely to happen in practice, but can be avoided by clamping the gint64 value, which we know to be positive, to G_MAXINT. Thanks to Tomasz Miąsko for pointing this out on !496. --- glib/gmain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glib/gmain.c b/glib/gmain.c index 347882551..c24690e44 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -3525,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); } } From 4544dcb78897ccad7d7fdd2a49ac9653c3349b08 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Mon, 26 Nov 2018 21:07:20 +0000 Subject: [PATCH 6/6] gmain: test far-future ready_time --- glib/tests/timeout.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/glib/tests/timeout.c b/glib/tests/timeout.c index cbbab7ff2..8f44e7552 100644 --- a/glib/tests/timeout.c +++ b/glib/tests/timeout.c @@ -91,6 +91,53 @@ test_weeks_overflow (void) 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; @@ -145,6 +192,7 @@ main (int argc, char *argv[]) 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 ();