[PATCH v3 1/4] 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. v3: Spelling fix. Resent as one patchset so the tests should succeed on the last part. Note: ipaddrrow_cmp() and ipforward_row_cmp() should do byte-swapping too. This will go in the next patch. 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..6fe0cbc58f4 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 subtraction 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> --- 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 6fe0cbc58f4..041e712f44f 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
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
The difference between two ULONGs may not fit in an int, causing comparison errors. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Introduce DWORD_cmp() to simplify the comparisons. v3: Spelling fix. compareIpAddrRow() and oidToIpForwardRow() should also do byte-swapping which will be done in the next patch. --- dlls/inetmib1/main.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/dlls/inetmib1/main.c b/dlls/inetmib1/main.c index 90da277375e..e66dbe0a811 100644 --- a/dlls/inetmib1/main.c +++ b/dlls/inetmib1/main.c @@ -770,11 +770,15 @@ static void oidToIpAddrRow(AsnObjectIdentifier *oid, void *dst) row->dwAddr = oidToIpAddr(oid); } -static int __cdecl compareIpAddrRow(const void *a, const void *b) +static int __cdecl DWORD_cmp(DWORD a, DWORD b) { - const MIB_IPADDRROW *key = a, *value = b; + return a < b ? -1 : a > b ? 1 : 0; /* a subtraction would overflow */ +} - return key->dwAddr - value->dwAddr; +static int __cdecl compareIpAddrRow(const void *a, const void *b) +{ + const MIB_IPADDRROW *rowA = a, *rowB = b; + return DWORD_cmp(rowA->dwAddr, rowB->dwAddr); } static BOOL mib2IpAddrQuery(BYTE bPduType, SnmpVarBind *pVarBind, @@ -865,9 +869,8 @@ static void oidToIpForwardRow(AsnObjectIdentifier *oid, void *dst) static int __cdecl compareIpForwardRow(const void *a, const void *b) { - const MIB_IPFORWARDROW *key = a, *value = b; - - return key->dwForwardDest - value->dwForwardDest; + const MIB_IPFORWARDROW *rowA = a, *rowB = b; + return DWORD_cmp(rowA->dwForwardDest, rowB->dwForwardDest); } static BOOL mib2IpRouteQuery(BYTE bPduType, SnmpVarBind *pVarBind, @@ -1223,13 +1226,9 @@ static void oidToUdpRow(AsnObjectIdentifier *oid, void *dst) static int __cdecl compareUdpRow(const void *a, const void *b) { - const MIB_UDPROW *key = a, *value = b; - int ret; - - ret = ntohl(key->dwLocalAddr) - ntohl(value->dwLocalAddr); - if (ret == 0) - ret = ntohs(key->dwLocalPort) - ntohs(value->dwLocalPort); - return ret; + const MIB_UDPROW *rowA = a, *rowB = b; + return DWORD_cmp(ntohl(rowA->dwLocalAddr), ntohl(rowB->dwLocalAddr)) || + ntohs(rowA->dwLocalPort) - ntohs(rowB->dwLocalPort); } static BOOL mib2UdpEntryQuery(BYTE bPduType, SnmpVarBind *pVarBind, -- 2.30.2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=108191 Your paranoid android. === debian11 (32 bit report) === inetmib1: main: Timeout === debian11 (32 bit Chinese:China report) === inetmib1: main: Timeout === debian11 (32 bit WoW report) === inetmib1: main: Timeout === debian11 (64 bit WoW report) === inetmib1: main: Timeout
Preferably with the commit msg changed to "inetmib1: ...". Signed-off-by: Huw Davies <huw(a)codeweavers.com>
compareUdpRow() was already performing the byte-swaps but compareIpAddrRow() and compareIpForwardRow() need them too. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Introduce DWORD_cmp() to simplify the comparisons. The byte order was fixed in compareUdpRow() 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/inetmib1/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/inetmib1/main.c b/dlls/inetmib1/main.c index e66dbe0a811..5d925183505 100644 --- a/dlls/inetmib1/main.c +++ b/dlls/inetmib1/main.c @@ -778,7 +778,7 @@ static int __cdecl DWORD_cmp(DWORD a, DWORD b) static int __cdecl compareIpAddrRow(const void *a, const void *b) { const MIB_IPADDRROW *rowA = a, *rowB = b; - return DWORD_cmp(rowA->dwAddr, rowB->dwAddr); + return DWORD_cmp(ntohl(rowA->dwAddr), ntohl(rowB->dwAddr)); } static BOOL mib2IpAddrQuery(BYTE bPduType, SnmpVarBind *pVarBind, @@ -870,7 +870,7 @@ static void oidToIpForwardRow(AsnObjectIdentifier *oid, void *dst) static int __cdecl compareIpForwardRow(const void *a, const void *b) { const MIB_IPFORWARDROW *rowA = a, *rowB = b; - return DWORD_cmp(rowA->dwForwardDest, rowB->dwForwardDest); + return DWORD_cmp(ntohl(rowA->dwForwardDest), ntohl(rowB->dwForwardDest)); } static BOOL mib2IpRouteQuery(BYTE bPduType, SnmpVarBind *pVarBind, -- 2.30.2
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
participants (3)
-
Francois Gouget -
Huw Davies -
Marvin