From 5b0fdfda6df931878ce9a44c514058af849c9136 Mon Sep 17 00:00:00 2001 From: Patrick Griffis Date: Thu, 7 Feb 2019 09:50:56 -0500 Subject: [PATCH 1/3] gnetworkaddress: Fix incorrect error propagation when resolving addresses Previously this would always error if ipv6 errored after ipv4 succeeded which was incorrect. This explicitly tests the order of erroring. Fixes #1644 --- gio/gnetworkaddress.c | 27 +++++++++----- gio/tests/network-address.c | 70 +++++++++++++++++++++++++++++++------ 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/gio/gnetworkaddress.c b/gio/gnetworkaddress.c index 60736874e..e81d311ce 100644 --- a/gio/gnetworkaddress.c +++ b/gio/gnetworkaddress.c @@ -38,6 +38,10 @@ #include +/* As recommended by RFC 8305 this is the time it waits for a following + DNS response to come in (ipv4 waiting on ipv6 generally) + */ +#define HAPPY_EYEBALLS_RESOLUTION_DELAY_MS 50 /** * SECTION:gnetworkaddress @@ -1143,6 +1147,7 @@ got_ipv6_addresses (GObject *source_object, GResolver *resolver = G_RESOLVER (source_object); GList *addresses; GError *error = NULL; + gboolean got_ipv4_first = FALSE; addresses = g_resolver_lookup_by_name_with_flags_finish (resolver, result, &error); if (!error) @@ -1161,18 +1166,18 @@ got_ipv6_addresses (GObject *source_object, { g_source_destroy (addr_enum->wait_source); g_clear_pointer (&addr_enum->wait_source, g_source_unref); + got_ipv4_first = TRUE; } - /* If we got an error before ipv4 then let it handle it. + /* If we got an error before ipv4 then let its response handle it. * If we get ipv6 response first or error second then * immediately complete the task. */ - if (error != NULL && !addr_enum->last_error) + if (error != NULL && !addr_enum->last_error && !got_ipv4_first) { addr_enum->last_error = g_steal_pointer (&error); - /* This shouldn't happen often but avoid never responding. */ - addr_enum->wait_source = g_timeout_source_new_seconds (1); + addr_enum->wait_source = g_timeout_source_new (HAPPY_EYEBALLS_RESOLUTION_DELAY_MS); g_source_set_callback (addr_enum->wait_source, on_address_timeout, addr_enum, NULL); @@ -1180,13 +1185,19 @@ got_ipv6_addresses (GObject *source_object, } else if (addr_enum->queued_task != NULL) { + GError *task_error = NULL; + + /* If both errored just use the ipv6 one, + but if ipv6 errored and ipv4 didn't we don't error */ + if (error != NULL && addr_enum->last_error) + task_error = g_steal_pointer (&error); + g_clear_error (&addr_enum->last_error); complete_queued_task (addr_enum, g_steal_pointer (&addr_enum->queued_task), - g_steal_pointer (&error)); + g_steal_pointer (&task_error)); } - else if (error != NULL) - g_clear_error (&error); + g_clear_error (&error); g_object_unref (addr_enum); } @@ -1229,7 +1240,7 @@ got_ipv4_addresses (GObject *source_object, else if (addr_enum->queued_task != NULL) { addr_enum->last_error = g_steal_pointer (&error); - addr_enum->wait_source = g_timeout_source_new (50); + addr_enum->wait_source = g_timeout_source_new (HAPPY_EYEBALLS_RESOLUTION_DELAY_MS); g_source_set_callback (addr_enum->wait_source, on_address_timeout, addr_enum, NULL); diff --git a/gio/tests/network-address.c b/gio/tests/network-address.c index 4a2718e24..d41d1a168 100644 --- a/gio/tests/network-address.c +++ b/gio/tests/network-address.c @@ -710,17 +710,18 @@ test_happy_eyeballs_slow_connection_and_ipv4 (HappyEyeballsFixture *fixture, } static void -test_happy_eyeballs_ipv6_error (HappyEyeballsFixture *fixture, - gconstpointer user_data) +test_happy_eyeballs_ipv6_error_ipv4_first (HappyEyeballsFixture *fixture, + gconstpointer user_data) { AsyncData data = { 0 }; GError *ipv6_error; - /* If ipv6 fails we still get ipv4. */ + /* If ipv6 fails, ensuring that ipv4 finishes before ipv6 errors, we still get ipv4. */ data.loop = fixture->loop; ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken"); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -731,17 +732,62 @@ test_happy_eyeballs_ipv6_error (HappyEyeballsFixture *fixture, } static void -test_happy_eyeballs_ipv4_error (HappyEyeballsFixture *fixture, - gconstpointer user_data) +test_happy_eyeballs_ipv6_error_ipv6_first (HappyEyeballsFixture *fixture, + gconstpointer user_data) +{ + AsyncData data = { 0 }; + GError *ipv6_error; + + /* If ipv6 fails, ensuring that ipv6 errors before ipv4 finishes, we still get ipv4. */ + + data.loop = fixture->loop; + ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken"); + mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); + + g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); + g_main_loop_run (fixture->loop); + + assert_list_matches_expected (data.addrs, fixture->input_ipv4_results); + + g_error_free (ipv6_error); +} + +static void +test_happy_eyeballs_ipv4_error_ipv4_first (HappyEyeballsFixture *fixture, + gconstpointer user_data) { AsyncData data = { 0 }; GError *ipv4_error; - /* If ipv4 fails we still get ipv6. */ + /* If ipv4 fails, ensuring that ipv4 errors before ipv6 finishes, we still get ipv6. */ data.loop = fixture->loop; ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken"); mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); + + g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); + g_main_loop_run (fixture->loop); + + assert_list_matches_expected (data.addrs, fixture->input_ipv6_results); + + g_error_free (ipv4_error); +} + +static void +test_happy_eyeballs_ipv4_error_ipv6_first (HappyEyeballsFixture *fixture, + gconstpointer user_data) +{ + AsyncData data = { 0 }; + GError *ipv4_error; + + /* If ipv4 fails, ensuring that ipv6 finishes before ipv4 errors, we still get ipv6. */ + + data.loop = fixture->loop; + ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken"); + mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -913,10 +959,14 @@ main (int argc, char *argv[]) happy_eyeballs_setup, test_happy_eyeballs_very_slow_ipv6, happy_eyeballs_teardown); g_test_add ("/network-address/happy-eyeballs/slow-connection-and-ipv4", HappyEyeballsFixture, NULL, happy_eyeballs_setup, test_happy_eyeballs_slow_connection_and_ipv4, happy_eyeballs_teardown); - g_test_add ("/network-address/happy-eyeballs/ipv6-error", HappyEyeballsFixture, NULL, - happy_eyeballs_setup, test_happy_eyeballs_ipv6_error, happy_eyeballs_teardown); - g_test_add ("/network-address/happy-eyeballs/ipv4-error", HappyEyeballsFixture, NULL, - happy_eyeballs_setup, test_happy_eyeballs_ipv4_error, happy_eyeballs_teardown); + g_test_add ("/network-address/happy-eyeballs/ipv6-error-ipv4-first", HappyEyeballsFixture, NULL, + happy_eyeballs_setup, test_happy_eyeballs_ipv6_error_ipv4_first, happy_eyeballs_teardown); + g_test_add ("/network-address/happy-eyeballs/ipv6-error-ipv6-first", HappyEyeballsFixture, NULL, + happy_eyeballs_setup, test_happy_eyeballs_ipv6_error_ipv6_first, happy_eyeballs_teardown); + g_test_add ("/network-address/happy-eyeballs/ipv4-error-ipv6-first", HappyEyeballsFixture, NULL, + happy_eyeballs_setup, test_happy_eyeballs_ipv4_error_ipv6_first, happy_eyeballs_teardown); + g_test_add ("/network-address/happy-eyeballs/ipv4-error-ipv4-first", HappyEyeballsFixture, NULL, + happy_eyeballs_setup, test_happy_eyeballs_ipv4_error_ipv4_first, happy_eyeballs_teardown); g_test_add ("/network-address/happy-eyeballs/both-error", HappyEyeballsFixture, NULL, happy_eyeballs_setup, test_happy_eyeballs_both_error, happy_eyeballs_teardown); g_test_add ("/network-address/happy-eyeballs/both-error-delays-1", HappyEyeballsFixture, NULL, From 5827cef22d304514514b213841de645bd1f92582 Mon Sep 17 00:00:00 2001 From: Patrick Griffis Date: Thu, 7 Feb 2019 09:51:54 -0500 Subject: [PATCH 2/3] tests: Unmark network-address test as flaky --- gio/tests/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/tests/meson.build b/gio/tests/meson.build index 7f47f62b6..ebd04461f 100644 --- a/gio/tests/meson.build +++ b/gio/tests/meson.build @@ -55,7 +55,7 @@ gio_tests = { 'memory-output-stream' : {}, 'monitor' : {}, 'mount-operation' : {}, - 'network-address' : {'extra_sources': ['mock-resolver.c'], 'suite': ['flaky']}, + 'network-address' : {'extra_sources': ['mock-resolver.c']}, 'network-monitor' : {}, 'network-monitor-race' : {}, 'permission' : {}, From ed57faeeda7e79ec13018855609080b0bcee7dca Mon Sep 17 00:00:00 2001 From: Patrick Griffis Date: Thu, 7 Feb 2019 10:10:53 -0500 Subject: [PATCH 3/3] tests: Use fewer magic numbers in network-address tests --- gio/tests/network-address.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/gio/tests/network-address.c b/gio/tests/network-address.c index d41d1a168..0341cccaf 100644 --- a/gio/tests/network-address.c +++ b/gio/tests/network-address.c @@ -622,13 +622,16 @@ happy_eyeballs_teardown (HappyEyeballsFixture *fixture, g_main_loop_unref (fixture->loop); } +static const guint FAST_DELAY_LESS_THAN_TIMEOUT = 25; +static const guint SLOW_DELAY_MORE_THAN_TIMEOUT = 100; + static void test_happy_eyeballs_basic (HappyEyeballsFixture *fixture, gconstpointer user_data) { AsyncData data = { 0 }; - data.delay_ms = 10; + data.delay_ms = FAST_DELAY_LESS_THAN_TIMEOUT; data.loop = fixture->loop; /* This just tests in the common case it gets all results */ @@ -648,7 +651,7 @@ test_happy_eyeballs_slow_ipv4 (HappyEyeballsFixture *fixture, /* If ipv4 dns response is a bit slow we just don't get them */ data.loop = fixture->loop; - mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -665,7 +668,7 @@ test_happy_eyeballs_slow_ipv6 (HappyEyeballsFixture *fixture, /* If ipv6 is a bit slow it waits for them */ data.loop = fixture->loop; - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -682,7 +685,7 @@ test_happy_eyeballs_very_slow_ipv6 (HappyEyeballsFixture *fixture, /* If ipv6 is very slow we don't get them */ data.loop = fixture->loop; - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 200); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, SLOW_DELAY_MORE_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -700,8 +703,8 @@ test_happy_eyeballs_slow_connection_and_ipv4 (HappyEyeballsFixture *fixture, * take long enough. */ data.loop = fixture->loop; - data.delay_ms = 500; - mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 200); + data.delay_ms = SLOW_DELAY_MORE_THAN_TIMEOUT * 2; + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, SLOW_DELAY_MORE_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -721,7 +724,7 @@ test_happy_eyeballs_ipv6_error_ipv4_first (HappyEyeballsFixture *fixture, data.loop = fixture->loop; ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken"); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -743,7 +746,7 @@ test_happy_eyeballs_ipv6_error_ipv6_first (HappyEyeballsFixture *fixture, data.loop = fixture->loop; ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken"); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); - mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -765,7 +768,7 @@ test_happy_eyeballs_ipv4_error_ipv4_first (HappyEyeballsFixture *fixture, data.loop = fixture->loop; ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken"); mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -787,7 +790,7 @@ test_happy_eyeballs_ipv4_error_ipv6_first (HappyEyeballsFixture *fixture, data.loop = fixture->loop; ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken"); mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); - mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -838,7 +841,7 @@ test_happy_eyeballs_both_error_delays_1 (HappyEyeballsFixture *fixture, ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken"); mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); - mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); @@ -866,7 +869,7 @@ test_happy_eyeballs_both_error_delays_2 (HappyEyeballsFixture *fixture, mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, FAST_DELAY_LESS_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop); @@ -893,7 +896,7 @@ test_happy_eyeballs_both_error_delays_3 (HappyEyeballsFixture *fixture, mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error); mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error); - mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 200); + mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, SLOW_DELAY_MORE_THAN_TIMEOUT); g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data); g_main_loop_run (fixture->loop);