[PATCH v2 1/2] iphlpapi: Fix ULONG comparisons, most notably for IPv4 addresses.
The difference between two ULONGs may not fit in an int, causing sorting errors. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Introduce DWORD_cmp() to simplify the comparisons. Note: ipaddrrow_cmp() and ipforward_row_cmp() should do byte-swapping too. This will go in the next patch. Another approach would be to modify all the comparison functions to return a 64-bit value which would ensure no overflow happens (they are all internal functions so that should work). The IPv6 address comparisons use memcmp() so the resulting value should fit in an int. My computer has UDP servers listening on 192.* and 0.* but the 192.* entries preceeded the 0.* ones in the UDP table returned by GetUdpTable(). Because of this SnmpExtensionQuery(SNMP_PDU_GETNEXT) found that the last matching UDP entry was lower than the table's first entry and thus returned the first entry instead of the next one. This resulted in an infinite loop in inetmib1:main depending on the exact set of running UDP servers and IPv4 addresses. https://test.winehq.org/data/patterns.html#inetmib1:main --- dlls/iphlpapi/iphlpapi_main.c | 45 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 53b5006d771..2a8086292b9 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -1593,9 +1593,15 @@ DWORD WINAPI GetIfEntry( MIB_IFROW *row ) return err; } +static int DWORD_cmp( DWORD a, DWORD b ) +{ + return a < b ? -1 : a > b ? 1 : 0; /* a substraction would overflow */ +} + static int ifrow_cmp( const void *a, const void *b ) { - return ((const MIB_IFROW*)a)->dwIndex - ((const MIB_IFROW*)b)->dwIndex; + const MIB_IFROW *rowA = a, *rowB = b; + return DWORD_cmp(rowA->dwIndex, rowB->dwIndex); } /****************************************************************** @@ -1921,7 +1927,8 @@ done: static int ipaddrrow_cmp( const void *a, const void *b ) { - return ((const MIB_IPADDRROW*)a)->dwAddr - ((const MIB_IPADDRROW*)b)->dwAddr; + const MIB_IPADDRROW *rowA = a, *rowB = b; + return DWORD_cmp(rowA->dwAddr, rowB->dwAddr); } /****************************************************************** @@ -2026,14 +2033,11 @@ DWORD WINAPI AllocateAndGetIpAddrTableFromStack( MIB_IPADDRTABLE **table, BOOL s static int ipforward_row_cmp( const void *a, const void *b ) { - const MIB_IPFORWARDROW *rowA = a; - const MIB_IPFORWARDROW *rowB = b; - int ret; - - if ((ret = rowA->dwForwardDest - rowB->dwForwardDest) != 0) return ret; - if ((ret = rowA->dwForwardProto - rowB->dwForwardProto) != 0) return ret; - if ((ret = rowA->dwForwardPolicy - rowB->dwForwardPolicy) != 0) return ret; - return rowA->dwForwardNextHop - rowB->dwForwardNextHop; + const MIB_IPFORWARDROW *rowA = a, *rowB = b; + return DWORD_cmp(rowA->dwForwardDest, rowB->dwForwardDest) || + DWORD_cmp(rowA->dwForwardProto, rowB->dwForwardProto) || + DWORD_cmp(rowA->dwForwardPolicy, rowB->dwForwardPolicy) || + DWORD_cmp(rowA->dwForwardNextHop, rowB->dwForwardNextHop); } /****************************************************************** @@ -2277,10 +2281,8 @@ err: static int ipnetrow_cmp( const void *a, const void *b ) { - const MIB_IPNETROW *row_a = a; - const MIB_IPNETROW *row_b = b; - - return RtlUlongByteSwap( row_a->dwAddr ) - RtlUlongByteSwap( row_b->dwAddr ); + const MIB_IPNETROW *rowA = a, *rowB = b; + return DWORD_cmp(RtlUlongByteSwap( rowA->dwAddr ), RtlUlongByteSwap( rowB->dwAddr )); } /****************************************************************** @@ -3043,13 +3045,12 @@ static void tcp_row_fill( void *table, DWORD num, ULONG family, ULONG table_clas static int tcp_row_cmp( const void *a, const void *b ) { - const MIB_TCPROW *rowA = a; - const MIB_TCPROW *rowB = b; + const MIB_TCPROW *rowA = a, *rowB = b; int ret; - if ((ret = RtlUlongByteSwap( rowA->dwLocalAddr ) - RtlUlongByteSwap( rowB->dwLocalAddr )) != 0) return ret; + if ((ret = DWORD_cmp(RtlUshortByteSwap( rowA->dwLocalAddr ), RtlUshortByteSwap( rowB->dwLocalAddr ))) != 0) return ret; if ((ret = RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort )) != 0) return ret; - if ((ret = RtlUlongByteSwap( rowA->dwRemoteAddr ) - RtlUlongByteSwap( rowB->dwRemoteAddr )) != 0) return ret; + if ((ret = DWORD_cmp(RtlUshortByteSwap( rowA->dwRemoteAddr ), RtlUshortByteSwap( rowB->dwRemoteAddr ))) != 0) return ret; return RtlUshortByteSwap( rowA->dwRemotePort ) - RtlUshortByteSwap( rowB->dwRemotePort ); } @@ -3432,12 +3433,10 @@ static void udp_row_fill( void *table, DWORD num, ULONG family, ULONG table_clas static int udp_row_cmp( const void *a, const void *b ) { - const MIB_UDPROW *rowA = a; - const MIB_UDPROW *rowB = b; - int ret; + const MIB_UDPROW *rowA = a, *rowB = b; - if ((ret = RtlUlongByteSwap( rowA->dwLocalAddr ) - RtlUlongByteSwap( rowB->dwLocalAddr )) != 0) return ret; - return RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort ); + return DWORD_cmp(RtlUlongByteSwap( rowA->dwLocalAddr), RtlUlongByteSwap( rowB->dwLocalAddr )) || + RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort ); } static int udp6_row_cmp( const void *a, const void *b ) -- 2.30.2
ipaddrrow_cmp() and ipforward_row_cmp() must perform byte-swapping in order for the rows to be sorted as expected. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Introduce DWORD_cmp() to simplify the comparisons. Relatedly, the byte order was fixed in compareUdpRow() inetmib1 for bug 52224: https://bugs.winehq.org/show_bug.cgi?id=52224 There is no specific test for this but adding traces showed that the other two comparison functions were also operating on the wrong byte order. --- dlls/iphlpapi/iphlpapi_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 2a8086292b9..fd19b83937c 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -1928,7 +1928,7 @@ done: static int ipaddrrow_cmp( const void *a, const void *b ) { const MIB_IPADDRROW *rowA = a, *rowB = b; - return DWORD_cmp(rowA->dwAddr, rowB->dwAddr); + return DWORD_cmp(RtlUlongByteSwap( rowA->dwAddr ), RtlUlongByteSwap( rowB->dwAddr )); } /****************************************************************** @@ -2034,10 +2034,10 @@ DWORD WINAPI AllocateAndGetIpAddrTableFromStack( MIB_IPADDRTABLE **table, BOOL s static int ipforward_row_cmp( const void *a, const void *b ) { const MIB_IPFORWARDROW *rowA = a, *rowB = b; - return DWORD_cmp(rowA->dwForwardDest, rowB->dwForwardDest) || + return DWORD_cmp(RtlUlongByteSwap( rowA->dwForwardDest ), RtlUlongByteSwap( rowB->dwForwardDest )) || DWORD_cmp(rowA->dwForwardProto, rowB->dwForwardProto) || DWORD_cmp(rowA->dwForwardPolicy, rowB->dwForwardPolicy) || - DWORD_cmp(rowA->dwForwardNextHop, rowB->dwForwardNextHop); + DWORD_cmp(RtlUlongByteSwap( rowA->dwForwardNextHop ), RtlUlongByteSwap( rowB->dwForwardNextHop )); } /****************************************************************** -- 2.30.2
On Tue, Feb 15, 2022 at 03:31:10PM +0100, Francois Gouget wrote:
+static int DWORD_cmp( DWORD a, DWORD b ) +{ + return a < b ? -1 : a > b ? 1 : 0; /* a substraction would overflow */
The patch looks fine, but to save yourself having to send in a spelling fix later on, let's get it right from the start ;-) This is also applies to the inetmib series. Huw.
participants (2)
-
Francois Gouget -
Huw Davies