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

upnp: Don't return loopback IPs in getOurIP. #2566

Merged
merged 1 commit into from Jan 22, 2021
Merged

upnp: Don't return loopback IPs in getOurIP. #2566

merged 1 commit into from Jan 22, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 21, 2021

Fixes #1620

net.LookupCNAME resolved to a loopback IP for me (127.0.1.1). The router can't port forward to this IP.

before

$ ./dcrd --upnp | grep -E "Successfully bound via UPnP|can't add UPnP port mapping"
-8: [WRN] SRVR: can't add UPnP port mapping: error 500 for AddPortMapping

after

$ git stash pop
$ go build
$ ./dcrd --upnp | grep -E "Successfully bound via UPnP|can't add UPnP port mapping"
-8: [WRN] SRVR: Successfully bound via UPnP to x.x.x.x:9108

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This reads fine, but will need some testing before being merged to master.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I did a bit of testing with this and verified the that behavior is more accurate than the existing code, so I'll approve this.

That said, it's still not accurate on systems with multiple addresses (e.g. both IPv4 and IPv6) nor those with multiple NICs. For example, the uPnP mapping fails completely on my Windows test system because it first tries to send the SSDP UDP from the address of one of the Hyper-V virtual NICs instead of the physical adapter in the host OS, then, if I hard code that in order to get past that, this getOurIP here returns a temporary IPv6 address instead of the IPv4 address it just tried to port map. Finally, the actual attempt to port map later fails because it tries to map the default port despite me having specified a different one in the listener.

@davecgh davecgh changed the title Don't return loopback IPs in getOurIP for uPnP upnp: Don't return loopback IPs in getOurIP. Jan 22, 2021
@davecgh davecgh added this to the 1.7.0 milestone Jan 22, 2021
@davecgh davecgh merged commit 5e961e8 into decred:master Jan 22, 2021
@davecgh
Copy link
Member

davecgh commented Jan 22, 2021

Also, I went ahead and fixed these things, but please note the code contribution guidelines as this didn't conform in several ways:

  1. Neither the commit title nor the PR title followed the documented format
  2. The commit message itself contains a GitHub issue number which is not allowed. Only the PR description may (and should) specify that.
  3. The commit message itself doesn't contain any of the pertinent information as it's all on the GitHub PR.

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.

--upnp support problem
2 participants