New subject: [Bug 35626] Patrician III: divide by zero exception scrolling the city view (side effect in user32.SubtractRect())

19 Feb 2014


      http://bugs.winehq.org/show_bug.cgi?id=35626
Bug ID: 35626
           Summary: Patrician III: divide by zero exception scrolling the
                    city view (side effect in user32.SubtractRect())
           Product: Wine
           Version: unspecified
          Hardware: x86
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: user32
          Assignee: wine-bugs@winehq.org
          Reporter: jcantero@gmail.com
Created attachment 47575
  --> http://bugs.winehq.org/attachment.cgi?id=47575
Backtrace of the exception, wine trace log and an example .c of the bug
The game 'Patrician III: Rise of the Hanse' crashes by a 'divide by zero'
exception in certain conditions (later explained). I've debugged and found the
root of the bug: it's a side effect in the wine implementation of
user32.SubtractRect() function. In my attachment there's a C program that
simulates the error (and a fixed version of the function to avoid it).
I'm using wine 1.6.2, but this bug applies to all versions of wine to this day.
In git-master it's here:
https://github.com/mirrors/wine/blob/master/dlls/user32/uitools.c#L1424 . The
bug was already reported serveral times, but due to lack of new information it
was CLOSED ABANDONED (for example bugs #11768 and #16535). The bug is also
mentioned as well-known in the comments of the AppDB entry of Patrician III
(the vast majority of users avoid the bug disabling the feature that triggers
it). Other applications can be also affected by this bug (see later).
But first is first: the symptoms. The users of Patrician 3 suffer this bug when
they try to scroll the city view and that city has a layer of weather effects
(rain, fog, snow, ...). If the user changes the view of the city using the
minimap, it works but only if the original rectangle of the view and the 
rectangle of the new position don't overlap. If they overlap in any way, or
when the user tries to scroll the view (then it always overlap), the situation
is detected by a call to user32.IntersectRect() function that returns TRUE, and
the program branchs through 2 calls to SubtractRect() (I suppose to find the
area that it's not in common between the two views). These calls fail when the
actual version of the wine user32 library is used: they return an empty
rectangle (with values {0, 0, 0, 0}) instead the expected one. The code of
Patrician III is not prepared to handle the unexpected result, and when it
tries to use such values in some calculations including a division, it crashes
due a 'divide by zero' exception. That's what I found debugging the executable
with winedbg. The attachment includes a log of (part of) the calls (it's a long
list but the only relevant info is just before the exception near the end of
the file).
The cause of this bug lies in... let's say an unorthodox use of the
SubtractRect() function. In the common case, the wine version works flawlessly.
But that particular code of Patrician III calls SubtractRect() using the same
pointer for the out value (dst) and one of the in values (src2). One of the
first things that the wine implementation of SubtractRect() does is copying the
scr1 value to dst. But with dst==src2, the assign also changes src2 as a side
effect, provoking the bug. That behaviour is what is needed to fix. This bug
can also affect other programs that uses the function in the same way
(fortunately the bug is not present in Windows version, so there can't be any
program that rely on it).
In my attachment the SubtractRect.c that can be compiled (alone) to demo the
bug behaviour. It calls SubtractRect() -a straight version from source code-
with 3 different parameters (working as intended) and with the 1st and 3rd same
parameter (bug). The .c has 2 #defines in the beginning, one to show more debug
info and another to replace SubtractRect() with a fixed SubtractRect2() that I
tested. The fix is not probably the best of the solutions, it only delays the
copy *dest = *src1; past the uses of src1 and src2. But it works! (a better
solution probably wouldn't use dst as a lvalue at all, only as a rvalue).
I've applied the patch to wine 1.6.2 and now Patrician III is working with the
weather effects to high :)
-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.

[Bug 35626] New: Patrician III: divide by zero exception scrolling the city view (side effect in user32.SubtractRect())