Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs()

[teor]

Currently, the address choice logic is:

• if we have an IPv6 address and can reach the ls IPv6 address, and prefer IPv6, use it
• if we have an IPv4 address and can reach the ls IPv4 address, use it

But it needs to be:

• if we have both addresses and can reach both, then use whatever we prefer
• if we have one address and can reach it, use it

This doesn't matter until clients put IPv6 addresses in the link specifier.

[teor]

These tickets aren't urgent, and they involve major refactoring.
Deferring to 0.3.3

[teor]

In my commit 20b0e9e07d in my branch bug23820_032 (#23820), I ripped out IPv6 support in hs_get_extend_info_from_lspecs(). We'll need to revert that commit, and then fix the issues in the function.

Remove buggy IPv6 support from hs_get_extend_info_from_lspecs()

The previous version of this function has the following issues:
* it doesn't choose between IPv4 and IPv6 addresses correctly, and
* it doesn't fall back to a 3-hop path when the address for a direct
  connection is unreachable.
But we can't fix these things in a bugfix release.

Instead, treat IPv6 addresses like any other unrecognised link specifier
and ignore them. If there is no IPv4 address, return NULL.

[nickm]

Defer a small handful of non-enhancement "new" tickets to 0.3.4

[nickm]

Per our triage process, these tickets are pending removal from 0.3.4.

[nickm]

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

[neel]

In ed25519_cert.h, link_specifier_t/link_specifier_st looks like this:

struct link_specifier_st {
  uint8_t ls_type;
  uint8_t ls_len;
  uint32_t un_ipv4_addr;
  uint16_t un_ipv4_port;
  uint8_t un_ipv6_addr[16];
  uint16_t un_ipv6_port;
  uint8_t un_legacy_id[20];
  uint8_t un_ed25519_id[32];
  TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized;
  uint8_t trunnel_error_code_;
};

But the fascist_firewall_choose_address_* family of functions defines DirPorts in the function, like this:

static void
fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
                                     uint16_t ipv4_orport,
                                     uint16_t ipv4_dirport,
                                     const tor_addr_t *ipv6_addr,
                                     uint16_t ipv6_orport,
                                     uint16_t ipv6_dirport,
                                     firewall_connection_t fw_connection,
                                     int pref_only,
                                     int pref_ipv6,
                                     tor_addr_port_t* ap)

Should I then assume the ipv4_dirport and ipv6_dirport is 0?

[teor]

Replying to neel:

In ed25519_cert.h, link_specifier_t/link_specifier_st looks like this:

struct link_specifier_st {
  uint8_t ls_type;
  uint8_t ls_len;
  uint32_t un_ipv4_addr;
  uint16_t un_ipv4_port;
  uint8_t un_ipv6_addr[16];
  uint16_t un_ipv6_port;
  uint8_t un_legacy_id[20];
  uint8_t un_ed25519_id[32];
  TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized;
  uint8_t trunnel_error_code_;
};

But the fascist_firewall_choose_address_* family of functions defines DirPorts in the function, like this:

static void
fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
                                     uint16_t ipv4_orport,
                                     uint16_t ipv4_dirport,
                                     const tor_addr_t *ipv6_addr,
                                     uint16_t ipv6_orport,
                                     uint16_t ipv6_dirport,
                                     firewall_connection_t fw_connection,
                                     int pref_only,
                                     int pref_ipv6,
                                     tor_addr_port_t* ap)

Should I then assume the ipv4_dirport and ipv6_dirport is 0?

Yes, link specifiers are only defined for ORPorts.
Also, #24732 will remove the IPv6 DirPort code, because it's unused.

[neel]

My PR is here: ​https://github.com/torproject/tor/pull/252

[teor]

Replying to neel:

My PR is here: ​https://github.com/torproject/tor/pull/252

Thanks for this patch!

The unit tests fail in the new function:
​https://travis-ci.org/torproject/tor/jobs/409667005#L3572

Please add new commits to the branch to make them pass.

You can run the unit tests before you push your branch using:

make check

You might be able to see the CI status on:
​https://github.com/torproject/tor/pull/252
​https://travis-ci.org/torproject/tor
​https://ci.appveyor.com/project/torproject/tor

If you can't, let us know, and we'll work out how to get you access.

[teor]

The Appveyor Windows CI gives a good error:

bash.exe : In file included from ../src/core/or/or.h:51:0,
At line:2 char:5
+     & $commandPath $args 2>&1
+     ~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (In file include...e/or/or.h:51:0,:String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
                 from ../src/feature/hs/hs_common.c:14:
../src/feature/hs/hs_common.c: In function 'hs_get_extend_info_from_lspecs':
../src/lib/net/address.h:308:49: error: null pointer dereference [-Werror=null-dereference]
         tor_addr_port_is_valid(&(ap)->addr, (ap)->port, (for_listening))
                                                 ^
../src/lib/net/address.h:305:29: note: in definition of macro 'tor_addr_port_is_valid'
          tor_port_is_valid((port), (for_listening)))
                             ^~~~
