Opened 10 months ago

Last modified 9 months ago

#32880 accepted defect

V3 handshaking state change doesn't use "connection_or_change_state()"

Reported by: opara Owned by: dgoulet
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Minor Keywords: tor-chan, 043-deferred
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:


When an OR connection acting as a server changes to state OR_CONN_STATE_OR_HANDSHAKING_V3, it does so by setting conn->base_.state directly and not using connection_or_change_state(), so afaict this state change is never passed to pubsub or to the channel object.

On the other hand, when changing to that same state when acting as a client, it does use connection_or_change_state() as expected.

This seems to me to be a bug, but maybe there was a good reason for doing it this way. Also it seems no one has complained about it since the code was added, so having it changed doesn't seem to be important. But documenting here anyways since someone may want to take a look at it at some point.

Child Tickets

Change History (5)

comment:1 Changed 10 months ago by opara

It seems like when the function connection_or_change_state() was added to tor in commit 15303c32, the function was used for the client state in connection_or.c, but not the server state in channeltls.c.

comment:2 Changed 10 months ago by dgoulet

Keywords: tor-chan added
Milestone: Tor: 0.4.3.x-final
Owner: set to dgoulet
Points: 0.1
Priority: LowMedium
Status: newaccepted

Wow, this is a good catch!

I believe indeed this is a bug. The part that sucks is that connection_or_change_state() is static to connection_or.c so we'll have to make it visible.

Apart from pubsub not seeing it, I think this is harmless fortunately.

Reason is that enter_v3_handshake_with_cell() seems to be reachable only if the connection is currently handshaking or "possibly waiting for renegotiation" the handshake for which these two states can never be set from an open channel.

So, within channel_tls_handle_state_change_on_orconn(), we in theory never hit channel_change_state(base_chan, CHANNEL_STATE_MAINT);.

Nevertheless, this is a bug since the code assumption is that every OR conn state change need to go through _one_ specific callsite.

comment:3 Changed 10 months ago by opara

From a design/layer standpoint, it may be good to move the code that acts on the connection in enter_v3_handshake_with_cell() from channeltls.c to connection_or.c so that channeltls.c only needs to call something like connection_or_init_server_v3_handshake(). That way the channel doesn't need to worry about connection-specific things like renegotiation, updating the connection's internal state, initializing the connection's handshake state, etc.

comment:4 Changed 9 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:5 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: unspecified
Note: See TracTickets for help on using tickets.