Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document listening on port 0 assigns a random unused port #24116

Closed
wants to merge 1 commit into from

Conversation

thonkle
Copy link

@thonkle thonkle commented Jan 20, 2022

If port 0 is supplied, the Linux kernel will select an available port. Reference: https://github.com/torvalds/linux/blob/2c271fe77d52a0555161926c232cd5bc07178b39/net/ipv4/inet_connection_sock.c#L358

This is also true for Windows: https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-bind#remarks

Since this behavior does not appear in manpages for bind(), listen(), etc. it is not reasonable to expect node operators to know this behavior exists, so it's best to document it where they will find it: in the help text.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 21, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23542 (net: open p2p connections to nodes that listen on non-default ports by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's not documented by the APIs, it's undefined behaviour, and we shouldn't document it either (unless we explicitly add code to support it).

@ghost
Copy link

ghost commented Feb 14, 2022

This is also true for Windows: https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-bind#remarks

I could not reproduce this on Win 10 based on below logs for bitcoind -port=0

2022-02-14T15:13:26Z Bound to 127.0.0.1:18334
2022-02-14T15:13:26Z Bound to [::]:0
2022-02-14T15:13:26Z Bound to 0.0.0.0:0

@thonkle
Copy link
Author

thonkle commented Feb 14, 2022

This is also true for Windows: https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-bind#remarks

I could not reproduce this on Win 10 based on below logs for bitcoind -port=0

2022-02-14T15:13:26Z Bound to 127.0.0.1:18334
2022-02-14T15:13:26Z Bound to [::]:0
2022-02-14T15:13:26Z Bound to 0.0.0.0:0

The log does not report the actual listening port when 0 is selected. Try sudo netstat -pln | grep bitcoin to see what port is being used.

@sipa
Copy link
Member

sipa commented Feb 14, 2022

I think we should just abort initialization if port 0 is requested. It's not actually something you can listen on, and the apparently default behavior of selecting a random port is undesirable for us.

@luke-jr
Copy link
Member

luke-jr commented Feb 14, 2022

The log does not report the actual listening port when 0 is selected.

IMO that means it isn't working as described by this PR.

Do we even recognise the assigned port for addr publishing purposes?

I think we should just abort initialization if port 0 is requested. It's not actually something you can listen on, and the apparently default behavior of selecting a random port is undesirable for us.

I agree. It seems code would be needed to support a random port, and I don't see justification for the added complexity that entails. Better to just make it an error.

@ghost
Copy link

ghost commented Feb 14, 2022

I think we should just abort initialization if port 0 is requested. It's not actually something you can listen on, and the apparently default behavior of selecting a random port is undesirable for us.

I agree. It seems code would be needed to support a random port, and I don't see justification for the added complexity that entails. Better to just make it an error.

Agree

@thonkle
Copy link
Author

thonkle commented Feb 14, 2022

Closing in favor of a new PR to do bounds checking on -port: #24344
Port 0 is not the only case that gives unexpected behavior, so we can fix all of them at once.

@thonkle thonkle closed this Feb 14, 2022
@thonkle thonkle mentioned this pull request Feb 15, 2022
fanquake added a commit to bitcoin-core/gui that referenced this pull request Oct 12, 2022
0452678 Validate `port` options (amadeuszpawlik)
f8387c4 Validate port value in `SplitHostPort` (amadeuszpawlik)

Pull request description:

  Validate `port`-options, so that invalid values are rejected early in the startup.
  Ports are `uint16_t`s, which effectively limits a port's value to <=65535. As discussed in bitcoin/bitcoin#24116 and bitcoin/bitcoin#24344, port "0" is considered invalid too.
  Proposed in bitcoin/bitcoin#21893 (comment)

  The `SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)` now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,

ACKs for top commit:
  luke-jr:
    utACK 0452678
  ryanofsky:
    Code review ACK 0452678. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.

Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d
@bitcoin bitcoin locked and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants