sezero / quakespasm

QuakeSpasm -- A modern, cross-platform Quake game engine based on FitzQuake.
https://sourceforge.net/projects/quakespasm/
GNU General Public License v2.0
239 stars 96 forks source link

Host & play on localhost #85

Closed Diordany closed 11 months ago

Diordany commented 11 months ago

Issue

Platform: Linux 6.6.2-arch1-1

The engine automatically assigns an IP address if none was set from the command line, my system selects 127.0.1.1. During the communication on my localhost, both 127.0.0.1 and 127.0.1.1 are used. The engine does not like this.

Steps to Reproduce

  1. Host a multiplayer game with one instance of QuakeSpasm, while having the IP address set to any ipv4 loopback address other than 127.0.0.1.
  2. Use another instance of QuakeSpasm on the same host to connect to that address (either manually or with the Search for local games... feature).

Technical Info

I've captured the packets using tcpdump -nXXi lo port quake:

Click to reveal the dump ``` 23:20:16.931804 IP 192.168.2.15.26000 > 192.168.2.15.35793: UDP, length 37 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0041 348f 4000 4011 80ae c0a8 020f c0a8 .A4.@.@......... 0x0020: 020f 6590 8bd1 002d 85ad 8000 0025 8331 ..e....-.....%.1 0x0030: 3237 2e30 2e31 2e31 3a32 3630 3030 0055 27.0.1.1:26000.U 0x0040: 4e4e 414d 4544 0065 316d 3100 0204 03 NNAMED.e1m1.... 23:20:17.666983 IP 192.168.2.15.26000 > 192.168.2.15.35793: UDP, length 37 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0041 34f9 4000 4011 8044 c0a8 020f c0a8 .A4.@.@..D...... 0x0020: 020f 6590 8bd1 002d 85ad 8000 0025 8331 ..e....-.....%.1 0x0030: 3237 2e30 2e31 2e31 3a32 3630 3030 0055 27.0.1.1:26000.U 0x0040: 4e4e 414d 4544 0065 316d 3100 0204 03 NNAMED.e1m1.... 23:20:18.418091 IP 127.0.0.1.51189 > 127.0.1.1.26000: UDP, length 12 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0028 5b59 4000 4011 e069 7f00 0001 7f00 .([Y@.@..i...... 0x0020: 0101 c7f5 6590 0014 ff27 8000 000c 0151 ....e....'.....Q 0x0030: 5541 4b45 0003 UAKE.. 23:20:20.917000 IP 127.0.0.1.51189 > 127.0.1.1.26000: UDP, length 12 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0028 5e46 4000 4011 dd7c 7f00 0001 7f00 .(^F@.@..|...... 0x0020: 0101 c7f5 6590 0014 ff27 8000 000c 0151 ....e....'.....Q 0x0030: 5541 4b45 0003 UAKE.. 23:20:20.922503 IP 127.0.0.1.26000 > 127.0.0.1.51189: UDP, length 9 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0025 e724 4000 4011 55a1 7f00 0001 7f00 .%.$@.@.U....... 0x0020: 0001 6590 c7f5 0011 fe24 8000 0009 8153 ..e......$.....S 0x0030: 9e00 00 ... 23:20:23.417969 IP 127.0.0.1.51189 > 127.0.1.1.26000: UDP, length 12 0x0000: 0000 0000 0000 0000 0000 0000 0800 4500 ..............E. 0x0010: 0028 5f11 4000 4011 dcb1 7f00 0001 7f00 .(_.@.@......... 0x0020: 0101 c7f5 6590 0014 ff27 8000 000c 0151 ....e....'.....Q 0x0030: 5541 4b45 0003 UAKE.. ```
  1. The server 192.168.2.15:26000 first sends two packets to the client 192.168.2.15.35793 with server information.
  2. The client 127.0.0.1:51189 then attempts to connect to the server with 127.0.1.1:26000 three times.
  3. The server replies, but with 127.0.0.1 rather than 127.0.1.1.
  4. The client doesn't like this and denies the legitimate replies.

The console reads:

Click to reveal the log ``` trying... wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:26000 | 020065907f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:57428 | 0200e0547f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:57428 | 0200e0547f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:57428 | 0200e0547f0000010000000000000000 still trying... still trying... wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:26000 | 020065907f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:34333 | 0200861d7f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:34333 | 0200861d7f0000010000000000000000 wrong reply address Expected: 127.0.1.1:26000 | 020065907f00010136000000ffffffff Received: 127.0.0.1:34333 | 0200861d7f0000010000000000000000 still trying... No Response Server Map Users --------------- --------------- ----- UNNAMED e1m1 1/ 4 == end list == Host_Error: CL_Connect: connect failed ```

