Opened 3 years ago

Last modified 2 months ago

#19665 new defect

Should *Port_set count sockets?

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-control technical-debt config torrc startup
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

In parse_ports in 0.2.8-stable and earlier, we didn't count sockets as listeners when setting options->*Port_set.

This makes sense for server ports, is confusing for ControlPort/ControlSocket (because you can set a control socket using the ControlPort option, can't you?), and didn't matter for client ports, because those values were never used.

In #17178 I modified the client port options to count sockets.
But we should definitely review the control port situation some time.
I don't think it's serious, because the current code warns on ControlPorts and ControlSockets.
But it makes it easy to introduce subtle bugs.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by nickm

Agreed that we should refactor. I'd love to remove the count_sockets argument from count_real_listeners.

comment:2 Changed 2 years ago by teor

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

Milestone renamed

comment:3 Changed 2 years 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:4 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:5 Changed 21 months ago by nickm

Keywords: tor-control technical-debt config torrc startup added; 030-proposed removed

comment:6 Changed 11 months ago by dgoulet

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Parent ID: #25762
Sponsor: Sponsor8

Parenting this ticket because we need to properly identify a "tor client" and using those *_Set variables is what should be done.

comment:7 Changed 11 months ago by nickm

Parent ID: #25762#25500

comment:8 Changed 11 months ago by dgoulet

Reviewer: nickm
Status: newneeds_review

See branch ticket19665_034_01.

comment:9 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

Needs a changes file.

Also, I don't understand what exactly the fix in this patch has to do with the original issue report above. It seems like this patch is fixing "ControlPort doesn't mean you're a client!" but the ticket is asking for something else.

comment:10 Changed 11 months ago by teor

We should split off 59a32e5b4ff210b95bbed294cdc787cb75539da5 into a separate ticket and backport it to 0.2.9 and later. Otherwise, relays with the control port open will warn based on the client required protocols list.

And I agree with nickm, we should make ControlPort_set count sockets. Maybe we should make all the *Port_set options count sockets, for consistency. Otherwise, weird things will happen in test networks that use sockets.

comment:11 Changed 10 months ago by dgoulet

Milestone: Tor: 0.3.4.x-finalTor: unspecified
Parent ID: #25500
Reviewer: nickm
Status: needs_revisionnew

Unparenting and the fix needed for #25550 is now in ticket #26062.

comment:12 Changed 2 months ago by gaba

Sponsor: Sponsor8
Note: See TracTickets for help on using tickets.