Opened 2 months ago

Last modified 12 days ago

#29144 merge_ready defect

Log the correct "auto" port number for listening sockets

Reported by: kjak Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 041-proposed, 035-backport, 040-backport, consider-backport-after-0404-alpha
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: catalyst Sponsor:

Description

When auto is used for the port number for a listening socket, the message that gets logged after opening the socket incorrectly says port 0 instead of the actual port being used.

A contrived config like this

ControlPort auto
SocksPort 127.0.0.1:auto
ORPort 127.0.0.1:auto
ORPort [::1]:auto

gives log messages like this

Jan 21 12:38:05.741 [notice] Opening Socks listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] Socks listener listening on port 22840.
Jan 21 12:38:05.741 [notice] Opened Socks listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] Opening Control listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] Control listener listening on port 14414.
Jan 21 12:38:05.741 [notice] Opened Control listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] Opening OR listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] OR listener listening on port 19719.
Jan 21 12:38:05.741 [notice] Opened OR listener on 127.0.0.1:0
Jan 21 12:38:05.741 [notice] Opening OR listener on [::1]:0
Jan 21 12:38:05.742 [notice] OR listener listening on port 4298.
Jan 21 12:38:05.742 [notice] Opened OR listener on [::1]:0

An upcoming PR will fix this so that the log messages will have the actual port numbers like this

Jan 21 12:38:57.709 [notice] Opening Socks listener on 127.0.0.1:0
Jan 21 12:38:57.709 [notice] Socks listener listening on port 5236.
Jan 21 12:38:57.709 [notice] Opened Socks listener on 127.0.0.1:5236
Jan 21 12:38:57.709 [notice] Opening Control listener on 127.0.0.1:0
Jan 21 12:38:57.709 [notice] Control listener listening on port 14584.
Jan 21 12:38:57.709 [notice] Opened Control listener on 127.0.0.1:14584
Jan 21 12:38:57.709 [notice] Opening OR listener on 127.0.0.1:0
Jan 21 12:38:57.709 [notice] OR listener listening on port 15220.
Jan 21 12:38:57.709 [notice] Opened OR listener on 127.0.0.1:15220
Jan 21 12:38:57.709 [notice] Opening OR listener on [::1]:0
Jan 21 12:38:57.709 [notice] OR listener listening on port 36901.
Jan 21 12:38:57.709 [notice] Opened OR listener on [::1]:36901

This was introduced by commit 27c868eff19dbcc208f6db66ec3e2de2104fa754 and occurs in 0.3.5.1-alpha.

Child Tickets

Change History (12)

comment:1 Changed 2 months ago by kjak

Status: newneeds_review

PR: https://github.com/torproject/tor/pull/663

This PR includes the fix and a changes file. make check-changes does not complain.

comment:2 Changed 8 weeks ago by teor

Keywords: 041-proposed added
Milestone: Tor: 0.4.1.x-final

Let's review this in 0.4.1, without discussing it at the team meeting.

comment:3 Changed 8 weeks ago by teor

Points: 0.1

comment:4 Changed 5 weeks ago by dgoulet

Reviewer: catalyst

comment:5 Changed 3 weeks ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me! I did a quick manual test with ControlPort auto and confirmed that the log message read as expected with a non-zero port number.

Note for whoever merges this: the pull request is based on master, but looks like it cherry-picks cleanly to 0.3.5.

comment:6 in reply to:  5 Changed 3 weeks ago by dgoulet

Replying to catalyst:

Note for whoever merges this: the pull request is based on master, but looks like it cherry-picks cleanly to 0.3.5.

Not sure this qualifies for a backport as the log are at notice level. You think it should?

comment:7 Changed 3 weeks ago by teor

Keywords: 035-backport 040-backport added

In general, we are backporting ux issues to 0.3.5, including logging issues.
This issue is a minor source code change that is very low risk, so I will backport it to 0.3.5.

Please cherry-pick to maint-0.3.5 before merging to 0.4.0 and master.
Then I will backport it some time after the mainline CI passes.

comment:8 Changed 3 weeks ago by teor

Status: merge_readyneeds_revision

Actually, I will do the cherry-pick, because our draft merge policy says:

 * For patch quality: We should make sure that every commit to Tor is
    reviewed and double-checked for conformance.

…

4. What patches can be merged without review?

The following items do not need a reviewer or a separate merger:

Putting out releases:
   * Creating the ChangeLog and ReleaseNotes
   * Bumping the version number
   * Creating a signed tag

Typo fixes in comments.

But it seems redundant to require someone to do the cherry-pick, someone else to review it, and then a third person to merge. We could probably get away with 2 people.

comment:9 Changed 3 weeks ago by teor

Status: needs_revisionneeds_review

comment:10 in reply to:  9 Changed 13 days ago by catalyst

Status: needs_reviewmerge_ready

Replying to teor:

The 0.3.5 cherry-pick is:
https://github.com/torproject/tor/pull/745

Thanks! Code looks good. Quick manual test works as expected.

comment:11 Changed 12 days ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final

LGTM; merging to 0.4.0 and marking for backport.

comment:12 Changed 12 days ago by teor

Keywords: consider-backport-after-0404-alpha added

These tickets are low-risk changes that were marked for backport after the release of 0.4.0.2-alpha. I want them to get testing in at least one alpha release, so I'll look at them after 0.4.0.4-alpha is released.

Note: See TracTickets for help on using tickets.