[Tor Trac #33375]: Stop advertising an IPv6 exit policy when DNS is broken for IPv6 #1771

neelchauhan · 2020-02-26T23:20:27Z

For the bug here.

coveralls · 2020-02-26T23:43:37Z

Pull Request Test Coverage Report for Build 8526

4 of 18 (22.22%) changed or added relevant lines in 2 files are covered.
2062 unchanged lines in 32 files lost coverage.
Overall coverage increased (+0.3%) to 64.086%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/relay/dns.c	3	17	17.65%

Files with Coverage Reduction	New Missed Lines	%
src/feature/relay/dns.c	1	25.53%
src/lib/confmgt/type_defs.c	2	98.6%
src/core/or/sendme.c	3	73.02%
src/lib/confmgt/unitparse.c	3	95.16%
src/feature/dirparse/microdesc_parse.c	4	95.49%
src/feature/dirparse/parsecommon.c	5	87.67%
src/lib/crypt_ops/crypto_rsa.c	7	91.63%
src/lib/math/prob_distr.c	7	92.62%
src/feature/control/btrack_orconn_maps.c	8	85.71%
src/lib/crypt_ops/crypto_rsa_openssl.c	10	91.7%

Totals
Change from base Build 8219:	0.3%
Covered Lines:	50779
Relevant Lines:	79236

💛 - Coveralls

teor2345 · 2020-03-02T00:24:51Z

changes/bug33375

+      marking the relay descriptor dirty for 24 hours, not advertising an
+      IPv6 exit policy with a dirty descriptor, and then resetting the
+      IPv6 DNS broken count every 24 hours. Fixes bug 33375; bugfix on
+      0.2.9.14. Patch by Neel Chauhan.


Can you help me understand why you chose 0.2.9.14 as the bugfix version?

IPv6 DNS support was added early in the 0.2 series, maybe 0.2.4 ?

teor2345 · 2020-03-02T00:26:55Z

changes/bug33375

+    - Stop advertising an IPv6 exit policy when DNS is broken for IPv6 by
+      marking the relay descriptor dirty for 24 hours, not advertising an
+      IPv6 exit policy with a dirty descriptor, and then resetting the
+      IPv6 DNS broken count every 24 hours. Fixes bug 33375; bugfix on


Can you rephrase the changes file so it's clearer?

It helps to have shorter sentences. Start with a sentence that summarises the change.

You don't need to describe the change in so much detail. Something like "disable the IPv6 exit policy for 24 hours" should be enough.

teor2345 · 2020-03-02T00:28:13Z

src/feature/relay/dns.c

@@ -127,6 +129,8 @@ static uint64_t n_ipv6_requests_made = 0;
 static uint64_t n_ipv6_timeouts = 0;
 /** Global: Do we think that IPv6 DNS is broken? */
 static int dns_is_broken_for_ipv6 = 0;
+/** Has IPv6 DNS failed ever? */
+static int dns_was_broken_for_ipv6 = 0;


Are this comment and variable name accurate?

The code resets IPv6 DNS after 24 hours, so it would be helpful to say "recent".

teor2345 · 2020-03-02T00:29:00Z

src/feature/relay/dns.c

@@ -2026,6 +2033,26 @@ dns_launch_correctness_checks(void)
  }
 }

+/** If appropriate, if IPv6 DNS failed, reset the countner every 24 hours */
+static void
+dns_launch_ipv6_broken_reset(void)


"launch" could mean now, or later.

Can we say "schedule" instead?

Suggested change

dns_launch_ipv6_broken_reset(void)

dns_schedule_ipv6_broken_reset(void)

teor2345 · 2020-03-02T00:30:06Z

src/feature/relay/dns.c

+  if (!launch_event)
+    launch_event = tor_evtimer_new(tor_libevent_get_base(),
+                                   dns_reset_ipv6_broken, NULL);
+  timeout.tv_sec = 24 * 60 * 60;


Please define this value using a named macro, rather than just doing the calculation directly in the code.

Named constants help people understand the code.

teor2345 · 2020-03-02T00:42:23Z

src/feature/relay/router.c

@@ -1939,6 +1939,12 @@ get_my_declared_family(const or_options_t *options)
  return result;
 }

+#ifdef HAVE_MODULE_RELAY
+#define IPV6_DNS_NOT_BROKEN !dns_seems_to_be_broken_for_ipv6()


There could be a critical bug here:

dns_seems_to_be_broken_for_ipv6() checks dns_is_broken_for_ipv6. But the patch adds dns_was_broken_for_ipv6, so it looks like you meant to use dns_was_broken_for_ipv6.

Good unit tests would have caught this bug. Better variable naming might also have avoided it.

I also don't think dns_was_broken_for_ipv6 will need a macro, because it's only defined in this file.

Please remove the macro for IPV6_DNS_NOT_BROKEN.

