-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Validate port-options #22087
Validate port-options #22087
Conversation
Should this be done with |
I'd suggest searching for and looking at a previous pull request proposal to do this. Consider also unit testing and Doxygen docs. |
Consider also the various places in the codebase where users can input port numbers. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
9850393
to
2b6eed6
Compare
Added doxygen descriptions
Have I missed something, like a config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might wanna add the validation at addpeeraddress
too. Currently, the port number will overflow if it is invalid.
Thanks for your input, @klementtan ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating it!
I think you might have missed out the scenario where the user enters -1
as the port. On a1c5914
$ ./src/bitcoin-cli -signet addpeeraddress "1.1.1.3" -1
{
"success": true
}
You might wanna refactor the logic in strencodings.cpp
so that the port validation code could be reused in other places(ie addpeeraddress
)
a1c5914
to
96942e8
Compare
You are absolutely right, fixed that. I also added a couple negative port numbers in the tests, thanks. |
Concept ACK. I would personally call this "validate" (as in input validation), not "santize". In the source base we use "sanitize" only in the context of removing invalid characters from strings in logging. |
96942e8
to
3309b68
Compare
I have changed the wording, thanks 👍 |
95bc9bd
to
effb74f
Compare
Check `port` options for invalid values (ports are parsed as uint16, so in practice values >65535 are invalid; port 0 is undefined and therefore considered invalid too). This allows for an early rejection of faulty values and an supplying an informative message to the user. Splits tests in `feature_proxy.py` to cover both invalid `hostname` and `port` values. Github-Pull: bitcoin#22087 Rebased-From: 1b6f8d3
ada8358 Sanitize port in `addpeeraddress()` (amadeuszpawlik) Pull request description: In connection to bitcoin#22087, it has been [pointed out](bitcoin#22087 (review)) that `addpeeraddress` needs to get its port-value sanitized. ACKs for top commit: fanquake: ACK ada8358 Tree-SHA512: 48771cd4f6940aa7840fa23488565c09dea86bd5ec5a5a1fc0374afb4857aebcd2a1f51e2d4cb7348460e0ad9793dc5d2962df457084ed2b8d8142cae650003f
ce4652a
to
1dae86b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1dae86b, and it looks almost ok, but I am concerned different SplitHostPort
hostOut
values when port is invalid could potentially be bad for callers. Or even if it doesn't break anything, it seems better just to add a additional return value not change hostOut
values in this PR.
It would also be good to note the change of behavior in the PR description or release notes that -port
and -rpcport
values that previously worked and were considered valid can now result in errors. For example port values with leading whitespace or trailing non-digit characters.
I do think code in second commit is a little awkward because it is parsing and validating address and port options in different place where they are used. But probably this is fine, since in the future this will probably be cleaned up by introducing options structs and moving new code here to a function like ApplyArgsManOptions
that does parsing and error-checking all in one place.
@amadeuszpawlik any chance you're still around / interested in responding to the latest review feedback? |
@fanquake Yes, definitely. Will have a go at it shortly 👍 |
72870e1
to
7a7eb90
Compare
9c2031d
to
de27526
Compare
Forward the validation of the port from `ParseUInt16(...)`. Consider port 0 as invalid. Add suitable test for the `SplitHostPort` function. Add doxygen description to the `SplitHostPort` function.
Check `port` options for invalid values (ports are parsed as uint16, so in practice values >65535 are invalid; port 0 is undefined and therefore considered invalid too). This allows for an early rejection of faulty values and an supplying an informative message to the user. Splits tests in `feature_proxy.py` to cover both invalid `hostname` and `port` values. Adds a release-note as previously valid `-port` and `-rpcport` values can now result in errors.
de27526
to
0452678
Compare
Thanks @ryanofsky for taking your time. I have now resolved all your comments and added a release-note. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 0452678. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.
utACK 0452678 |
(In the future, though, please don't rebase every time you make changes - only when needed) |
@ryanofsky just fyi. You've re-ACK the previous commit hash in your latest comment |
Thank you, fixed now |
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 #24116 and #24344, port "0" is considered invalid too.Proposed in #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,