Menu
▾
▴
#292 Clean up the address weirdness in ServerLink.cpp
This patch is a little cleanup for ServerLink.cpp. It
corrects several problems/annoyances:
1. Server address isn't stored when creating a link,
getsockname() is called later instead - unnecessarily.
2. Instead, usendaddr is stored. It has the wrong type
(sockaddr instead of sockaddr_in), so there is an
unnecessary copy operation. The problem is: AFAIK
nothing guarrantees that sockaddr is larger than
sockaddr_in. Thus, by copying sockaddr_in into sockaddr
one might overwrite other data.
3. There is also another copy of the server address,
urecvaddr (also wrong type and also memcpy which
potentially overwrites other data). This one is really
weird, it is used as a parameter to recvfrom. I think
it is meant to restrict the addresses one can receive
data from. However, recvfrom ignores the content of the
parameter and overwrites it with the address of the
packet sender.
4. The bind() call for the UDP socket uses the server
address structure. This one has an illegal IP address
(bind() should get a local address to bind to) and a
constant port. This means, we try to bind locally to
the same port as the server's UDP port. If this port is
taken, we fail to open a UDP connection.
My changes:
1. Instead of saving usendaddr and urecvaddr, only
serveraddr is stored, with the correct type. This
variable is initialized when we open the TCP connection
to the server.
2. The code creates a connected UDP socket, with the
server as the other end of the connection. This way we
can use send() and recv() for UDP. The receiver is
automatically the server, and only packets from the
server are accepted.
3. bind() for the UDP socket is called with port 0,
this way the system chooses an open port itself.
Logged In: YES
user_id=8134
The client should and does try to use the port used in the
TCP server connection for it's UDP connection. It should not
use port 0 as the first try. If you'd like to make it retry
with port 0 if the original request failed, that's ok.
I'd like to move towards adding IPv6 support so if we are
changing stored structs we should allow for them to be IPv6
in the future.
Patch for ServerLink.cpp and ServerLink.h (v2)
Logged In: YES
user_id=1074999
Ok, trying to use the same port as the server first now. If
this fails, it will still bind a random port.
As to IPv6 compatibility: storing addresses in the structure
of the wrong type is definitely not the way to go. I checked
it and sockaddr seems to be usually of the same size as
sockaddr_in, so it won't cause any problems (though it still
might on other platforms). But sockaddr_in6 is definitely
bigger than sockaddr, so it won't fit into this field.
Instead, I defined a union type ip_address that can hold a
sockaddr_in as well as sockaddr_in6. There are also two
macros, IS_IPV4_ADDRESS() and IS_IPV6_ADDRESS() that take a
pointer to ip_address and tell you whether the address
inside is IPv4 or IPv6. I put the declaration of the
sockaddr_in6 part into an #ifdef HAVE_IPV6 block, you can
define HAVE_IPV6 later to reflect whether the platform
supports IPv6.
Logged In: YES
user_id=1074999
Missed one small detail (I didn't test on Windows so the
compiler didn't catch it). In ServerLink::ServerLink() I have:
- conn.addr = (CNCTType*)&addr;
+ conn.addr = (CNCTType*)&serveraddr;
conn.saddr = sizeof(addr);
Instead, it should be:
- conn.addr = (CNCTType*)&addr;
- conn.saddr = sizeof(addr);
+ conn.addr = (CNCTType*)&serveraddr;
+ conn.saddr = sizeof(serveraddr);
Logged In: YES
user_id=1074999
Out of interest: why is this ignored? If it is a plain "no
time to test / check in" then I can't help. But if you
prefer to combine this change with adding IPv6 support then
I could do it for you. Or, if you see any other problem with
the patch - just tell please.
Logged In: YES
user_id=8134
Just been busy and haven't gotten to it. I do appreciate the
effort though! I'll get to it soon.