It's confusing to hide functions behind a macro. And double negatives are also confusing.

Instead, put a function-like macro in the !defined(HAVE_MODULE_RELAY) section of dns.h.

teor2345 · 2020-03-20T11:19:02Z

changes/bug33375

@@ -0,0 +1,4 @@
+  o Minor bugfixes (coding best practices checks):
+    - Stop advertising an IPv6 exit policy when DNS is broken for IPv6 by
+      marking the relay descriptor dirty for 24 hours. Fixes bug 33375;


This changes file doesn't describe what the patch actually does.

Maybe delete "for IPv6 by marking the relay descriptor dirty".

teor2345 · 2020-03-20T11:22:15Z

src/feature/relay/dns.c

@@ -1486,7 +1491,11 @@ configure_nameservers(int force)
  if (evdns_base_count_nameservers(the_evdns_base) == 1) {
    SET("max-timeouts:", "1000000");
  } else {
-    SET("max-timeouts:", "10");
+    if (options->TestingTorNetwork) {


This code makes libevent stop using a DNS server when there is a timeout.

Please don't change this code.

We want libevent to switch servers after a few timeouts, so that we only disable IPv6 DNS if all the configured DNS servers are failing.

teor2345 · 2020-03-20T11:31:35Z

src/feature/relay/dns.c

@@ -1559,7 +1568,9 @@ evdns_callback(int result, char type, int count, int ttl, void *addresses,
        log_notice(LD_EXIT, "More than half of our IPv6 requests seem to "


Please increase the minimum number of DNS queries in these lines:

if (n_ipv6_timeouts > 10 && n_ipv6_timeouts > n_ipv6_requests_made / 2) {

But set it to 1 when TestingTorNetwork is 1.

Also, please replace the numbers by named constants. One of the reasons you changed the wrong code, is that there are multiple "10"s in this file.

neelchauhan force-pushed the bug33375 branch from 902b237 to 3dfd879 Feb 26, 2020

Stop advertising an IPv6 exit policy when DNS is broken for IPv6

1d93df8

neelchauhan force-pushed the bug33375 branch from 3dfd879 to 1d93df8 Feb 26, 2020

teor2345 reviewed Mar 2, 2020

View changes

neelchauhan added 8 commits Mar 2, 2020

fixup! Stop advertising an IPv6 exit policy when DNS is broken for IPv6

96cd7e0

fixup! Stop advertising an IPv6 exit policy when DNS is broken for IPv6

b8a47f9

Add unit test for ticket #33375

47e9538

fixup! Add unit test for ticket #33375

f8531bc

fixup! Add unit test for ticket #33375

463a41c

fixup! Add unit test for ticket #33375

64394dd

fixup! Add unit test for ticket #33375

69e33a3

fixup! Add unit test for ticket #33375

c7bb7dd

teor2345 reviewed Mar 20, 2020

View changes

neelchauhan added 2 commits Mar 20, 2020

fixup! Stop advertising an IPv6 exit policy when DNS is broken for IPv6

e7b7648

fixup! Stop advertising an IPv6 exit policy when DNS is broken for IPv6

b5565e7

torproject-pusher closed this May 25, 2021

torproject-pusher deleted the branch torproject:master May 25, 2021

torproject / tor Public

[Tor Trac #33375]: Stop advertising an IPv6 exit policy when DNS is broken for IPv6 #1771

[Tor Trac #33375]: Stop advertising an IPv6 exit policy when DNS is broken for IPv6 #1771

neelchauhan commented Feb 26, 2020

coveralls commented Feb 26, 2020 •

edited

teor2345 Mar 2, 2020

teor2345 Mar 2, 2020

teor2345 Mar 2, 2020

teor2345 Mar 2, 2020

teor2345 Mar 2, 2020

teor2345 Mar 2, 2020

teor2345 Mar 20, 2020

teor2345 Mar 20, 2020

teor2345 Mar 20, 2020

teor2345 Mar 20, 2020

teor2345 Mar 20, 2020

	dns_launch_ipv6_broken_reset(void)
	dns_schedule_ipv6_broken_reset(void)

		@@ -1559,7 +1568,9 @@ evdns_callback(int result, char type, int count, int ttl, void *addresses,
		log_notice(LD_EXIT, "More than half of our IPv6 requests seem to "

torproject / tor Public

[Tor Trac #33375]: Stop advertising an IPv6 exit policy when DNS is broken for IPv6 #1771

[Tor Trac #33375]: Stop advertising an IPv6 exit policy when DNS is broken for IPv6 #1771

Conversation

neelchauhan commented Feb 26, 2020

coveralls commented Feb 26, 2020 • edited

Pull Request Test Coverage Report for Build 8526

💛 - Coveralls

coveralls commented Feb 26, 2020 •

edited