Use random ports for network client #124

cyberodactyl · 2024-08-17T10:42:06Z

See issue #48.

Concerns:

Does the name CreateSetupNetIO sounds clear enough?
Client machine now spends a little bit more resources for checking port availability before using the ports
Giving "port checking attempts parameter" a class constant may be a better idea than default parameter

Regarding 2, you would probably say "Then check for free ports only when initial CreateNetIO fails", well, the problem is, per my observation, a C4NetIO no longer appears to be usable once it fails to claim socket, perhaps we can pass a C4NetIO factory to the CreateSetupNetIO to create a new C4NetIO instance once it fails, but do we really need to overcomplicate here?

# Conflicts: # src/C4Network2IO.cpp

maxmitti · 2024-08-18T09:05:30Z

First of all, thank you for the effort, we appreciate it.

1. Does the name `CreateSetupNetIO` sounds clear enough?

I am not convinced, but let’s focus on more important things for now.

2. Client machine now spends a little bit more resources for checking port availability before using the ports

In itself doing more work is fine, as long as there is no easily noticable difference.

3. Giving "port checking attempts parameter" a class constant may be a better idea than default parameter

Yes, simply a constant would be better (a static constexpr variable, not #define). In my opinion it should even be a free constant, placed at the top of the .cpp file.

Regarding 2, you would probably say "Then check for free ports only when initial CreateNetIO fails", […]

That is correct ;)
I only looked at your changes on my phone now and am ignoring the implementation of C4NetIO for now, but only trying other ports on failure is what we want, especially because C4NetIO creation could also fail for other reasons. Another issue are race-conditions. In theory, between checking for port availability and actually using the port it could get occupied.

You are free and encouraged to modify C4NetIO directly.
A simple breakdown what I would expect:

Pass a value to C4NetIO indicating whether a fallback to other ports is desired.
Check the return value of bind() for the exact error code which is returned for occupied ports.
For clients it doesn’t really matter if the port is from a specific range, better let the OS choose it. AFAIK, this would mean that the code can simply continue without a successful bind().
We can discuss whether a predictable port range would be preferred, but letting the OS choose should be done as a final try anyway.
For hosts a fallback may be desirable as well. We should discuss this. Should we want a fallback, it must be communicated to the user in case manual port forwarding is used. E.g. “WARNING: The specified TCP port 11112 is unavailable, using 12345 instead. This could affect your port forwarding configuration.”
Determine the actually used port number and correctly update the port number that is published to other clients. I am assuming thisis essential to avoid the repeated errors you mentioned in the chat. In general, please share such information in the PR as well.

As usual, I invite @Fulgen301 to join the discussion.

Somebodyisnobody · 2024-08-20T19:45:49Z

First a big 😘 for caring about this issue!

I have some remarks to the host fallback:

For hosts a fallback may be desirable as well. We should discuss this. Should we want a fallback, it must be communicated to the user in case manual port forwarding is used. E.g. “WARNING: The specified TCP port 11112 is unavailable, using 12345 instead. This could affect your port forwarding configuration.”

A host fallback would be the icing on the cake. But if you inform the user I would give him the option to abort the lobby (before registering the lobby to league.clonkspot.org because this causes other processes to run like the Discord-Bot or other watches). So like WARNING: The specified TCP port 11112 is unavailable, using 12345 instead. <Ok> <Abort> <Retry?>

Please be aware, that almost every host is connected over a router with a firewall for IPv6. So we don't only have the network address translation table that we need to adjust for IPv4 (DNAT "port forwarding") but also a firewall for the nodes behind the SOHO-router for IPv6 (and IPv4). Therefore i would suggest the following message: This could affect your port forwarding or firewall configuration on your router.

Fulgen301

The logic is backwards - you check whether a port is available, and then create a C4NeIO instance; this is a TOCTOU race condition, as the port may have been used by another application in the meantime. The correct way is to attempt to create the C4NetIO instance and determine a new port to use if it fails - or do what @maxmitti said and ignore bind errors since the OS can already choose a random port.

