Re: [PATCH] ws2_32: Implement automatic broadcast for UDP sockets on sendto() (try 2)
On Sat, 2016-12-03 at 00:15 -0200, Bruno Jesus wrote:
@@ -2446,8 +2476,32 @@ static int WS2_send( int fd, struct ws2_async *wsa, int flags ) return -1; }
+ if (wsa->addr->sa_family == WS_AF_INET) + { + /* When the target IPv4 address ends in 255 we are going to check if + * it matches the broadcast address. Trying to send the packet without + * setting SO_BROADCAST results in EACCES, to avoid that we will enable + * the flag and send the packet, after that we will restore the flag + * This is the most common estimate to reduce the number of UDP packets + * that we need to check. */ + struct sockaddr_in *addr = (struct sockaddr_in*) hdr.msg_name; + in_addr_t address = addr->sin_addr.s_addr; + + if (address != INADDR_BROADCAST && (address & 0xFF000000) == 0xFF000000)
This doesn't work for a /25 network, for example. You really need to use the network mask here to calculate the broadcast address.
+/* required for broadcast tests */ +static void disable_firewall(void) +{ + system("netsh firewall set opmode disable"); /* XP */ + system("netsh Advfirewall set allprofiles state off"); /* >= Vista*/ +} + +static void enable_firewall(void) +{ + system("netsh firewall set opmode enable"); /* XP */ + system("netsh Advfirewall set allprofiles state on"); /* >= Vista*/ +}
This will not work if the test is run as an unprivileged user. It would be better to use an API like INetFwMgr and skip the affected tests when you can't modify the firewall. I'm working on a fix for a similar problem in the rpcrt4 test. I can share a patch if you're interested.
On Mon, Dec 5, 2016 at 8:38 AM, Hans Leidekker <hans(a)codeweavers.com> wrote:
On Sat, 2016-12-03 at 00:15 -0200, Bruno Jesus wrote:
@@ -2446,8 +2476,32 @@ static int WS2_send( int fd, struct ws2_async *wsa, int flags ) return -1; }
+ if (wsa->addr->sa_family == WS_AF_INET) + { + /* When the target IPv4 address ends in 255 we are going to check if + * it matches the broadcast address. Trying to send the packet without + * setting SO_BROADCAST results in EACCES, to avoid that we will enable + * the flag and send the packet, after that we will restore the flag + * This is the most common estimate to reduce the number of UDP packets + * that we need to check. */ + struct sockaddr_in *addr = (struct sockaddr_in*) hdr.msg_name; + in_addr_t address = addr->sin_addr.s_addr; + + if (address != INADDR_BROADCAST && (address & 0xFF000000) == 0xFF000000)
This doesn't work for a /25 network, for example. You really need to use the network mask here to calculate the broadcast address.
Hi, thanks for the review. As I wrote in the comments this is the most common estimate, it should be enough for the majority of home networks everywhere. So far there is only one application affected too which is a game. Checking if every packet is UDP and if every UDP packet is not in broadcast state and then checking if its address is a broadcast would be take too much time in a operation that has to be very fast. Unless I'm missing something as I said in the first version of the patch.
+/* required for broadcast tests */ +static void disable_firewall(void) +{ + system("netsh firewall set opmode disable"); /* XP */ + system("netsh Advfirewall set allprofiles state off"); /* >= Vista*/ +} + +static void enable_firewall(void) +{ + system("netsh firewall set opmode enable"); /* XP */ + system("netsh Advfirewall set allprofiles state on"); /* >= Vista*/ +}
This will not work if the test is run as an unprivileged user. It would be better to use an API like INetFwMgr and skip the affected tests when you can't modify the firewall.
I'm working on a fix for a similar problem in the rpcrt4 test. I can share a patch if you're interested.
Ok, no problem. Please share the patch with me.
On Mon, 2016-12-05 at 10:04 -0200, Bruno Jesus wrote:
On Mon, Dec 5, 2016 at 8:38 AM, Hans Leidekker <hans(a)codeweavers.com> wrote:
On Sat, 2016-12-03 at 00:15 -0200, Bruno Jesus wrote:
@@ -2446,8 +2476,32 @@ static int WS2_send( int fd, struct ws2_async *wsa, int flags ) return -1; }
+ if (wsa->addr->sa_family == WS_AF_INET) + { + /* When the target IPv4 address ends in 255 we are going to check if + * it matches the broadcast address. Trying to send the packet without + * setting SO_BROADCAST results in EACCES, to avoid that we will enable + * the flag and send the packet, after that we will restore the flag + * This is the most common estimate to reduce the number of UDP packets + * that we need to check. */ + struct sockaddr_in *addr = (struct sockaddr_in*) hdr.msg_name; + in_addr_t address = addr->sin_addr.s_addr; + + if (address != INADDR_BROADCAST && (address & 0xFF000000) == 0xFF000000)
This doesn't work for a /25 network, for example. You really need to use the network mask here to calculate the broadcast address.
Hi, thanks for the review. As I wrote in the comments this is the most common estimate, it should be enough for the majority of home networks everywhere. So far there is only one application affected too which is a game. Checking if every packet is UDP and if every UDP packet is not in broadcast state and then checking if its address is a broadcast would be take too much time in a operation that has to be very fast. Unless I'm missing something as I said in the first version of the patch.
You are trading correctness for performance, and even with this filter the getsockopt calls are probably too expensive. I guess this information should somehow be cached.
On Mon, Dec 5, 2016 at 11:15 AM, Hans Leidekker <hans(a)codeweavers.com> wrote:
This doesn't work for a /25 network, for example. You really need to use the network mask here to calculate the broadcast address.
Hi, thanks for the review. As I wrote in the comments this is the most common estimate, it should be enough for the majority of home networks everywhere. So far there is only one application affected too which is a game. Checking if every packet is UDP and if every UDP packet is not in broadcast state and then checking if its address is a broadcast would be take too much time in a operation that has to be very fast. Unless I'm missing something as I said in the first version of the patch.
You are trading correctness for performance, and even with this filter the getsockopt calls are probably too expensive. I guess this information should somehow be cached.
I agree with you, it is a tradeoff. The filter is fast because the patch reduces the scope of the test to only a few handful of packets that are probably broadcast addresses, only then it will check the broadcast flag and in the kernel this is very fast because judging by the kernel source code. Only if previous conditions are met it will check if it really was a broadcast address. A different approach would be to let it hit EACCES when sending and check for the error and only then do all tests, that would still work and still be only a handful of packets but then we could check for the broadcast address everytime without suffering from performance and getting perfect correctness. What do you think?
On Mon, Dec 5, 2016 at 6:28 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
... A different approach would be to let it hit EACCES when sending and check for the error and only then do all tests, that would still work and still be only a handful of packets but then we could check for the broadcast address everytime without suffering from performance and getting perfect correctness. What do you think?
Oh, I like that idea. If I had thought of it then I probably would have done it that way in the first place... Best, Erich
On Mon, 2016-12-05 at 11:28 -0200, Bruno Jesus wrote:
You are trading correctness for performance, and even with this filter the getsockopt calls are probably too expensive. I guess this information should somehow be cached.
I agree with you, it is a tradeoff. The filter is fast because the patch reduces the scope of the test to only a few handful of packets that are probably broadcast addresses, only then it will check the broadcast flag and in the kernel this is very fast because judging by the kernel source code. Only if previous conditions are met it will check if it really was a broadcast address.
It's still a context switch. Couldn't there be more than a handful of broadcast packets?
A different approach would be to let it hit EACCES when sending and check for the error and only then do all tests, that would still work and still be only a handful of packets but then we could check for the broadcast address everytime without suffering from performance and getting perfect correctness. What do you think?
That would get rid of some the performance cost (more importantly it would no longer hurt the general case), but it would still require get/setsockopt calls for every broadcast packet.
On Mon, Dec 5, 2016 at 12:09 PM, Hans Leidekker <hans(a)codeweavers.com> wrote:
On Mon, 2016-12-05 at 11:28 -0200, Bruno Jesus wrote:
You are trading correctness for performance, and even with this filter the getsockopt calls are probably too expensive. I guess this information should somehow be cached.
I agree with you, it is a tradeoff. The filter is fast because the patch reduces the scope of the test to only a few handful of packets that are probably broadcast addresses, only then it will check the broadcast flag and in the kernel this is very fast because judging by the kernel source code. Only if previous conditions are met it will check if it really was a broadcast address.
It's still a context switch. Couldn't there be more than a handful of broadcast packets?
A different approach would be to let it hit EACCES when sending and check for the error and only then do all tests, that would still work and still be only a handful of packets but then we could check for the broadcast address everytime without suffering from performance and getting perfect correctness. What do you think?
That would get rid of some the performance cost (more importantly it would no longer hurt the general case), but it would still require get/setsockopt calls for every broadcast packet.
Just as background: Old games used broadcast to help auto finding other players in the same network, I have seen a handful of network messaging programs that also did that to simplify file transfer by finding if the user is in the same network avoiding a server relay. Some games that use broadcast are Command & Conquer, Age of Empires and Worms 2. This is not something you do 100 times per second, eg C&C used a 3 second interval. But none of these games are not affected by this bug. This bug is about a very particular case in the broadcast implementation of Windows that allows a broadcast without explicit SO_BROADCAST. We have 2 case scenarios by using the EACCES approach: 1 - The socket is SOCK_RAW and that is why it was not allowed; 2 - The socket is UDP and SO_BROADCAST was not enabled. The first is discarded as soon as we check the socket type, which uses SO_TYPE. Then we will check if the SO_BROADCAST was disabled and only then check the broadcast address. In other words, for every packet that hits EACCES (overly extremely rare) we will call SO_TYPE and then check the UDP stuff is required and if yes try resending the packet. There won't be any performance hit in the standard socket usage (data sending using tcp/udp), only when EACCES happens that we will need to do something.
participants (3)
-
Bruno Jesus -
Erich E. Hoover -
Hans Leidekker