../src/feature/hs/hs_common.c:1720:8: note: in expansion of macro 'tor_addr_port_is_valid_ap'
   if (!tor_addr_port_is_valid_ap(ap, 0) || !have_legacy_id) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~
../src/feature/hs/hs_common.c:1729:64: error: null pointer dereference [-Werror=null-dereference]
              fascist_firewall_allows_address_addr(&ap->addr, ap->port,
                                                              ~~^~~~~~
In file included from ../src/lib/crypt_ops/crypto_rsa.h:21:0,
                 from ../src/lib/crypt_ops/crypto.h:20,
                 from ../src/core/or/or.h:30,
                 from ../src/feature/hs/hs_common.c:14:
../src/feature/hs/hs_common.c:1748:48: error: null pointer dereference [-Werror=null-dereference]
            "it: %s:%u", fmt_addr(&ap->addr), ap->port);
                                              ~~^~~
../src/lib/log/log.h:250:51: note: in definition of macro 'log_fn'
   log_fn_(severity, domain, __FUNCTION__, args, ##__VA_ARGS__)
                                                   ^~~~~~~~~~~
../src/feature/hs/hs_common.c:1755:50: error: null pointer dereference [-Werror=null-dereference]
                          onion_key, &ap->addr, ap->port);
                                                ~~^~~~~~
cc1.exe: all warnings being treated as errors
make: *** [Makefile:8173: src/feature/hs/hs_common.o] Error 1
make: *** Waiting for unfinished jobs....

​https://ci.appveyor.com/project/torproject/tor/build/job/7isiro2d4mryqtr0#L1825

[neel]

I have fixed these issues. The same PR is used for this new patch: ​https://github.com/torproject/tor/pull/252

[teor]

Appveyor builds on the latest master are failing due to #26986, so I merged the fix to b23588 and pushed to ​https://github.com/teor2345/tor.git

I'll review your branch once the CI succeeds.

[teor]

Hi Neel, thanks for this pull request.

I did a review on the pull request on github:
​https://github.com/torproject/tor/pull/252

[neel]

New PR is (here)​https://github.com/torproject/tor/pull/256.

UPDATE: Fix link.

[teor]

Please don't open a new pull request for every branch. New pull requests don't have the comments from old pull requests.

Instead, add new commits to the existing branch, then push that branch. Then the pull request will update with the new commits.

[neel]

Thank you for the advice. I will follow this for future patches.

However, could you still review the patch even with the "new" pull request?

[teor]

Replying to neel:

Thank you for the advice. I will follow this for future patches.

However, could you still review the patch even with the "new" pull request?

Yes, of course. It might take me a few days.

I'll also try to push your new branch to the old pull request, so I can see my comments.

[teor]

Hi, the code looks good, but a few comments need updating.

Reminder: please add extra commits to b23588a. When you push b23588a to your github, the pull request will update automatically.

[neel]

I have updated my comments and pushed it to b23588a.

[teor]

Thanks!

When I was reviewing your comments, I noticed that you deleted some code that we need to keep. Please fix with extra commits on that branch.

[neel]

I have updated my code and used the same branch b23588a.

[teor]

Thanks!

I added a commit to your branch with some more comment fixes based on:
​https://github.com/torproject/tor/pull/256#discussion_r208425879
​https://github.com/torproject/tor/pull/256#pullrequestreview-144234103
And I fixed another outdated function comment.

I also ran make test-network-all on the branch, using the chutney fix from #27067.

And I got the following errors:

$ make test-network-all
mkdir -p ./test_network_log
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-v23-ipv6-md single-onion-ipv6-md.
tor-stable found, running mixed flavors: mixed+hs-v23.
FAIL: basic-min
FAIL: bridges-min
PASS: hs-v2-min
PASS: hs-v3-min
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723794
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
PASS: single-onion-v23
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723833
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 1
PASS: bridges+ipv6-min
PASS: ipv6-exit-min
FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533723949
Warning: Bug:     0   tor                                 0x000000010bdb0a2c log_backtrace_impl + 76 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     1   tor                                 0x000000010bda1177 tor_bug_occurred_ + 503 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     10  tor                                 0x000000010b83fc17 do_main_loop + 2423 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     11  tor                                 0x000000010b8445fb tor_run_main + 779 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     12  tor                                 0x000000010b9a54a1 tor_main + 161 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     13  tor                                 0x000000010b7a039b main + 27 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     14  libdyld.dylib                       0x00007fff54fbe015 start + 1 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     2   tor                                 0x000000010bac2f6e hs_client_dir_info_changed + 206 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     3   tor                                 0x000000010bb16b9c networkstatus_set_current_consensus + 9740 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     4   tor                                 0x000000010ba59e11 connection_dir_client_reached_eof + 20913 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     5   tor                                 0x000000010ba54a1f connection_dir_reached_eof + 143 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     6   tor                                 0x000000010b8210d4 connection_handle_read + 11668 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     7   tor                                 0x000000010b836938 conn_read_callback + 88 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     8   libevent-2.1.6.dylib                0x000000010c7839c2 event_process_active_single_queue + 1057 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug:     9   libevent-2.1.6.dylib                0x000000010c780cb3 event_base_loop + 1074 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at ../src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: Every introduction point for service ggdz6i6fshpuszndyqu373r7vzo2kltoofydyie2mtpv4rketkixxgad is unusable or we can't extend to it. We can't connect. Number: 7
Warning: Not enough info to open a circuit to a rendezvous point for hidden service ggdz6i6fshpuszndyqu373r7vzo2kltoofydyie2mtpv4rketkixxgad. Number: 1
Warning: tor_addr_is_null: Bug: Called with unknown address family 214 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 43
Warning: tor_addr_is_null: Bug: Called with unknown address family 32 (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
Warning: tor_bug_occurred_: Bug: ../src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev 52941e3283a40281) Number: 2
PASS: single-onion-ipv6-md
PASS: mixed+hs-v23
Log and result files are available in ./test_network_log.
make: *** [test-network-all] Error 1
Exit 2

