[PATCH v4 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. -- v4: ddraw/tests: Use compare_uint() in compare_float() instead of abs(). https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
From: Jeff Smith <whydoubt(a)gmail.com> The result of abs(INT_MIN) is INT_MIN, which breaks the ulps comparison. --- dlls/ddraw/tests/ddraw1.c | 19 ++++++++----------- dlls/ddraw/tests/ddraw2.c | 19 ++++++++----------- dlls/ddraw/tests/ddraw4.c | 19 ++++++++----------- dlls/ddraw/tests/ddraw7.c | 19 ++++++++----------- 4 files changed, 32 insertions(+), 44 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index cde80c58e99..6c57eb4470c 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -50,6 +50,13 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; @@ -60,10 +67,7 @@ static BOOL compare_float(float f, float g, unsigned int ulps) if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) - return FALSE; - - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -74,13 +78,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw2.c b/dlls/ddraw/tests/ddraw2.c index 86056014f96..98737f36541 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -51,6 +51,13 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; @@ -61,10 +68,7 @@ static BOOL compare_float(float f, float g, unsigned int ulps) if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) - return FALSE; - - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -75,13 +79,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 4458cea5145..bc465332694 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -57,6 +57,13 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; @@ -67,10 +74,7 @@ static BOOL compare_float(float f, float g, unsigned int ulps) if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) - return FALSE; - - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, float w, unsigned int ulps) @@ -81,13 +85,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index bbb78169aea..917720d9802 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -68,6 +68,13 @@ struct create_window_thread_param HANDLE thread; }; +static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) +{ + unsigned int diff = x > y ? x - y : y - x; + + return diff <= max_diff; +} + static BOOL compare_float(float f, float g, unsigned int ulps) { int x = *(int *)&f; @@ -78,10 +85,7 @@ static BOOL compare_float(float f, float g, unsigned int ulps) if (y < 0) y = INT_MIN - y; - if (abs(x - y) > ulps) - return FALSE; - - return TRUE; + return compare_uint(x, y, ulps); } static BOOL compare_vec3(struct vec3 *vec, float x, float y, float z, unsigned int ulps) @@ -99,13 +103,6 @@ static BOOL compare_vec4(const struct vec4 *vec, float x, float y, float z, floa && compare_float(vec->w, w, ulps); } -static BOOL compare_uint(unsigned int x, unsigned int y, unsigned int max_diff) -{ - unsigned int diff = x > y ? x - y : y - x; - - return diff <= max_diff; -} - static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) { return compare_uint(c1 & 0xff, c2 & 0xff, max_diff) -- 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=135528 Your paranoid android. === w10pro64 (64 bit report) === ddraw: ddraw4.c:17306: Test failed: WM_KILLFOCUS was not received. ddraw4.c:17483: Test failed: Got unexpected hr 0x887600e1. ddraw4.c:17486: Test failed: Got unexpected hr 0x887600ff. 2058:ddraw4: unhandled exception c0000005 at 000000000049C936 === w10pro64 (32 bit report) === ddraw: ddraw7.c:10266: Test failed: Failed to set foreground window. ddraw7.c:10268: Test failed: Got unexpected hr 0. ddraw7.c:10270: Test failed: Got unexpected hr 0. ddraw7.c:10272: Test failed: Got unexpected hr 0. ddraw7.c:10274: Test failed: Got unexpected hr 0. ddraw7.c:10276: Test failed: Got unexpected hr 0. ddraw7.c:10286: Test failed: Got unexpected hr 0. ddraw7.c:10288: Test failed: Got unexpected hr 0. ddraw7.c:10290: Test failed: Got unexpected hr 0. ddraw7.c:10294: Test failed: Failed to set foreground window. ddraw7.c:10298: Test failed: Got unexpected hr 0. ddraw7.c:10300: Test failed: Got unexpected hr 0. ddraw7.c:10306: Test failed: Got unexpected hr 0. ddraw7.c:10330: Test failed: Got unexpected hr 0. ddraw7.c:10332: Test failed: Got unexpected hr 0x887600e1. ddraw7.c:10338: Test failed: Got unexpected hr 0. ddraw7.c:10368: Test failed: Failed to set foreground window. ddraw7.c:10382: Test failed: Failed to set foreground window. ddraw7.c:17254: Test failed: WM_KILLFOCUS was not received. ddraw7.c:17416: Test failed: Got unexpected hr 0x887600e1. ddraw7.c:17419: Test failed: Got unexpected hr 0x887600ff. 2064:ddraw7: unhandled exception c0000005 at 0051C7CE
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3458
participants (4)
-
Jeff Smith -
Jeffrey Smith (＠whydoubt)
-
Marvin -
Zebediah Figura (＠zfigura)