[PATCH 0/1] MR3458: ddraw/tests: Fix overflow bugs in compare_float.
1. Subtracting integers may result in an overflow or underflow. 2. Right at the 'edge' of underflowing, the result of subtraction may be `INT_MIN`, and the call to `abs()` will also result in `INT_MIN`. This fix accounts for all of these conditions. EXAMPLES: 1. can be encountered by comparing 2.0 and -2.0 2. can be encountered by comparing -2.0 and 2.0 NOTE: There are 14 more instances of `compare_float` across several modules. I would like to make sure the logic is sound with this MR, then I can take on the rest. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
From: Jeff Smith <whydoubt(a)gmail.com> 1. Subtracting integers may result in an overflow or underflow. 2. Right at the 'edge' of underflowing, the result of subtraction may be INT_MIN, and the call to abs() will also result in INT_MIN. This fix accounts for all of these conditions. --- dlls/ddraw/tests/ddraw1.c | 9 ++++++++- dlls/ddraw/tests/ddraw2.c | 9 ++++++++- dlls/ddraw/tests/ddraw4.c | 9 ++++++++- dlls/ddraw/tests/ddraw7.c | 9 ++++++++- 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index cde80c58e99..48ce8214180 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -54,13 +54,20 @@ static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; + int delta; if (x < 0) x = INT_MIN - x; if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) + delta = x - y; + + /* Check for overflow and related conditions */ + if (((x ^ y) & (x ^ delta)) < 0 || delta == INT_MIN) + return FALSE; + + if (abs(delta) > ulps) return FALSE; return TRUE; diff --git a/dlls/ddraw/tests/ddraw2.c b/dlls/ddraw/tests/ddraw2.c index 86056014f96..0b226c97d3e 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -55,13 +55,20 @@ static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; + int delta; if (x < 0) x = INT_MIN - x; if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) + delta = x - y; + + /* Check for overflow and related conditions */ + if (((x ^ y) & (x ^ delta)) < 0 || delta == INT_MIN) + return FALSE; + + if (abs(delta) > ulps) return FALSE; return TRUE; diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 4458cea5145..93ad8636a17 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -61,13 +61,20 @@ static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; + int delta; if (x < 0) x = INT_MIN - x; if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) + delta = x - y; + + /* Check for overflow and related conditions */ + if (((x ^ y) & (x ^ delta)) < 0 || delta == INT_MIN) + return FALSE; + + if (abs(delta) > ulps) return FALSE; return TRUE; diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index bbb78169aea..b3a0a60af87 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -72,13 +72,20 @@ static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; int y = *(int *)&g; + int delta; if (x < 0) x = INT_MIN - x; if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) + delta = x - y; + + /* Check for overflow and related conditions */ + if (((x ^ y) & (x ^ delta)) < 0 || delta == INT_MIN) + return FALSE; + + if (abs(delta) > ulps) return FALSE; return TRUE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135454 Your paranoid android. === w10pro64 (32 bit report) === ddraw: ddraw1.c:8058: Test failed: Failed to set foreground window. ddraw1.c:8060: Test failed: Got unexpected hr 0. ddraw1.c:8062: Test failed: Got unexpected hr 0. ddraw1.c:8064: Test failed: Got unexpected hr 0. ddraw1.c:8066: Test failed: Got unexpected hr 0. ddraw1.c:8076: Test failed: Got unexpected hr 0. ddraw1.c:8078: Test failed: Got unexpected hr 0. ddraw1.c:8080: Test failed: Got unexpected hr 0. ddraw1.c:8084: Test failed: Failed to set foreground window. ddraw1.c:8086: Test failed: Got unexpected hr 0. ddraw1.c:8088: Test failed: Got unexpected hr 0. ddraw1.c:8094: Test failed: Got unexpected hr 0. ddraw1.c:8142: Test failed: Failed to set foreground window. ddraw1.c:8154: Test failed: Failed to set foreground window. === w10pro64 (32 bit report) === ddraw: ddraw4.c:17316: Test failed: WM_KILLFOCUS was not received. ddraw4.c:17493: Test failed: Got unexpected hr 0x887600e1. ddraw4.c:17496: Test failed: Got unexpected hr 0x887600ff. 1e48:ddraw4: unhandled exception c0000005 at 004B515D === w10pro64 (64 bit report) === ddraw: ddraw4.c:17316: Test failed: WM_KILLFOCUS was not received. ddraw4.c:17493: Test failed: Got unexpected hr 0x887600e1. ddraw4.c:17496: Test failed: Got unexpected hr 0x887600ff. 20b0:ddraw4: unhandled exception c0000005 at 000000000049CAD6
```diff + delta = x - y; + + /* Check for overflow and related conditions */ + if (((x ^ y) & (x ^ delta)) < 0 || delta == INT_MIN) + return FALSE; + + if (abs(delta) > ulps) return FALSE;
return TRUE; ```
Would "return compare_uint(x, y, ulps);" work? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40853
Would "return compare_uint(x, y, ulps);" work?
Yeah, we want that instead I think. I used it in 69ecfdfb0bf9e5fddc3b74732f7db94e4948f24e in vkd3d, but never got around to porting that to Wine tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40856
That does sound like the right way forward, though based on my back-of-the-envelope calculations I think there are some additional tweaks that need to be made. Try comparing a tiny positive value with a tiny negative value. The simplest way to fix this is non-intuitive: ```diff - if (x < 0) + if (x >= 0) x = INT_MIN - x; - if (y < 0) + if (y >= 0) y = INT_MIN - y; ``` Adding a `compare_uint` everywhere we have a `compare_float` does not seem ideal (neither does all of the separate copies of `compare_float` already floating around). Would both of these functions be better off in some shared test header? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40863
If "tiny" here means "denormal", I don't think we particularly care. compare_float() isn't all that appropriate for comparing numbers close to zero, and denormals are just a special case of that. If we did care about denormals, we should probably just compare the signs; we essentially never want this comparison to succeed for number with opposing signs. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40864
Yeah, I was attempting continuity of negative to positive values, which as you say we don't particularly care about. Since we only care about the two sets in isolation (and the subnormal and NaN ranges act as neutral zones), there is no need for the existing reordering. `compare_float` here could be simply ```c { return compare_uint(*(unsigned int *)&f, *(unsigned int *)&g, ulps); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40874
Except that I think we do still want to treat 0.0 and -0.0 as equal or near-equal. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40879
I'm going with a solution that 1. uses `compare_uint`, 2. ensures the things that should _definitively_ return false _will_ return false (e.g. 2 vs -2), and 3. other results stay the same as before (in the 0.0/-0.0/subnormal range), even if of dubious value. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458#note_40887
participants (5)
-
Henri Verbeet (＠hverbeet) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin -
Zebediah Figura (＠zfigura)