[PATCH 0/1] MR7916: kernel32: Fixed MulDiv's inconsistencies with Windows.
`MulDiv` doesn't match the correct Windows behavior for the function, which actually differs in some edge cases for 32-bit and 64-bit. Test case: ```c MulDiv(1, 0x80000000, 0x80000000); // WINE - both 32-bit and 64-bit: 0 // Windows (32-bit/kernel32.dll): 2 // Windows (64-bit/kernelbase.dll): -1 ``` On Windows 32-bit, this would return 2. On 64-bit, it'd return -1. However, on WINE, this call would return 0 for both 32-bit and 64-bit. This discrepancy was previously abused by threat actors to detect if the application runs under WINE. --- On Windows, `MulDiv` has a different implementation for 32-bit (from `kernel32.dll`) and 64-bit (leveraged to `kernelbase.dll` from `kernel32.dll`). After adjusting the 64-bit code to be an exact replica of the original code from `kernelbase.dll`, it'd still return the incorrect value due to Microsoft's code having undefined behavior with negation. It could be avoided with `-O0` but I opted into rewriting it with the undefined behavior to be, well.. defined. To clarify, the original code from Microsoft has a bug (logically, at least). But WINE is supposed to have matching behavior so we can run Windows programs as intended. The 32-bit version of `MulDiv` in Windows has a more complicated bug; I've documented it in the code. --- This is my first contribution to WINE - I cannot find any `CONTRIBUTING.md` or similar file with guidelines so I did my best to follow the convention of other merge requests. I've added unit testing for `MulDiv` to ensure there won't be a regression in the future. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916
From: shavitush <the(a)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 -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7916
Thank you for your interest in contributing to Wine. Unfortunately, we must reject this MR (and possibly your future MRs) for two critical reasons: 1. Wine development strictly prohibits examining disassembled Microsoft code. Your reference to making the code "an exact replica of the original code from kernelbase.dll" indicates a cleanroom violation. 2. We generally only fix API discrepancies when there are real-world applications affected by them. While you mention malware (Raspberry Robin) exploiting this difference, malware detection is not a use case we aim to support. Please consult our developer guidelines at https://gitlab.winehq.org/wine/wine/-/wikis/Developers and patch submission process at https://gitlab.winehq.org/wine/wine/-/wikis/Submitting-Patches. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101964
This merge request was closed by Shavit Yosef. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916
On Sat Apr 26 23:19:31 2025 +0000, Jinoh Kang wrote:
Thank you for your interest in contributing to Wine. Unfortunately, we must reject this MR (and possibly your future MRs) for two critical reasons: 1. Wine development strictly prohibits examining disassembled Microsoft code. Your reference to making the code "an exact replica of the original code from kernelbase.dll" indicates a cleanroom violation. 2. We generally only fix API discrepancies when there are real-world applications affected by them. While you mention malware (Raspberry Robin) exploiting this difference, malware detection is not a use case we aim to support. Please consult our developer guidelines at https://gitlab.winehq.org/wine/wine/-/wikis/Developers and patch submission process at https://gitlab.winehq.org/wine/wine/-/wikis/Submitting-Patches. Understood. Apologies for wasting your precious time; and thanks for your work on WINE!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101965
We generally only fix API discrepancies when there are real-world applications affected by them. While you mention malware (Raspberry Robin) exploiting this difference, malware detection is not a use case we aim to support.
Is that a general rule? Surely there are cases in which emulating the exact Windows behavior is so complicated that it's not worth the effort if there are not applications depending on that, but when emulating the Windows behavior is not particularly difficult it doesn't make sense to make Wine incorrect just because you don't have a real application. I've contributed plenty of changes that I had devised from tests, even without an application depending on them. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101982
On Sun Apr 27 15:51:46 2025 +0000, Giovanni Mascellani wrote:
We generally only fix API discrepancies when there are real-world applications affected by them. While you mention malware (Raspberry Robin) exploiting this difference, malware detection is not a use case we aim to support. Is that a general rule? Surely there are cases in which emulating the exact Windows behavior is so complicated that it's not worth the effort if there are not applications depending on that, but when emulating the Windows behavior is not particularly difficult it doesn't make sense to make Wine incorrect just because you don't have a real application. I've contributed plenty of changes that I had devised from tests, even without an application depending on them. For implementing a *new* API it's generally desirable to match Windows, but doing that too much is sometimes a waste of time for both the author and the maintainer. Touching existing API, on the other hand, requires scrutiny due to possibility of regression etc..
"Try to implement accurately for known-used code path(s), and leave a FIXME() for others" seems like the sweet spot for me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101985
On Sun Apr 27 15:55:50 2025 +0000, Jinoh Kang wrote:
For implementing a *new* API it's generally desirable to match Windows, but doing that too much is sometimes a waste of time for both the author and the maintainer. Touching existing API, on the other hand, requires scrutiny due to possibility of regression etc.. "Try to implement accurately for known-used code path(s), and leave a FIXME() for others" seems like the sweet spot for me. By the way, personally I think mismatches for unused code path is a good idea as long as it makes the code simpler. We can try to implement every trivial edge case, but then they collectively increase maintenance burden and risks making them too much close to some copyrighted commercial Win32 implementation from Redmond for no good reason that Wine could argue in court, IMHO.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101987
On Sun Apr 27 16:01:27 2025 +0000, Jinoh Kang wrote:
By the way, personally I think mismatches for unused code path is a good idea as long as it makes the code simpler. We can try to implement every trivial edge case, but then they collectively increase maintenance burden and risks making them too close to some copyrighted commercial Win32 implementation from Redmond for no good reason that Wine could argue in court, IMHO. Here's an example:
We implement a bunch of Native APIs (Nt\*). Wait, aren't they internal APIs? Aren't they copyrightable? No? Because a lot of applications decided to rely directly on those internal APIs and made it technically and physically impossible to fairly compete with other Win32 implementation without adding those internals? The antitrust laws agree, right? Well how about, like, win32k and wow64? Do we have known apps that rely on them? Or maybe merely copying the export names (but not the parameter types and such) are considered fair use? Maybe not, but covered by another clause? Act? In other legislations? Are those questions even valid? Is it even profitable to test these in the court? Are we lawyers? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7916#note_101992
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Jinoh Kang (＠iamahuman)
-
Shavit Yosef (＠shavitush)
-
shavitush