The difference between two ULONGs may not fit in an int, causing sorting errors.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- Another approach would be to modify all the comparison functions to return a 64-bit value which would ensure no overflow happensi (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 | 60 ++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 15 deletions(-)
diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c index 53b5006d771..0366083d44a 100644 --- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -1595,7 +1595,12 @@ DWORD WINAPI GetIfEntry( MIB_IFROW *row )
static int ifrow_cmp( const void *a, const void *b ) { - return ((const MIB_IFROW*)a)->dwIndex - ((const MIB_IFROW*)b)->dwIndex; + DWORD indexA = ((const MIB_IFROW*)a)->dwIndex; + DWORD indexB = ((const MIB_IFROW*)b)->dwIndex; + + return indexA < indexB ? -1 : /* avoids overflows */ + indexA > indexB ? 1 : + 0; }
/****************************************************************** @@ -1921,7 +1926,12 @@ done:
static int ipaddrrow_cmp( const void *a, const void *b ) { - return ((const MIB_IPADDRROW*)a)->dwAddr - ((const MIB_IPADDRROW*)b)->dwAddr; + DWORD addrA = ((const MIB_IPADDRROW*)a)->dwAddr; + DWORD addrB = ((const MIB_IPADDRROW*)b)->dwAddr; + + return addrA < addrB ? -1 : /* avoids overflows */ + addrA > addrB ? 1 : + 0; }
/****************************************************************** @@ -2028,12 +2038,16 @@ 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; + return rowA->dwForwardDest < rowB->dwForwardDest ? -1 : /* avoids overflows */ + rowA->dwForwardDest > rowB->dwForwardDest ? 1 : + rowA->dwForwardProto < rowB->dwForwardProto ? -1 : + rowA->dwForwardProto > rowB->dwForwardProto ? 1 : + rowA->dwForwardPolicy < rowB->dwForwardPolicy ? -1 : + rowA->dwForwardPolicy > rowB->dwForwardPolicy ? 1 : + rowA->dwForwardNextHop < rowB->dwForwardNextHop ? -1 : + rowA->dwForwardNextHop > rowB->dwForwardNextHop ? 1 : + 0; }
/****************************************************************** @@ -2277,10 +2291,14 @@ err:
static int ipnetrow_cmp( const void *a, const void *b ) { - const MIB_IPNETROW *row_a = a; - const MIB_IPNETROW *row_b = b; + const MIB_IPNETROW *rowA = a; + const MIB_IPNETROW *rowB = b; + ULONG addrA = RtlUlongByteSwap( rowA->dwAddr ); + ULONG addrB = RtlUlongByteSwap( rowB->dwAddr );
- return RtlUlongByteSwap( row_a->dwAddr ) - RtlUlongByteSwap( row_b->dwAddr ); + return addrA < addrB ? -1 : /* avoids overflows */ + addrA > addrB ? 1 : + 0; }
/****************************************************************** @@ -3045,11 +3063,21 @@ static int tcp_row_cmp( const void *a, const void *b ) { const MIB_TCPROW *rowA = a; const MIB_TCPROW *rowB = b; + ULONG addrA, addrB; int ret;
- if ((ret = RtlUlongByteSwap( rowA->dwLocalAddr ) - RtlUlongByteSwap( rowB->dwLocalAddr )) != 0) return ret; + /* Avoid overflows in IPv4 address comparisons */ + addrA = RtlUlongByteSwap( rowA->dwLocalAddr ); + addrB = RtlUlongByteSwap( rowB->dwLocalAddr ); + if (addrA < addrB) return -1; + if (addrA > addrB) return 1; if ((ret = RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort )) != 0) return ret; - if ((ret = RtlUlongByteSwap( rowA->dwRemoteAddr ) - RtlUlongByteSwap( rowB->dwRemoteAddr )) != 0) return ret; + + /* Avoid overflows in IPv4 address comparisons */ + addrA = RtlUlongByteSwap( rowA->dwRemoteAddr ); + addrB = RtlUlongByteSwap( rowB->dwRemoteAddr ); + if (addrA < addrB) return -1; + if (addrA > addrB) return 1; return RtlUshortByteSwap( rowA->dwRemotePort ) - RtlUshortByteSwap( rowB->dwRemotePort ); }
@@ -3434,10 +3462,12 @@ static int udp_row_cmp( const void *a, const void *b ) { const MIB_UDPROW *rowA = a; const MIB_UDPROW *rowB = b; - int ret; + ULONG addrA = RtlUlongByteSwap( rowA->dwLocalAddr ); + ULONG addrB = RtlUlongByteSwap( rowB->dwLocalAddr );
- if ((ret = RtlUlongByteSwap( rowA->dwLocalAddr ) - RtlUlongByteSwap( rowB->dwLocalAddr )) != 0) return ret; - return RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort ); + return addrA < addrB ? -1 : /* avoids overflows */ + addrA > addrB ? 1 : + RtlUshortByteSwap( rowA->dwLocalPort ) - RtlUshortByteSwap( rowB->dwLocalPort ); }
static int udp6_row_cmp( const void *a, const void *b )
On Tue, Feb 15, 2022 at 03:35:37AM +0100, Francois Gouget wrote:
The difference between two ULONGs may not fit in an int, causing sorting errors.
Signed-off-by: Francois Gouget fgouget@codeweavers.com
Another approach would be to modify all the comparison functions to return a 64-bit value which would ensure no overflow happensi (they are all internal functions so that should work).
The IPv6 address comparisons use memcmp() so the resulting value should fit in an int.
How about introducing a DWORD_cmp() helper?
Huw.
On Tue, 15 Feb 2022, Huw Davies wrote:
On Tue, Feb 15, 2022 at 03:35:37AM +0100, Francois Gouget wrote:
The difference between two ULONGs may not fit in an int, causing sorting errors.
Signed-off-by: Francois Gouget fgouget@codeweavers.com
Another approach would be to modify all the comparison functions to return a 64-bit value which would ensure no overflow happensi (they are all internal functions so that should work).
The IPv6 address comparisons use memcmp() so the resulting value should fit in an int.
How about introducing a DWORD_cmp() helper?
That works. I'll resubmit.
-
Francois Gouget -
Huw Davies