Potential Fix

You can actually connect to the server without a problem if you manually type 127.0.0.1, but this workaround doesn't fix the issue for the Search for local games... feature.

I've written a potential fix that uses the netinet/in.h header to make an exception if the reply comes from localhost, which is defined as 127.0.0.1 in the header. This solves the issue on my system.

Edit: There's another potential fix here that checks both the send and read addresses. This fix also allows the engine to check the ports after confirming that both addresses are localhost, which is necessary (see the log).

And also a third potential fix that doesn't fiddle with the low level stuff, but is less accurate than the second one (see the comments).

Notes

Diordany commented 11 months ago

I've also been experimenting with another fix that checks both the send and read addresses, with the assumption that the local host network is 127.0.0.0/8.

sezero commented 11 months ago

@ericwa (and @andrei-drexler?): Can you please review the linked patches?

Diordany commented 11 months ago

~After some more testing I've noticed that host & play only works properly for two QuakeSpasm instances on localhost. And that also seems to be the case without my patch.~

Edit: this is a different issue.

sezero commented 11 months ago

OK then, closing.

Diordany commented 11 months ago

After some more testing I've noticed that host & play only works properly for two QuakeSpasm instances on localhost. And that also seems to be the case without my patch.

I'm sorry for my confusing message @sezero. I should've mentioned that I was talking about a different issue that I discovered through further testing. I only mentioned it for context, because I thought it might be relevant to this issue in some way. I'll remove it from this conversation to avoid further confusion.

The main issue here is still present without my patch.

sezero commented 11 months ago

Re-opening

Diordany commented 11 months ago

I've been experimenting with another fix that doesn't fiddle with the low level stuff. This one simply checks if the argument given to the connect command starts with "127" (again assuming a localhost network of 127.0.0.0/8) and replaces the argument with "localhost".

It's less accurate than this fix which actually tests the IP address data rather than the start of the argument string, but it does fix my issue as well.

Shpoike commented 11 months ago

if you're going to use address aliasing, be sure to use non-overlapping netmasks on each alias, so the system will only have one source address to pick from for each outbound one.

the only other option is to use the -ip arg, but that's limited to one address in most engines (fte's sv_port accepts port or addr:port space-separated tokens for multiple ports optionally with explicit addresses). having the engine startup include a scan for all addresses to open a per-address socket on each address will fail the instant someone switches wifi network etc - just using inaddr_any and leaving the OS to handles the routing is simply the more robust solution.

just ignoring the address the user is trying to use and using a different one instead is insanity. localhost tends to expand to ::1 first, nowadays, but most nq engines doesn't support ipv6. hurrah for extra confusion when you're trying to be explicit about using 127.0.0.1 instead for compat with older servers. also '127.com' is apparently a valid domain name (resolving to two different ips) so that's extra breakage with your 'fix', well done.

Diordany commented 11 months ago

if you're going to use address aliasing, be sure to use non-overlapping netmasks on each alias, so the system will only have one source address to pick from for each outbound one.

the only other option is to use the -ip arg, but that's limited to one address in most engines (fte's sv_port accepts port or addr:port space-separated tokens for multiple ports optionally with explicit addresses). having the engine startup include a scan for all addresses to open a per-address socket on each address will fail the instant someone switches wifi network etc - just using inaddr_any and leaving the OS to handles the routing is simply the more robust solution.

just ignoring the address the user is trying to use and using a different one instead is insanity. localhost tends to expand to ::1 first, nowadays, but most nq engines doesn't support ipv6. hurrah for extra confusion when you're trying to be explicit about using 127.0.0.1 instead for compat with older servers. also '127.com' is apparently a valid domain name (resolving to two different ips) so that's extra breakage with your 'fix', well done.

Thank you for your reply I'll look into this later.

Diordany commented 11 months ago

To address a few points:

also '127.com' is apparently a valid domain name (resolving to two different ips) so that's extra breakage with your 'fix', well done.

Yeah, this is very problematic. At the time of writing I only considered the fact that QuakeSpasm automatically changes 127 to 127.0.0.127 on my platform. I overlooked this obvious scenario.

hurrah for extra confusion when you're trying to be explicit about using 127.0.0.1 instead for compat with older servers.

I'm not sure why you're saying this. I was being explicit about 127.0.0.0/8 and localhost. Are you refering to the steps to reproduce?

Diordany commented 11 months ago

@sezero my issue was resolved by correcting my system administration, this issue can be closed now. I also want to thank @Shpoike for pointing out the error in my approach.

Note: but please try to be a bit more respectful in the future @Shpoike. "... is insanity.", "hurrah for extra confusion when you're trying to ..." and "... so that's extra breakage with your 'fix', well done." rubbed me the wrong way.