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.

Sign up for GitHub

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

Jump to bottom

fix: don't use invalid port 0 when NATMap fails #3058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+9 −2
Open

fix: don't use invalid port 0 when NATMap fails #3058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion p2p/host/basic/basic_host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,8 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {

finalAddrs = ma.Unique(finalAddrs)

// counts the number of addresses that are correctly port mapped
successMapCnt := 0
// use nat mappings if we have them
if h.natmgr != nil && h.natmgr.HasDiscoveredNAT() {
// We have successfully mapped ports on our NAT. Use those
Expand Down Expand Up @@ -929,6 +931,8 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
continue
}

successMapCnt++

Comment on lines +934 to +935
Copy link
Member

@sukunrt sukunrt Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add the port 0 check to line 911 where we check for is unspecified address. If you do so, it removes the need for introducing this new successMapCnt variable.

I would prefer keeping this change small because this code is pretty bad and we intend to rewrite it soon in #2229

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the check placed on line 911 mean that the continue is executed when the extMaddr port is 0? If so, when all extMaddr ports are 0, will observedAddrs not be added to the finalAddrs address list?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so.

		if !manet.IsIPUnspecified(extMaddr) {
				// Add in the mapped addr.
				finalAddrs = append(finalAddrs, extMaddr)
} else {

If the nat provided address is not an unspecified addr and(new change) it's not port == 0, we will add it to final addrs.
We still fall through and add the observed addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is OK to add the address whose port is not 0 to finalAddrs here.

However, at

go-libp2p/p2p/host/basic/basic_host.go

Line 952 in f421202

finalAddrs = append(finalAddrs, ma.Join(ip, extMaddrNoIP))
, it is also necessary to judge whether the port is 0. If the port is 0, should the obsMaddr address be added directly without splicing?

for _, addr := range resolved {
// Now, check if we have any observed addresses that
// differ from the one reported by the router. Routers
Expand All @@ -953,7 +957,10 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
}
}
}
} else {
}

// if there is no properly handled port mapping address, add the observed address directly
if successMapCnt > 0 {
var observedAddrs []ma.Multiaddr
if h.ids != nil {
observedAddrs = h.ids.OwnObservedAddrs()
Expand Down
2 changes: 1 addition & 1 deletion p2p/net/nat/nat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (nat *NAT) GetMapping(protocol string, port int) (addr netip.AddrPort, foun
return netip.AddrPort{}, false
}
extPort, found := nat.mappings[entry{protocol: protocol, port: port}]
if !found {
if !found || extPort == 0 {
return netip.AddrPort{}, false
}
Comment on lines 109 to 112
Copy link
Member

Choose a reason for hiding this comment

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

Remove the check from here.

return netip.AddrPortFrom(nat.extAddr, uint16(extPort)), true
Expand Down
Loading