FunkyFr3sh / cnc-ddraw

GDI, OpenGL and Direct3D 9 re-implementation of the DirectDraw API for classic 2D games for better compatibility with Windows 2000, XP, Vista, 7, 8, 10, 11, Wine (Linux/macOS/Android) and Virtual Machines
https://discord.gg/afWXJNDDF5
MIT License
2.13k stars 143 forks source link

Potential issue with WndProc #254

Closed elishacloud closed 9 months ago

elishacloud commented 9 months ago

I was looking at how you handle WndProc and it appears like you are just using SetWindowLongA() for this.

However, I recently ran into an issue where a strange address was returned when using that function and after researching it I found out that if the window was created in Unicode (which some DirectDraw games do, like Unreal Gold) then it can return the "handle" rather than the actual pointer. Instead, you should first check the Unicode status (IsWindowUnicode()) of the window and then call either SetWindowLongA() or SetWindowLongW() depending on the return.

See here for more details: https://masm32.com/board/index.php?topic=1762.msg17898#msg17898

Example code:

LONG GetWndProc(HWND hWnd)
{
    return (IsWindowUnicode(hWnd) ?
        GetWindowLongW(hWnd, GWL_WNDPROC) :
        GetWindowLongA(hWnd, GWL_WNDPROC));
}

LONG SetWndProc(HWND hWnd, LONG ProcAddress)
{
    return (IsWindowUnicode(hWnd) ?
        SetWindowLongW(hWnd, GWL_WNDPROC, ProcAddress) :
        SetWindowLongA(hWnd, GWL_WNDPROC, ProcAddress));
}
FunkyFr3sh commented 9 months ago

SetWindowLong does not always return a pointer, sometimes it returns an ID. You should never treat as pointer or you may run into issues. Always use CallWindowProc instead.

See here: https://github.com/FunkyFr3sh/cnc-ddraw/blob/master/src/wndproc.c#L899C12-L899C27

I don't think I'll need to worry about unicode in cnc-ddraw since I'm only dealing with old DirectDraw games. but I assume it should work fine if called from SetWindowLongA/CallWindowProcA

FunkyFr3sh commented 9 months ago

Oh, I forgot to mention. This can happen with any game. Direct3D/OpenGL also hook the wndproc. So if you try to hook after their hooks it could also return such an ID/Handle, even though the window was created CreateWindowExA and not with CreateWindowExW.

I also had this happening in other cases, for example sandboxing software that hooked the wndproc would also cause SetWindowLongA to return a ID/Handle.

So it seems like that whatever got the toplevel hook currently decides if a pointer or an ID/Handle will be returned. If the last hook was done with SetWindowLongW and you try to hook with SetWindowLongA afterwards you'll get a ID/Handle.

elishacloud commented 9 months ago

I see. Thanks for the details!

FunkyFr3sh commented 9 months ago

Np!

Oh, there's one more thing you need to take care of...

Don't ever let the games hook the wndproc directly, you need to hook SetWindowLongA and handle it on your own. Old games don't use CallWindowProc because that apparently wasn't a thing back then? They would crash if a ID/Handle is returned.

Here's the cnc-ddraw code to deal with exactly that problem: https://github.com/FunkyFr3sh/cnc-ddraw/blob/master/src/winapi_hooks.c#L464

Edit: I think GTA1 was a good test game for such cases

elishacloud commented 9 months ago

Ok, thanks for the tip!

elishacloud commented 9 months ago

Added code here: https://github.com/elishacloud/dxwrapper/commit/bd7955becb8618cbbf81e9c7d10dd258cbbfa5b8

FunkyFr3sh commented 9 months ago

nice 👍