Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19191 closed defect (fixed)

{BUG} directory_send_command: Bug: Squid does not like URLs longer than 4095 bytes, and this one is 20515 bytes long

Reported by: fk Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.3-alpha
Severity: Normal Keywords: regression TorCoreTeam201606
Cc: Actual Points: .2
Parent ID: Points: .2
Reviewer: Sponsor:

Description

After upgrading the relay 40A60D1F1E8AFBED22FCB30B3FBE2E275A14CF99 to Tor v0.2.8.3-alpha I got the following warning after starting:

May 27 14:02:26.111 [warn] {BUG} directory_send_command: Bug: Squid does not like URLs longer than 4095 bytes, and this one is 20515 bytes long: /tor/server/d/0E2ED00342161B68622BDF90623068BD9FF56890+0E3A7F3C5F[...]

It was repeated a couple of times with slightly different paths.

This relay was previously running 0.2.7.6, but I'm also using 0.2.8.2-alpha on
other systems and haven't seen the warnings there.

I'm not sure if there's a connection, but bootstrapping did not make it beyond 80%.

Enabling debug logging resulted in lots of messages like:

May 27 14:27:16.604 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.605 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.605 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.606 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.606 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.607 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.607 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:16.608 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:27.346 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:30.560 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:33.302 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.
May 27 14:27:33.717 [info] {NET} get_interface_address6_via_udp_socket_hack: Address that we determined via UDP socket magic is unsuitable for public comms.

Tor is running in an ElectroBSD jail without raw socket access (which is probably required for the UDP socket magic to work).

After adding the Address directive bootstrapping worked and the relay is currently serving traffic.

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by nickm

Keywords: regression added
Milestone: Tor: 0.2.8.x-final

comment:2 Changed 3 years ago by nickm

So, it appears that max_dl_per_request is now happily returning a big number...

  /* If we're going to tunnel our connections, we can ask for a lot more
   * in a request. */
  if (!directory_fetches_from_authorities(options)) {
    max = 500;
  }

... but we still run our squid-check code on all (non-tunneled) directory connections, since conceivably they will hit a transparent proxy with a size limit:

  /* warn in the non-tunneled case */
  if (direct && (strlen(proxystring) + strlen(url) >= 4096)) {
    log_warn(LD_BUG,
             "Squid does not like URLs longer than 4095 bytes, and this "
             "one is %d bytes long: %s%s",
             (int)(strlen(proxystring) + strlen(url)), proxystring, url);
  }

So I think there are a couple possible "real problems" I can think of here:

  • The "real problem" might be that max_dl_per_request should be checking something other than directory_fetches_from_authorities() to predict whether we'll be tunneling the connection.
  • The "real problem" might be that we're not tunneling this connection, even though we don't fetch from authorities.

comment:3 Changed 3 years ago by nickm

In directory_command_should_use_begindir(), where we decide whether we should use begindir, we knock every connection down to not using begindir if !directory_must_use_begindir.

Suggested short-term fix for 0.2.8: we should replace the check in max_dl_per_request() with "if directory_must_use_begindir".

comment:4 Changed 3 years ago by nickm

Actual Points: .2
Keywords: TorCoreTeam201606 added
Owner: set to nickm
Points: .2
Status: newaccepted

Likely fix in branch bug19191_028 . Better fixes are conceivable.

The changes in fetch logic over 0.2.8 are tricky enough I wasn't able to tell quite when we introduced the bug for the changes file; please let me know what version if you can tell.

comment:5 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:6 Changed 3 years ago by dgoulet

I can confirm this works on 028 and 029 that is my relay does bootstrap correctly and I'm not seeing the BUG(). This seems like a logic change though that is quite the difference between directory_fetches_from_authorities and directory_must_use_begindir.

However, considering the fact that if we are a relay or dirauth, we don't tunnel thus setting the limit simply makes sense.

lgtm;

comment:7 Changed 3 years ago by fk

6eeedc02d8aee seems to prevent the BUG() message but does not
allow to bootstrap without an Address directive so this is probably
an unrelated regression.

dgoulet, did your relay actually have bootstrapping issues without the patch?

comment:8 Changed 3 years ago by nickm

dgoulet says

This seems like a logic change though that is quite the difference between directory_fetches_from_authorities and directory_must_use_begindir.

However, considering the fact that if we are a relay or dirauth, we don't tunnel thus setting the limit simply makes sense.

Right, but I think that the new logic is correct. The function asks, "When is it safe for us to ignore ask for 500 things at a time?" And the answer is "only when we must tunnel", not "when we don't fetch from authorities." So I think that the change is right.

fk says:

so this is probably an unrelated regression.

Yeah, I believe there is no way that the change in 6eeedc02d8aee0bf1eda8e592764ec9c5df4add7 could cause a regression like that.

So, merging to 0.2.8. Thanks, all!

comment:9 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:10 in reply to:  7 Changed 3 years ago by dgoulet

Replying to fk:

dgoulet, did your relay actually have bootstrapping issues without the patch?

For completion, it did not have any issues, bootstrapping was fine.

Note: See TracTickets for help on using tickets.