Opened 11 months ago
Closed 8 months ago
#29144 closed defect (fixed)
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: | Tor: 0.3.5.1-alpha |
Severity: | Normal | Keywords: | 041-proposed, 035-backport, 040-backport, consider-backport-after-0404 |
Cc: | Actual Points: | 0.1 | |
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 (14)
comment:1 Changed 11 months ago by
Status: | new → needs_review |
---|
comment:2 Changed 11 months ago by
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 11 months ago by
Points: | → 0.1 |
---|
comment:4 Changed 10 months ago by
Reviewer: | → catalyst |
---|
comment:5 follow-up: 6 Changed 10 months ago by
Status: | needs_review → merge_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 Changed 10 months ago by
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 10 months ago by
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 10 months ago by
Status: | merge_ready → needs_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 follow-up: 10 Changed 10 months ago by
Status: | needs_revision → needs_review |
---|
The 0.3.5 cherry-pick is:
https://github.com/torproject/tor/pull/745
comment:10 Changed 9 months ago by
Status: | needs_review → merge_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 9 months ago by
Milestone: | Tor: 0.4.1.x-final → Tor: 0.3.5.x-final |
---|
LGTM; merging to 0.4.0 and marking for backport.
comment:12 Changed 9 months ago by
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.
comment:13 Changed 8 months ago by
Keywords: | consider-backport-after-0404 added; consider-backport-after-0404-alpha removed |
---|
Drop the -alpha from backport tags
comment:14 Changed 8 months ago by
Actual Points: | → 0.1 |
---|---|
Resolution: | → fixed |
Status: | merge_ready → closed |
Version: | → Tor: 0.3.5.1-alpha |
PR: https://github.com/torproject/tor/pull/663
This PR includes the fix and a changes file.
make check-changes
does not complain.