[PATCH v3 1/4] iphlpapi: Fix ULONG comparisons, most notably for IPv4 addresses.

[Francois Gouget]

The difference between two ULONGs may not fit in an int, causing sorting
errors.

Signed-off-by: Francois Gouget 
---
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 )

[PATCH v3 2/4] iphlpapi: Fix the IPv4 address network order for comparisons.

[Francois Gouget]

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 
---
 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 ));
 }

/******************************************************************

[PATCH v3 2/4] iphlpapi: Fix the IPv4 address network order for comparisons.

[Huw Davies]

Signed-off-by: Huw Davies

[PATCH v3 3/4] inetmib: Avoid overflows in IPv4 address comparisons.

[Francois Gouget]

The difference between two ULONGs may not fit in an int, causing
comparison errors.

Signed-off-by: Francois Gouget 
---
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,

[PATCH v3 3/4] inetmib: Avoid overflows in IPv4 address comparisons.

[Marvin]

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

[PATCH v3 3/4] inetmib: Avoid overflows in IPv4 address comparisons.

[Huw Davies]

Preferably with the commit msg changed to "inetmib1: ...".

Signed-off-by: Huw Davies

[PATCH v3 4/4] inetmib1: Fix the IPv4 address network order for comparisons.

[Francois Gouget]

compareUdpRow() was already performing the byte-swaps but
compareIpAddrRow() and compareIpForwardRow() need them too.

Signed-off-by: Francois Gouget 
---
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,

[PATCH v3 4/4] inetmib1: Fix the IPv4 address network order for comparisons.

[Huw Davies]

Signed-off-by: Huw Davies

[Huw Davies]

Signed-off-by: Huw Davies