But Tor master passes make test-network-all.

I think that these errors happen because you're using tor_addr_t's without initialising them. See my comments on the pull request.

[teor]

I think that these errors happen because you're using tor_addr_t's without initialising them.

If fascist_firewall_choose_address_ls() and hs_get_extend_info_from_lspecs() had unit tests, we would have discovered these bugs sooner.

Would you like to write unit tests?
If not, someone will write tests eventually.

[neel]

I have pushed new commits.

I do not want to write a unit test (for now at least).

[teor]

Replying to neel:

I have pushed new commits.

I do not want to write a unit test (for now at least).

Ok, I opened #27086 for the unit tests.

[teor]

I ran make test-network-all on this branch, using the latest chutney master. I am seeing some tests pass, but some failures from this branch. I also saw some HS failures (#24610) and bridge failures (#27080).

So I am not sure if the code in ​https://github.com/torproject/tor/pull/256 works. It also doesn't have any unit tests. (All the other fascist_firewall_choose_address*() functions have unit tests.)

We need to be careful about our address parsing code, because it is a source of security and anonymity issues.

I don't think we can merge this branch until the bugs are fixed:

$ make test-network-all
mkdir -p ./test_network_log
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-v23-ipv6-md single-onion-ipv6-md.
tor-stable found, running mixed flavors: mixed+hs-v23.
PASS: basic-min
FAIL: bridges-min
PASS: hs-v2-min
PASS: hs-v3-min
PASS: single-onion-v23
FAIL: bridges+ipv6-min
FAIL: ipv6-exit-min
FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533778119
Warning: Bug:     0   tor                                 0x0000000100c53a2c log_backtrace_impl + 76 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     1   tor                                 0x0000000100c44177 tor_bug_occurred_ + 503 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     10  tor                                 0x00000001006e2c57 do_main_loop + 2423 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     11  tor                                 0x00000001006e763b tor_run_main + 779 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     12  tor                                 0x0000000100848451 tor_main + 161 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     13  tor                                 0x00000001006433db main + 27 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     14  libdyld.dylib                       0x00007fff54fbe015 start + 1 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     2   tor                                 0x0000000100965f1e hs_client_dir_info_changed + 206 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     3   tor                                 0x00000001009b9b9c networkstatus_set_current_consensus + 9740 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     4   tor                                 0x00000001008fcdc1 connection_dir_client_reached_eof + 20913 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     5   tor                                 0x00000001008f79cf connection_dir_reached_eof + 143 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     6   tor                                 0x00000001006c4114 connection_handle_read + 11668 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     7   tor                                 0x00000001006d9978 conn_read_callback + 88 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     8   libevent-2.1.6.dylib                0x00000001016269c2 event_process_active_single_queue + 1057 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug:     9   libevent-2.1.6.dylib                0x0000000101623cb3 event_base_loop + 1074 (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at ../src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
Warning: Every introduction point for service p2qpw3p725i6dspcw5yxy7zltrqulp4dnspfza3quiasro3rgx2yabid is unusable or we can't extend to it. We can't connect. Number: 8
Warning: Not enough info to open a circuit to a rendezvous point for hidden service p2qpw3p725i6dspcw5yxy7zltrqulp4dnspfza3quiasro3rgx2yabid. Number: 1
Warning: tor_bug_occurred_: Bug: ../src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev dc06393aac4a459c) Number: 1
PASS: single-onion-ipv6-md
PASS: mixed+hs-v23
Detail: chutney/tools/warnings.sh /Users/base/chutney/net/nodes.1533778259
Warning: Unable to add signatures to consensus: Mismatched digest. Number: 20
Log and result files are available in ./test_network_log.

Edit: typo

[neel]

Surprisingly the code did not crash on my laptop (FreeBSD 12), but did on my desktop (FreeBSD 12 again) and on a VM (Debian 9).

I figured out how to get fix the bug, however, with a catch: instead of using fascist_firewall_choose_address_base() to choose the address in fascist_firewall_choose_address_ls(), I do it manually with:

  if (have_v6 && (pref_ipv6 || !have_v4)) {
    tor_addr_copy(&ap->addr, &addr_v6);
    ap->port = port_v6;
  } else if (have_v4 && (!pref_ipv6 || !have_v6)) {
    tor_addr_copy(&ap->addr, &addr_v4);
    ap->port = port_v4;
  } 

Would this be okay?

This code goes in place of fascist_firewall_choose_address_base() in fascist_firewall_choose_address_ls(). This passes make test-network-all.

These changes are now pushed. A copy old branch (before the change) is here: ​https://github.com/neelchauhan/tor/tree/b23588a_old

UPDATE: Make comment more descriptive and announce the push.

[teor]

We try not to duplicate code. I think your new patch is much simpler than fascist_firewall_choose_address_base(), because it doesn't actually choose addresses using the firewall rules on the client.

Here's what I suggest:

• If fascist_firewall_choose_address_base() doesn't work, then we should fix it.
• If fascist_firewall_choose_address_ls() is calling fascist_firewall_choose_address_base() wrong, then we should fix the call.

Also, I don't understand what you mean by:

We compare
addresses manually here as fascist_firewall_choose_address_base()
doesn't always return addresses given from lspecs.

Can you explain what behaviour you need from fascist_firewall_choose_address_base(), and what is happening instead?

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

But localhost (127.0.0.1 and ::1) should be reachable from the clients in the chutney networks. So I'm not sure why we're seeing this bug in chutney.

Maybe some unit tests could help track it down.

[teor]

Replying to teor:

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

You can log a BUG() warning and fail the connection when there's no reachable address. We can write the 3-hop fallback code later in #23818.

[neel]

Replying to teor:

Replying to teor:

I think you might be expecting fascist_firewall_choose_address_base() to always return a valid address. But sometimes, there won't be any reachable addresses. If there aren't any reachable addresses, we need the 3-hop fallback code, which hasn't been written yet.

You can log a BUG() warning and fail the connection when there's no reachable address. We can write the 3-hop fallback code later in #23818.

I plan to do this after having reverted my address choosing code and switched back to fascist_firewall_choose_address_base().

I do have a question: where should I put the BUG() warning when there's no reachable address?

UPDATE: Fix spelling.

[teor]

I do have a question: where should I put the BUG() warning when there's no reachable address?

hs_get_extend_info_from_lspecs() is a good place to check if the address and port returned by fascist_firewall_choose_address_ls() are valid. (You can use tor_addr_port_is_valid() to check ap.)

In future, when we make other functions call hs_get_extend_info_from_lspecs(), they might want to do different checks.

[neel]

I tried using BUG() here:

neel@kat:~/tor/tor % git diff
diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c
index df43d5cde..35e5d9ac1 100644
--- a/src/feature/hs/hs_common.c
+++ b/src/feature/hs/hs_common.c
@@ -1724,7 +1724,7 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
   fascist_firewall_choose_address_ls(lspecs, 0, &ap, direct_conn);
 
   /* Legacy ID is mandatory, and we require an IP address. */
-  if (!tor_addr_port_is_valid_ap(&ap, 0) || !have_legacy_id) {
+  if (BUG(!tor_addr_port_is_valid_ap(&ap, 0)) || !have_legacy_id) {
     /* If we're missing the legacy ID or the IP address, return NULL. */
     goto done;
   }
neel@kat:~/tor/tor %

I compiled the code and ran make test-network-all and got this:

neel@kat:~/tor/tor % make test-network-all                                      
mkdir -p ./test_network_log                                                     
ping6 ::1 or ping ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exi
t-min hs-v23-ipv6-md  single-onion-ipv6-md.                                     
tor-stable not found, skipping mixed flavors: mixed+hs-v23.                     
SKIP: mixed+hs-v23                                                              
PASS: basic-min                                                                 
FAIL: bridges-min                                                               
PASS: hs-v2-min                                                                 
PASS: hs-v3-min                                                                 
PASS: single-onion-v23                                                          
FAIL: bridges+ipv6-min                                                          
PASS: ipv6-exit-min                                                             
FAIL: hs-v23-ipv6-md                                                            
Detail: chutney/tools/warnings.sh /usr/home/neel/tor/chutney//net/nodes.1534250$
96                                                                              
Warning: Bug:     0x107a055 <_start+0xa5> at /usr/home/neel/tor/tor/src/app/tor 
(on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                          
Warning: Bug:     0x107a249 <main+0x19> at /usr/home/neel/tor/tor/src/app/tor ($
n Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29
Warning: Bug:     0x107a3ac <tor_main+0x4c> at /usr/home/neel/tor/tor/src/app/t$
r (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29
Warning: Bug:     0x107a6b4 <connection_add_impl+0x1f4> at /usr/home/neel/tor/to
r/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 5             
Warning: Bug:     0x107d1e1 <do_main_loop+0x421> at /usr/home/neel/tor/tor/src/a
pp/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                   
Warning: Bug:     0x107ef90 <tor_run_main+0xe0> at /usr/home/neel/tor/tor/src/ap
p/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                    
Warning: Bug:     0x10a580b <connection_ap_handshake_attach_circuit+0x46b> at /u
sr/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Num
ber: 24                                                                         
Warning: Bug:     0x10a6168 <connection_ap_handshake_attach_circuit+0xdc8> at /u
sr/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Num
ber: 24                                                                         
Warning: Bug:     0x10a799f <command_process_cell+0x72f> at /usr/home/neel/tor/t
or/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1            
Warning: Bug:     0x10ab0ea <connection_ap_attach_pending+0xda> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 24    
Warning: Bug:     0x10b0c72 <connection_or_process_inbuf+0x1b2> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1     
Warning: Bug:     0x10c0059 <circuit_receive_relay_cell+0x349> at /usr/home/neel
/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1
Warning: Bug:     0x10c05c2 <circuit_receive_relay_cell+0x8b2> at /usr/home/neel
/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1      
Warning: Bug:     0x10f4b3e <connection_dir_reached_eof+0x2e> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4       
Warning: Bug:     0x10f6ce8 <connection_dir_reached_eof+0x21d8> at /usr/home/nee
l/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4     
Warning: Bug:     0x110a125 <hs_circ_handle_introduce2+0x155> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1       
Warning: Bug:     0x110e0a1 <hs_client_get_random_intro_from_edge+0x211> at /usr
/home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Numbe
r: 24                                                                           
Warning: Bug:     0x110ed8e <hs_client_dir_info_changed+0x5e> at /usr/home/neel/
tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4       
Warning: Bug:     0x1111cfc <hs_get_extend_info_from_lspecs+0x21c> at /usr/home/
neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25 
Warning: Bug:     0x1118cac <hs_service_receive_introduce2+0xbc> at /usr/home/ne
el/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1    
Warning: Bug:     0x11222b4 <networkstatus_set_current_consensus+0xe04> at /usr/
home/neel/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number
: 4                                                                             
Warning: Bug:     0x116081d <rend_process_relay_cell+0x18d> at /usr/home/neel/to
r/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1 
Warning: Bug:     0x11a1ce3 <connection_handle_read+0x8a3> at /usr/home/neel/tor
/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 1          
Warning: Bug:     0x11a1f97 <connection_handle_read+0xb57> at /usr/home/neel/tor
/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4          
Warning: Bug:     0x120fcf1 <tor_bug_occurred_+0x111> at /usr/home/neel/tor/tor/
src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29              
Warning: Bug:     0x121370c <log_backtrace_impl+0x4c> at /usr/home/neel/tor/tor/
src/app/tor (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29              
Warning: Bug:     0x801b54f4e <event_base_loop+0x50e> at /usr/local/lib/libevent
-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 29                
Warning: Bug:     0x801b58da0 <event_base_assert_ok_nolock_+0x9d0> at /usr/local
/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 24   
Warning: Bug:     0x801b5906f <event_base_assert_ok_nolock_+0xc9f> at /usr/local
/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 5    
Warning: Bug: Non-fatal assertion !(!tor_addr_port_is_valid_ap(&ap, 0)) failed i
n hs_get_extend_info_from_lspecs at src/feature/hs/hs_common.c:1727. Stack trace
: (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25                        
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed 
in retry_all_socks_conn_waiting_for_desc at src/feature/hs/hs_client.c:275. Stac
k trace: (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4
Warning: Didn't recognize cell, but circ stops here! Closing circ. Number: 1    
Warning: Every introduction point for service cuq3slyq45o3smrd4chykbhklcltdfbg63
xopsezrwrxrfi3hkbtznad is unusable or we can't extend to it. We can't connect. N
umber: 8                                                                        
Warning: Not enough info to open a circuit to a rendezvous point for hidden serv
ice cuq3slyq45o3smrd4chykbhklcltdfbg63xopsezrwrxrfi3hkbtznad. Number: 1         
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_client.c:275: retry_all_socks
_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DES
C) failed. (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 4                
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_common.c:1727: hs_get_extend_
info_from_lspecs: Non-fatal assertion !(!tor_addr_port_is_valid_ap(&ap, 0)) fail
ed. (on Tor 0.3.5.0-alpha-dev b4b67c5fb5205e9a) Number: 25                      
PASS: single-onion-ipv6-md                                                      
Log and result files are available in ./test_network_log.                       
*** Error code 1                                                                
                                                                                
Stop.                                                                           
make: stopped in /usr/home/neel/tor/tor                                         
neel@kat:~/tor/tor %

This error is very similar to what you got. Surprisingly, I did not get this error on two laptops from different brands (I tested FreeBSD on both Dell and HP), so I am testing it on a server where the tests are more accurate.

Is the above code sample wrong (it hasn't been committed to git)? Can I fix it here, or should I do #23818 first, then update my branch for this patch with the latest Tor code, and then finish the patch?

[neel]

Also, when this code crashes, does it mean the connection wants to fall back to a 3-hop circuit but can't (and therefore crashes)?

[neel]

I think I know what the bug for hs-v23-ipv6-md is: some addresses are being marked as invalid as they don't make it through fascist_firewall_allows_address_ap() (I believe the main Tor codebase does not make addresses go though this now, nor does my old "simplified" address choosing code that got abandoned).

A solution can be to add a flag like verify_addr to fascist_firewall_choose_address_impl(), fascist_firewall_choose_address(), and fascist_firewall_choose_address_base() that determines whether we should verify the addresses or not, and set this flag to 0 in fascist_firewall_choose_address_ls()'s call to fascist_firewall_choose_address_base() and 1 in other calls.

In my solution, we end up choosing addresses by replacing code in fascist_firewall_choose_address_impl() from something like this:

   if (fascist_firewall_allows_address_ap(a, fw_connection, pref_only,
                                          pref_ipv6)) {

to this:

   if (fascist_firewall_allows_address_ap(a, fw_connection, pref_only,
                                          pref_ipv6) || !verify_addr) {

Would this be okay? I know this may not be a good solution, but it may help fix this bug we are talking about (and is similar to my previous address choosing code which worked, but with the fascist_firewall_allows_address_* family of functions).

I also attempted to do a three-hop fallback, and merged it with this codebase (not committed at all), but it still crashed with the same error described. It is possible that my three-hop fallback is broken.

[neel]

Well, the "solution" above did not end up working (not committed, however). It still crashed.

[neel]

I have a solution which should work (I tested and it does not crash), but it requires fixing #23818 first. I will upload a patch/PR for #23818 hopefully by the end of the day.

[neel]

Replying to neel:

I have a solution which should work (I tested and it does not crash), but it requires fixing #23818 first. I will upload a patch/PR for #23818 hopefully by the end of the day.

Never mind, this patch crashes also.

[neel]

I'll just give up on this bug. It is crashing too much.

[teor]

Replying to neel:

I think I know what the bug for hs-v23-ipv6-md is: some addresses are being marked as invalid as they don't make it through fascist_firewall_allows_address_ap() (I believe the main Tor codebase does not make addresses go though this now, nor does my old "simplified" address choosing code that got abandoned).

This issue sounds like a bug in fascist_firewall_allows_address_ap(), or a bug in how we're using fascist_firewall_allows_address_ap().

(The v2 onion service code uses fascist_firewall_allows_address_ap() and it works.)

[neel]

I looked closer and after inserting log_warn statements containing the IP address, I learned that while ap->addr gets set in fascist_firewall_choose_address_base(), if ap->addr can be an unknown address (probably AF_UNSPEC and ap->port is 0.

For instance, if I use this code on my branch:

diff --git a/src/core/or/policies.c b/src/core/or/policies.c
index e9e2cb4a5..a9a98ecc3 100644
--- a/src/core/or/policies.c
+++ b/src/core/or/policies.c
@@ -877,6 +877,9 @@ fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
   if (result) {
     tor_addr_copy(&ap->addr, &result->addr);
     ap->port = result->port;
+    char *out_addr = tor_addr_to_str_dup(&ap->addr);
+    log_warn(LD_BUG, "%s %d\n", out_addr, ap->port);
+    tor_free(out_addr);
   }
 }
 
@@ -1049,6 +1052,10 @@ fascist_firewall_choose_address_ls(const smartlist_t *lspecs,
                                        FIREWALL_OR_CONNECTION,
                                        pref_only, pref_ipv6,
                                        ap);
+
+  char *out_addr = tor_addr_to_str_dup(&ap->addr);
+  log_warn(LD_BUG, "%s %d\n", out_addr, ap->port);
+  tor_free(out_addr);
 }
 
 /** Like fascist_firewall_choose_address_base(), but takes <b>node</b>, and

and running make test-network-all gave me this for hs-v23-ipv6-md:

FAIL: hs-v23-ipv6-md
Detail: chutney/tools/warnings.sh /usr/home/neel/code/tor/chutney//net/nodes.1534465377
Warning: Bug:     0x111f299 <main+0x19> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x111f43c <tor_main+0x4c> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x111f744 <connection_add_impl+0x1f4> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1122271 <do_main_loop+0x421> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1124028 <tor_run_main+0xe8> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x1153037 <connection_handle_read+0xb57> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11bdb1e <connection_dir_reached_eof+0x2e> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11bfcc8 <connection_dir_reached_eof+0x21d8> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11daade <hs_client_dir_info_changed+0x5e> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x11efb34 <networkstatus_set_current_consensus+0xe04> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x12b51b1 <tor_bug_occurred_+0x111> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x12b8bcc <log_backtrace_impl+0x4c> at /usr/home/neel/code/tor/tor/src/app/tor (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x80139076f <event_base_loop+0x51f> at /usr/local/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug:     0x80139481d <event_base_assert_ok_nolock_+0xc8d> at /usr/local/lib/libevent-2.1.so.6 (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Bug: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed in retry_all_socks_conn_waiting_for_desc at src/feature/hs/hs_client.c:275. Stack trace: (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3
Warning: Every introduction point for service 6zlc2ni5rciibhqtva3354stgd6rywukmhb6cnevps3rdnucozisshyd is unusable or we can't extend to it. We can't connect. Number: 7
Warning: Not enough info to open a circuit to a rendezvous point for hidden service 6zlc2ni5rciibhqtva3354stgd6rywukmhb6cnevps3rdnucozisshyd. Number: 1
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5000 Number: 70
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5001 Number: 77
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5002 Number: 6
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 5003 Number: 10
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 7000 Number: 93
Warning: fascist_firewall_choose_address_base: Bug: 127.0.0.1 7001 Number: 102
Warning: fascist_firewall_choose_address_base: Bug: ::1 5000 Number: 95
Warning: fascist_firewall_choose_address_base: Bug: ::1 5001 Number: 51
Warning: fascist_firewall_choose_address_base: Bug: ::1 5002 Number: 7
Warning: fascist_firewall_choose_address_base: Bug: ::1 5003 Number: 4
Warning: fascist_firewall_choose_address_base: Bug: ::1 5004 Number: 21
Warning: fascist_firewall_choose_address_ls: Bug: 127.0.0.1 5001 Number: 1
Warning: fascist_firewall_choose_address_ls: Bug: <unknown address type> 0 Number: 22
Warning: tor_bug_occurred_: Bug: src/feature/hs/hs_client.c:275: retry_all_socks_conn_waiting_for_desc: Non-fatal assertion !(status == HS_CLIENT_FETCH_HAVE_DESC) failed. (on Tor 0.3.5.0-alpha-dev 9320918155a5ad74) Number: 3

I think the address referred to by this line:

Warning: fascist_firewall_choose_address_ls: Bug: <unknown address type> 0 Number: 22

is the one which crashes Tor.

[teor]

Yes, "0" is the numeric value for AF_UNSPEC. fascist_firewall_allows_address_ap() returns an AF_UNSPEC address when neither address is reachable.

That's a bug in the new code for this ticket, because each client should have at least one reachable address.

In the hs-ipv6-md network, chutney configures:

• an IPv4-only client
• an IPv6-only client
• an IPv6-only onion service
• a few IPv4/IPv6 relays

​https://gitweb.torproject.org/chutney.git/tree/networks/hs-ipv6-md

Please check the addresses that you're getting out of the lspecs. Maybe the parsing is wrong.

[neel]

The problematic input to fascist_firewall_choose_address_ls() is the one which despite having a valid IPv4 address does not pass fascist_firewall_allows_address_ap().

I found a solution which I have tested to work: Add a flag to fascist_firewall_choose_address_impl(), fascist_firewall_choose_address(), and fascist_firewall_choose_address_base() called check_fw where we use fascist_firewall_allows_address_ap() if check_fw is 1, or just tor_addr_port_is_valid_ap() if check_fw is 0.

All callers to fascist_firewall_choose_address_base() except for fascist_firewall_choose_address_ls() will set check_fw to 1. The reason why fascist_firewall_choose_address_ls() uses 0 for check_fw is to prevent the regression we have in make test-network-all.

It may not be a perfect solution (or it may), but it works while keeping our fascist_firewall_choose_address_* family of functions and not duplicating code.

The same PR is used (​https://github.com/torproject/tor/pull/256).

[teor]

No, I'm sorry, we need to check address reachability on clients. We can't disable reachability checks to work around other bugs in the code.

Here is one bug:

• When direct_conn is false, hs_get_extend_info_from_lspecs() calls fascist_firewall_choose_address_ls() on the IPv4 address.

Here is a fix:

• When direct_conn is false, hs_get_extend_info_from_lspecs() accepts any IPv4 address.

Here's why that works:

• An IPv6-only client can't connect to IPv4, but the relay at the end of its circuit should be able to extend to any IPv4 address.

Replying to teor:

Yes, "0" is the numeric value for AF_UNSPEC. fascist_firewall_allows_address_ap() returns an AF_UNSPEC address when neither address is reachable.

That's a bug in the new code for this ticket, because each client should have at least one reachable address.

In the hs-ipv6-md network, chutney configures:

• an IPv4-only client
• an IPv6-only client
• an IPv6-only onion service
• a few IPv4/IPv6 relays

​https://gitweb.torproject.org/chutney.git/tree/networks/hs-ipv6-md

Please check the addresses that you're getting out of the lspecs. Maybe the parsing is wrong.

[teor]

Thanks for the update.
We need some minor comment changes, and then I think we're good to go.

Does it pass "make test-network-all"?

[neel]

I have updated the comment.

Also, it passes make test-network-all.

[teor]

(Well, you're doing better than me. The master branch fails make test-network-all right now.)

[teor]

I'd like to defer this patch until #27080 is fixed, or until 0.3.6. It's not an essential feature, and changes to this code have been risky in the past.

nickm, what do you think?

(If we do want it in 0.3.5, let's make sure test-network-all passes on a few machines.)

I am also concerned about merging this feature without:

• unit tests (#27086),
• the 3-hop fallback for single onion services (#23818), and
• the corresponding service-side IPv6 code (#23576 and #24193).

We shouldn't hold it back forever, but if we wait until the next release, some of these related tasks should get done in that release.

Edit: #24193 is implemented by this ticket.

[teor]

nickm has agreed to defer this feature until 0.3.6.

[dgoulet]

@teor, should we still merge this. I'm a bit confused about the fact that we have concerns but still merge it? :S...

[teor]

Replying to dgoulet:

@teor, should we still merge this. I'm a bit confused about the fact that we have concerns but still merge it? :S...

I would like this new code to have unit tests (#27086) before we merge it.

The other related tickets would be nice to have, but they're optional.

[neel]

I decided that I will add the unit tests as well and do #27086.

[teor]

Thanks neel!
Let us know if you need some help.

[neel]

I have added a test. The same PR (​https://github.com/torproject/tor/pull/256) is used here.

On my system (FreeBSD 11.2 on HP EliteBook 1040 G3), the test passes. There is a merge as the last commit, but this is because of a merge conflict in the #includes of src/test/test_policy.c and CI would not build without it.

[teor]

Thanks!

I think this is good test code, but we can make the link specifier code a bit more reliable by using memcpy(), and setting and getting lengths. See the pull request comments for details.

[neel]

I have added the requested changes and have pushed them. Same PR.

[teor]

The tests for fascist_firewall_choose_address_ls() look good to me.

It looks like we still need tests for hs_get_extend_info_from_lspecs(), then we're done.

[teor]

hs_get_extend_info_from_lspecs() already has good coverage:
​https://coveralls.io/builds/19903451/source?filename=src/feature/hs/hs_common.c#L1677

But the direct_conn line needs to be tested, because it changed in this branch:
​https://coveralls.io/builds/19903451/source?filename=src/feature/hs/hs_common.c#L1740

Thanks!

[nickm]

Tor 0.3.6.x has been renamed to 0.4.0.x.

[neel]

I have added a hs_get_extend_info_from_lspecs() test. I am reusing the ls and mock configuration structs used to test fascist_firewall_choose_address_ls().

Sorry for the delay, I was really busy in the past two weeks.

[teor]

We all get busy at times.

Thanks for these tests.
It looks like we're missing a few tests for each function.

I commented on the pull request.

[neel]

I have (hopefully) added the tests requested. Can you review it and push it if it's okay?

[teor]

We just need a few tweaks to the log messages: if the remote side sends bad data, it's a protocol warning, not a bug warning. (This was my mistake in my last review.)

Maybe we should also log warnings when we don't have a legacy ID or a reachable IP address?

• a missing legacy ID is a protocol warning
• an unreachable IP address is an info-level warning (not a bug and not a protocol warning, because it can happen without any side doing the wrong thing)

​https://github.com/torproject/tor/pull/256/commits/d838a4beab1737faff1840666ec8dce1200881d8#diff-b2566dcf1fb829adf3289823089d4ad4R1752

[neel]

I have added the needed changes to fascist_firewall_choose_address_ls().

I have also added the new warnings to hs_get_extend_info_from_lspecs() and added a test for the missing legacy ID as the last commit. The unreachable IP address warning is untested for now.

Setting as needs review.

[teor]

Thanks, just two more tweaks left.

This branch will need a merge or a rebase before we merge it into master.
I'm happy to do the merge in a new pull request, once we get the logging sorted.

[neel]

I have made and pushed the changes. Setting as needs review.

[teor]

Ok, I think we're good here. I'll do the rebase and squash once I have my dev environment working again.

[teor]

I rebased and squashed neel's branch in:
​https://github.com/torproject/tor/pull/583

I also edited the changes file to say what the patch does (not how it is implemented).

Thanks neel!

[teor]

This branch passes all our current chutney tests.

It also passes the IPv6 v3 onion service test in chutney. So maybe the test is wrong, or this branch is choosing IPv4 addresses?

[teor]

These IPv6 tickets won't make it into 0.4.0, let's think about doing them in 0.4.1.
(They're not on the roadmap, so we'll need to decide if diagnosing the underlying issues is worth the time.)

[teor]

It looks like I have fixed the bug in the IPv6 v3 onion service code in #23576. I'll rebase and re-review this code in a few weeks' time, once #23576 has merged.

[teor]

I won't get to review this ticket for another week or two: doing the review requires fixing other tickets as well: #29237, #23576, #27251.

[teor]

I wasn't able to do this review in the last two weeks: I have been very busy with roadmap coding, sbws reviews, mainline merges, the review backlog, the backport backlog, and 0.4.0 bugs.

I expect to have time to do #29237 and #27251 by the end of March. This review might take another week after that.

[teor]

Pili asked me to tag these tickets

[teor]

The IPv6 tickets are not a must-have for feature parity, because v2 used a workaround for IPv6.