[PATCH 1/2] 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 <fgouget(a)codeweavers.com>
---
compareIpAddrRow() and oidToIpForwardRow() should also do byte-swapping
which will be done in the next patch.
---
 dlls/inetmib1/main.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/dlls/inetmib1/main.c b/dlls/inetmib1/main.c
index 90da277375e..d19648ff792 100644
--- a/dlls/inetmib1/main.c
+++ b/dlls/inetmib1/main.c
@@ -772,9 +772,11 @@ static void oidToIpAddrRow(AsnObjectIdentifier *oid, void *dst)
 
 static int __cdecl compareIpAddrRow(const void *a, const void *b)
 {
-    const MIB_IPADDRROW *key = a, *value = b;
+    const MIB_IPADDRROW *rowA = a, *rowB = b;
 
-    return key->dwAddr - value->dwAddr;
+    return rowA->dwAddr < rowB->dwAddr ? -1 : /* avoids overflows */
+           rowA->dwAddr > rowB->dwAddr ? 1 :
+           0;
 }
 
 static BOOL mib2IpAddrQuery(BYTE bPduType, SnmpVarBind *pVarBind,
@@ -865,9 +867,11 @@ static void oidToIpForwardRow(AsnObjectIdentifier *oid, void *dst)
 
 static int __cdecl compareIpForwardRow(const void *a, const void *b)
 {
-    const MIB_IPFORWARDROW *key = a, *value = b;
+    const MIB_IPFORWARDROW *rowA = a, *rowB = b;
 
-    return key->dwForwardDest - value->dwForwardDest;
+    return rowA->dwForwardDest < rowB->dwForwardDest ? -1 : /* avoids overflows */
+           rowA->dwForwardDest > rowB->dwForwardDest ? 1 :
+           0;
 }
 
 static BOOL mib2IpRouteQuery(BYTE bPduType, SnmpVarBind *pVarBind,
@@ -1223,13 +1227,13 @@ 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;
+    const MIB_UDPROW *rowA = a, *rowB = b;
+    ULONG addrA = ntohl( rowA->dwLocalAddr );
+    ULONG addrB = ntohl( rowB->dwLocalAddr );
 
-    ret = ntohl(key->dwLocalAddr) - ntohl(value->dwLocalAddr);
-    if (ret == 0)
-        ret = ntohs(key->dwLocalPort) - ntohs(value->dwLocalPort);
-    return ret;
+    return addrA < addrB ? -1 : /* avoids overflows */
+           addrA > addrB ? 1 :
+           ntohs(rowA->dwLocalPort) - ntohs(rowB->dwLocalPort);
 }
 
 static BOOL mib2UdpEntryQuery(BYTE bPduType, SnmpVarBind *pVarBind,
-- 
2.30.2

[PATCH 2/2] 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 <fgouget(a)codeweavers.com>
---
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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/dlls/inetmib1/main.c b/dlls/inetmib1/main.c
index d19648ff792..d5bcc45a1e9 100644
--- a/dlls/inetmib1/main.c
+++ b/dlls/inetmib1/main.c
@@ -773,9 +773,11 @@ static void oidToIpAddrRow(AsnObjectIdentifier *oid, void *dst)
 static int __cdecl compareIpAddrRow(const void *a, const void *b)
 {
     const MIB_IPADDRROW *rowA = a, *rowB = b;
+    ULONG addrA = ntohl( rowA->dwAddr );
+    ULONG addrB = ntohl( rowB->dwAddr );
 
-    return rowA->dwAddr < rowB->dwAddr ? -1 : /* avoids overflows */
-           rowA->dwAddr > rowB->dwAddr ? 1 :
+    return addrA < addrB ? -1 : /* avoids overflows */
+           addrA > addrB ? 1 :
            0;
 }
 
@@ -868,9 +870,11 @@ static void oidToIpForwardRow(AsnObjectIdentifier *oid, void *dst)
 static int __cdecl compareIpForwardRow(const void *a, const void *b)
 {
     const MIB_IPFORWARDROW *rowA = a, *rowB = b;
+    ULONG addrA = ntohl( rowA->dwForwardDest );
+    ULONG addrB = ntohl( rowB->dwForwardDest );
 
-    return rowA->dwForwardDest < rowB->dwForwardDest ? -1 : /* avoids overflows */
-           rowA->dwForwardDest > rowB->dwForwardDest ? 1 :
+    return addrA < addrB ? -1 : /* avoids overflows */
+           addrA > addrB ? 1 :
            0;
 }
 
-- 
2.30.2

[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=107987

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

[Francois Gouget]

On Mon, 14 Feb 2022, Marvin wrote:
[...]
> Full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=107987
>
> Your paranoid android.
>
> === debian11 (32 bit report) ===
>
> inetmib1:
> main: Timeout

I think these timeouts are caused by discrepencies in the sorting 
between iphlpapi and inetmib1, particularly wrt. byte-swapping.

My understanding is that each library is supposed to sort its tables 
which means the two sets of patches stand on their own. But for the 
sake of the tests it may be best if they go in at about the same time.
(and maybe I should have done a single series for the TestBot, though 
the intermediate patches would still have been in trouble)

-- 
Francois Gouget <fgouget(a)codeweavers.com>