Fulgen301 · 2024-08-18T09:15:50Z

src/C4Network2IO.cpp

+{
+	uint16_t port;
+
+	if (defaultPort == NULL)


Never use NULL in C++ - defaultPort also isn't a pointer.

Fulgen301 · 2024-08-18T09:16:51Z

src/C4Network2IO.cpp

+		}
+	}
+
+	T *netIO = CreateNetIO(this->logger, name, io, port, thread);


Unnecessary this->. Additionally, you should use const as the variable isn't changed, and brace-initialization unless the variable's type is auto:

Suggested change

T *netIO = CreateNetIO(this->logger, name, io, port, thread);

T *const netIO{CreateNetIO(logger, name, io, port, thread)};

Fulgen301 · 2024-08-18T09:17:52Z

src/C4Network2IO.cpp

+std::uint16_t C4Network2IO::DiscoverFreePort(const uint16_t attempts)
+{
+	static constexpr uint16_t maxPort = 65535;
+	uint16_t port = NULL;


As above. Also, we use std:: for types wherever possible.

Suggested change

uint16_t port = NULL;

std::uint16_t port{0};

Fulgen301 · 2024-08-18T09:18:08Z

src/C4Network2IO.cpp

+
+std::uint16_t C4Network2IO::DiscoverFreePort(const uint16_t attempts)
+{
+	static constexpr uint16_t maxPort = 65535;


This is a constant and therefore should be spelled MaxPort; otherwise, same as above.

Fulgen301 · 2024-08-18T09:18:13Z

src/C4Network2IO.cpp

+	return false;
+}
+
+std::uint16_t C4Network2IO::DiscoverFreePort(const uint16_t attempts)


Same as above.

Fulgen301 · 2024-08-18T09:19:00Z

src/C4Network2IO.cpp

+	static constexpr uint16_t maxPort = 65535;
+	uint16_t port = NULL;
+
+	for (uint16_t i = 0; i < attempts; ++i)


Same as above.

Fulgen301 · 2024-08-18T09:19:14Z

src/C4Network2IO.cpp

+
+	for (uint16_t i = 0; i < attempts; ++i)
+	{
+		port = SafeRandom(maxPort);


This can return 0, which isn't a valid port.

Fulgen301 · 2024-08-21T09:55:35Z

src/C4Network2IO.cpp

@@ -1248,6 +1264,43 @@ void C4Network2IO::SendConnPackets()
 		}
 }

+bool C4Network2IO::CheckPortAvailability(const uint16_t port)
+{
+	const auto pNetIO = std::make_unique<C4NetIOUDP>();


You can't check whether a TCP port is available by attempting to bind a UDP socket.

cyberodactyl added 3 commits August 17, 2024 12:58

Use random ports for connection

9f2e7fa

Merge branch 'refs/heads/master' into issue-48-net-client-random-ports

c095789

# Conflicts: # src/C4Network2IO.cpp

Merge with UPnP

5103b1d

Fulgen301 requested changes Aug 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use random ports for network client #124

Use random ports for network client #124

cyberodactyl commented Aug 17, 2024

maxmitti commented Aug 18, 2024 •

edited

Loading

Somebodyisnobody commented Aug 20, 2024

Fulgen301 left a comment •

edited

Loading

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 18, 2024

Fulgen301 Aug 21, 2024

	T *netIO = CreateNetIO(this->logger, name, io, port, thread);
	T *const netIO{CreateNetIO(logger, name, io, port, thread)};

Use random ports for network client #124

Are you sure you want to change the base?

Use random ports for network client #124

Conversation

cyberodactyl commented Aug 17, 2024

maxmitti commented Aug 18, 2024 • edited Loading

Somebodyisnobody commented Aug 20, 2024

Fulgen301 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmitti commented Aug 18, 2024 •

edited

Loading

Fulgen301 left a comment •

edited

Loading