Opened 4 years ago

Last modified 9 months ago

#10481 new defect

connection_mark_unattached_ap_: checking always true edge_has_sent_end

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client cleanup refactor technical-debt
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

  ENTRY_TO_EDGE_CONN(conn)->edge_has_sent_end = 1; /* no circ yet */
  if ((edge_conn->on_circuit != NULL || edge_conn->edge_has_sent_end) &&
      connection_edge_is_rendezvous_stream(edge_conn)) {
    rend_client_note_connection_attempt_ended(
                                    edge_conn->rend_data->onion_address);
  }

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by cypherpunks

Summary: _connection_mark_unattached_ap: checking always true edge_has_sent_endconnection_mark_unattached_ap_: checking always true edge_has_sent_end

comment:2 Changed 4 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.5.x-final

Is there any bad effect here? If so, this should maybe be 024-backport as well.

comment:3 Changed 4 years ago by nickm

Keywords: 025-triaged added

comment:4 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

Defer to 0.2.6, since as far as we can tell this is harmless.

comment:5 Changed 4 years ago by nickm

(The code here seems confused and confusing in a lot of other ways beyond the issue here. We should try to untangle it while we're messing with it)

comment:6 Changed 4 years ago by andrea

This code's an icky mess which comes with a comment to that effect and telling us to make it suck less:

/** An AP stream has failed/finished. If it hasn't already sent back
 * a socks reply, send one now (based on endreason). Also set
 * has_sent_end to 1, and mark the conn.
 */
MOCK_IMPL(void,
connection_mark_unattached_ap_,(entry_connection_t *conn, int endreason,
                                int line, const char *file))
{
  connection_t *base_conn = ENTRY_TO_CONN(conn);
  edge_connection_t *edge_conn = ENTRY_TO_EDGE_CONN(conn);
  tor_assert(base_conn->type == CONN_TYPE_AP);
  ENTRY_TO_EDGE_CONN(conn)->edge_has_sent_end = 1; /* no circ yet */

  /* If this is a rendezvous stream and it is failing without ever
   * being attached to a circuit, assume that an attempt to connect to
   * the destination hidden service has just ended.
   *
   * XXXX This condition doesn't limit to only streams failing
   * without ever being attached.  That sloppiness should be harmless,
   * but we should fix it someday anyway. */
  if ((edge_conn->on_circuit != NULL || edge_conn->edge_has_sent_end) &&
      connection_edge_is_rendezvous_stream(edge_conn)) {
    rend_client_note_connection_attempt_ended(
                                    edge_conn->rend_data->onion_address);
  }

The condition is equivalent to:

if (connection_edge_is_rendezvous_stream(edge_conn)) {
    rend_client_note_connection_attempt_ended(
                                    edge_conn->rend_data->onion_address);
  }

This probably causes some superfluous calls to rend_client_note_connection_attempt_ended(), but we should investigate exactly what side effects calling that can have and the circumstances under which this is called, and then remove the stupid.

comment:7 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

comment:8 Changed 15 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:9 Changed 14 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:10 Changed 9 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:11 Changed 9 months ago by nickm

Keywords: 025-triaged removed

remove an old triage keyword.

comment:12 Changed 9 months ago by nickm

Keywords: cleanup refactor technical-debt added
Severity: Normal
Note: See TracTickets for help on using tickets.