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

p2p: fix exclusive node DNS resolution for certain hosts #8643

Merged
merged 1 commit into from Jan 11, 2023

Conversation

jeffro256
Copy link
Contributor

Fixes #8633. The function append_net_address did not parse hostname + port addresses (e.g. bar:29080) correctly if the hostname did not contain a . character.

@selsta
Copy link
Collaborator

selsta commented Nov 17, 2022

One thing I'm currently confused about is running your test without your fix makes the

NA_HOST_AND_PORT_TEST("bar:29080", "bar", "29080"); // Issue https://github.com/monero-project/monero/issues/8633

test succeed. Shouldn't it fail according to the bug?

Edit: Ignore, the bug wasn't in get_network_address_host_and_port().

@jeffro256
Copy link
Contributor Author

Yes, as @selsta points out, the bug is in append_net_address specifically where it doesn't even call get_network_host_and_port on the address if the hostname does not contain a . character. IDK why it was written that way, but I also modified get_network_host_and_port to cover the case of an IPv6 address with no port, just to be comprehensive.

tests/unit_tests/net.cpp Outdated Show resolved Hide resolved
tests/unit_tests/net.cpp Outdated Show resolved Hide resolved
tests/unit_tests/net.cpp Outdated Show resolved Hide resolved
src/net/parse.cpp Outdated Show resolved Hide resolved
@jeffro256
Copy link
Contributor Author

@vtnerd did this last commit cover everything?

@selsta
Copy link
Collaborator

selsta commented Nov 17, 2022

@jeffro256 looks good, please squash

Fixes monero-project#8633. The function `append_net_address` did not parse hostname + port addresses (e.g. `bar:29080`) correctly if the hostname did not contain a `'.'` character.

@vtnerd comments 1

clear up 2nd conditional statement
jeffro256 added a commit to jeffro256/monero that referenced this pull request Nov 21, 2022
Unrelated, but similar code-wise to monero-project#8643. There is a check in `DNSResolver` which automatically fails to resolve hostnames which do not contain the `.` character. This PR removes that check.
jeffro256 added a commit to jeffro256/monero that referenced this pull request Nov 22, 2022
Unrelated, but similar code-wise to monero-project#8643. There is a check in `DNSResolver` which automatically fails to resolve hostnames which do not contain the `.` character. This PR removes that check.
jeffro256 added a commit to jeffro256/monero that referenced this pull request Nov 22, 2022
Unrelated, but similar code-wise to monero-project#8643. There is a check in `DNSResolver` which automatically fails to resolve hostnames which do not contain the `.` character. This PR removes that check.
@luigi1111 luigi1111 merged commit b363eeb into monero-project:master Jan 11, 2023
@jeffro256 jeffro256 deleted the address_host_port_bar branch January 11, 2023 21:10
niklasha pushed a commit to niklasha/monero that referenced this pull request Mar 18, 2024
Unrelated, but similar code-wise to monero-project#8643. There is a check in `DNSResolver` which automatically fails to resolve hostnames which do not contain the `.` character. This PR removes that check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using --add-exclusive-node foo:28080 errors with: Failed to parse or resolve address from string: foo:28080
4 participants