From: shavitush the@shav.it
MulDiv on Windows returns wildly different results from WINE when given certain values - e.g. MulDiv(1, 0x80000000, 0x80000000) - returns 0 on WINE (32-bit/64-bit). But on Windows, it returns 2 on 32-bit, and -1 on 64-bit.
Both the 32-bit and 64-bit implementations from Microsoft suffer from different logical bugs; this commit implements MulDiv just like Windows does on both 32-bit kernel32.dll, and 64-bit kernelbase.dll, also transforming the undefined behaviors into defined behavior so it lines up with Windows. --- dlls/kernel32/kernel_main.c | 131 ++++++++++++++++++++++++++++++---- dlls/kernel32/tests/process.c | 23 ++++++ 2 files changed, 139 insertions(+), 15 deletions(-)
diff --git a/dlls/kernel32/kernel_main.c b/dlls/kernel32/kernel_main.c index ae16195a550..ed44ee38653 100644 --- a/dlls/kernel32/kernel_main.c +++ b/dlls/kernel32/kernel_main.c @@ -22,6 +22,7 @@ #include <stdarg.h> #include <string.h> #include <signal.h> +#include <limits.h>
#include "windef.h" #include "winbase.h" @@ -170,33 +171,133 @@ BOOL WINAPI DllMain( HINSTANCE hinst, DWORD reason, LPVOID reserved ) }
/*********************************************************************** - * MulDiv (KERNEL32.@) + * MulDiv (KERNEL32.@, KERNELBASE.@) * RETURNS * Result of multiplication and division * -1: Overflow occurred or Divisor was 0 */ -INT WINAPI MulDiv( INT nMultiplicand, INT nMultiplier, INT nDivisor) +INT WINAPI MulDiv( INT nNumber, INT nNumerator, INT nDenominator ) { - LONGLONG ret; +#ifdef _WIN64 + unsigned __int64 abs_denominator, intermediate64, unsigned_quotient; + int abs_numerator, abs_number;
- if (!nDivisor) return -1; + abs_denominator = (unsigned int)-nDenominator;
- /* We want to deal with a positive divisor to simplify the logic. */ - if (nDivisor < 0) + if (nDenominator > 0) { - nMultiplicand = - nMultiplicand; - nDivisor = -nDivisor; + abs_denominator = (unsigned int)nDenominator; + } + + abs_numerator = -nNumerator; + + if (nNumerator >= 0) + { + abs_numerator = nNumerator; + } + + abs_number = -nNumber; + + if (nNumber >= 0) + { + abs_number = nNumber; + } + + intermediate64 = ((unsigned __int64)(unsigned int)abs_denominator >> 1) + abs_number * (__int64)abs_numerator; + + if ((unsigned int)abs_denominator <= ((DWORD)((intermediate64 >> 32) & 0xFFFFFFFFU))) + { + return -1; + } + + unsigned_quotient = intermediate64 / abs_denominator; + + if ((unsigned_quotient & INT_MIN) != 0LL) + { + return -1; + } + + // sign check + if ((nDenominator ^ nNumerator ^ nNumber) >= 0) + { + return unsigned_quotient; + } + + return -(int)unsigned_quotient; +#else +#define UINT32_ABS(x) \ + ((x) == INT_MIN ? INT_MIN : \ + (x) < 0 ? (unsigned int)(-(x)) : \ + (unsigned int)(x)) + + unsigned int abs_number, abs_numerator, abs_denominator, rounding_term, unsigned_quotient; + unsigned __int64 product, rounded_product; + + abs_number = UINT32_ABS(nNumber); + abs_numerator = UINT32_ABS(nNumerator); + abs_denominator = UINT32_ABS(nDenominator); + product = (unsigned __int64)abs_numerator * abs_number; + + // Assume nDenominator == INT_MIN + // In 32-bit MSVC, this results in it INT_MIN itself due to how two's complement arithmetic works, + // and the lack of a positive equivalent value within the valid range for int32. So.. undefined behavior. + // That makes `-nDenominator` effectively evaluate to `0x80000000` (`INT_MIN`). + // The original code performs `-nDenominator >> 1`. At that point, MSVC thinks it's a signed integer! + // Therefore, it compiles to `sar` instead of `shr`. + // Disassembly from kernel32.dll reads: + // neg ecx ; Two's Complement Negation + // push ecx + // sar ecx, 1 ; Shift Arithmetic Right + // + // Note - sar and not shr! + // + // 0x80000000 is `1000 0000 0000 0000 0000 0000 0000 0000` + // In that case, `sar ecx, 1` results in `1100 0000 0000 0000 0000 0000 0000 0000`, which is.. as you guessed - 0xC0000000. + if (nDenominator == INT_MIN) + { + rounding_term = 0xC0000000U; + } + + else + { + rounding_term = abs_denominator >> 1; + } + + rounded_product = product + rounding_term; + + if (rounded_product < product || + (rounded_product >> 32) >= abs_denominator) + { + return -1; + } + + unsigned_quotient = (unsigned int)(rounded_product / abs_denominator); + + if ((nDenominator ^ nNumerator ^ nNumber) >= 0) + { + if (unsigned_quotient > INT_MIN) + { + return -1; + } + + return (int)unsigned_quotient; }
- /* If the result is positive, we "add" to round. else, we subtract to round. */ - if ( ( (nMultiplicand < 0) && (nMultiplier < 0) ) || - ( (nMultiplicand >= 0) && (nMultiplier >= 0) ) ) - ret = (((LONGLONG)nMultiplicand * nMultiplier) + (nDivisor/2)) / nDivisor; else - ret = (((LONGLONG)nMultiplicand * nMultiplier) - (nDivisor/2)) / nDivisor; + { + if (unsigned_quotient > INT_MIN) + { + return -1; + } + + else if (unsigned_quotient == INT_MIN) + { + return INT_MIN; + } + }
- if ((ret > 2147483647) || (ret < -2147483647)) return -1; - return ret; + return -(int)unsigned_quotient; +#endif }
/****************************************************************************** diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 8e168622c17..30dab76b3c0 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -5547,6 +5547,28 @@ static void test_GetProcessInformation(void) } }
+static void test_MulDiv(void) +{ + // Test for undefined behavior in Microsoft's MulDiv persisting in WINE, + // as well as normal use/quirks between 32-bit/64-bit +#ifdef _WIN64 + ok(MulDiv(1, 0x80000000, 0x80000000) == -1, "UB not happening\n"); + ok(MulDiv(5, -1, -1) == 5, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 0x80000000, -1) == -1, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 0x80000000, 0x80000000) == -1, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 5, 0x80000000) == 0, "Unexpected MulDiv result\n"); +#else + ok(MulDiv(1, 0x80000000, 0x80000000) == 2, "UB not happening\n"); + ok(MulDiv(5, -1, -1) == 5, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 0x80000000, -1) == -1, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 0x80000000, 0x80000000) == 6, "Unexpected MulDiv result\n"); + ok(MulDiv(5, 5, 0x80000000) == -1, "Unexpected MulDiv result\n"); +#endif + + ok(MulDiv(6, 6, 6) == 6, "Unexpected MulDiv result\n"); + ok(MulDiv(6, 6, 0) == -1, "Unexpected MulDiv result\n"); +} + START_TEST(process) { HANDLE job, hproc, h, h2; @@ -5677,6 +5699,7 @@ START_TEST(process) test_services_exe(); test_startupinfo(); test_GetProcessInformation(); + test_MulDiv();
/* things that can be tested: * lookup: check the way program to be executed is searched