Wednesday, June 13, 2007

AllocateHwnd is not Thread-Safe

[This article also serves as announcement of DSiWin32 1.26.]

[Update: Reported as QC #47559. Vote for it!]

You're probably asking yourself - what's that AllocateHwnd anyway? And why must it be thread-safe?

As the Google is guick to tell (BTW, Steve, thanks for the search filter!), AllocateHwnd is used to create a hidden window which you can use to receive messages in non-windowed components. Of course, you can use it outside of any component to set up simple and easy messaging subsystem anywhere in your application. If you need more communication channels, just call AllocateHwnd many times.

I won't bother you with the usage pattern - if you want to use AllocateHwnd and don't know how, use the search link above. You'll find many examples, including this one from DelphiDabbler, which Steve's searcher lists on the first place.

An example of a very popular component using AllocateHwnd internally is Delphi's TTimer.

That should answer the first question, but what about thread-safety?

Well, many programmers use AllocateHwnd in threaded code to create hidden windows where messages are processed. Many are also using TTimer inside threads without knowing the first thing about AllocateHwnd. But almost nobody knows that this is totally unsafe and may lead to rare and obscure crashes. AllocateHwnd was written with single-threaded VCL applications in mind and you can use it from a thread only if you take special precaution.

Why is AllocateHwnd dangerous

Let's see how the AllocateHwnd is implemented. Following code was copied from D2007's Classes.pas (in very old Delphis, AllocateHwnd was implemented in Forms.pas):

var
UtilWindowClass: TWndClass = (
style: 0;
lpfnWndProc: @DefWindowProc;
cbClsExtra: 0;
cbWndExtra: 0;
hInstance: 0;
hIcon: 0;
hCursor: 0;
hbrBackground: 0;
lpszMenuName: nil;
lpszClassName: 'TPUtilWindow');

function AllocateHWnd(Method: TWndMethod): HWND;
var
TempClass: TWndClass;
ClassRegistered: Boolean;
begin
UtilWindowClass.hInstance := HInstance;
{$IFDEF PIC}
UtilWindowClass.lpfnWndProc := @DefWindowProc;
{$ENDIF}
ClassRegistered := GetClassInfo(HInstance, UtilWindowClass.lpszClassName,
TempClass);
if not ClassRegistered or (TempClass.lpfnWndProc <> @DefWindowProc) then
begin
if ClassRegistered then
Windows.UnregisterClass(UtilWindowClass.lpszClassName, HInstance);
Windows.RegisterClass(UtilWindowClass);
end;
Result := CreateWindowEx(WS_EX_TOOLWINDOW, UtilWindowClass.lpszClassName,
'', WS_POPUP {+ 0}, 0, 0, 0, 0, 0, 0, HInstance, nil);
if Assigned(Method) then
SetWindowLong(Result, GWL_WNDPROC, Longint(MakeObjectInstance(Method)));
end;

Basically, the code registers window class if necessary, creates a new window of that class, and sets window procedur for that window to MakeObjectInstance(Method). Nothing special, except this last step. Can you tell why it is necessary at all?


The reason lies in the discrepancy between Delphi's object model and Win32 API, which is not object oriented. The TWndMethod parameter passed to the AllocateHwnd is not just an address of code, but contains also the address of the object this method belongs to.


On the other hand, Win32 API wants to call a simple method anytime it has to deliver a message to a window.


MakeObjectInstance bridges this gap. It manages a linked list of methods together with a dynamically generated code preamble (address of which is returned from the MakeObjectInstance function). When Windows calls this code preamble, it makes sure that correct method is called on the correct object.


MakeObjectInstance is complicated, but it works. That is, until you call it from two threads at the same time. You see, MakeObjectInstance does nothing to lock its internal list while it is being manipulated. If you do this from two threads running on two CPUs, or even if you have only one CPU and context switch occurs at a bad time, internal instance list can get corrupted. Later, this may lead to crashes, bad program behaviour, you name it. And you'll never find the true culprit.


Admittedly, there is only a small window - few instructions - which are problematic. In most applications such problems will never occur. But if you're running 24/7 server which calls AllocateHwnd/DeallocateHwnd constantly from multiple threads, you can be sure that sooner or later it will crash.


Solution


There are two possible solutions to the problem - one is to wrap all AllocateHwnd and DeallocateHwnd in some sort of critical section, spinlock or mutex that will allow only one instance to be called at the same time and other is to write a better and thread-safe AllocateHwnd. First solution is somewhat clumsy to implement in production code while the second can be hard to write.


Actually, I search the net wide and deep and found only two alternative AllocateHwnd implementations (references below). I'm sure there are more. I just couldn't find them. None of them was really suitable for my needs so I created a third one using ideas from both of them. My version — DSiAllocateHwnd, DSiDeallocateHwnd and TDSiTimer — has been published as a part of the DSiWin32 library.

This is the current version of my AllocateHwnd alternative:
const 
GWL_METHODCODE = SizeOf(pointer) * 0;
GWL_METHODDATA = SizeOf(pointer) * 1;

CDSiHiddenWindowName = 'DSiUtilWindow';

var
GDSiWndHandlerCritSect: TRTLCriticalSection;
GDSiWndHandlerCount: integer;

function DSiClassWndProc(Window: HWND; Message, WParam, LParam: longint): longint; stdcall;
var
instanceWndProc: TMethod;
msg : TMessage;
begin
instanceWndProc.Code := Pointer(GetWindowLong(Window, GWL_METHODCODE));
instanceWndProc.Data := Pointer(GetWindowLong(Window, GWL_METHODDATA));
if Assigned(TWndMethod(instanceWndProc)) then
begin
msg.msg := Message;
msg.wParam := WParam;
msg.lParam := LParam;
TWndMethod(instanceWndProc)(msg);
Result := msg.Result
end
else
Result := DefWindowProc(Window, Message, WParam,LParam);
end; { DSiClassWndProc }

function DSiAllocateHWnd(wndProcMethod: TWndMethod): HWND;
var
alreadyRegistered: boolean;
tempClass : TWndClass;
utilWindowClass : TWndClass;
begin
Result := 0;
FillChar(utilWindowClass, SizeOf(utilWindowClass), 0);
EnterCriticalSection(GDSiWndHandlerCritSect);
try
alreadyRegistered := GetClassInfo(HInstance, CDSiHiddenWindowName, tempClass);
if (not alreadyRegistered) or (tempClass.lpfnWndProc <> @DSiClassWndProc) then begin
if alreadyRegistered then
Windows.UnregisterClass(CDSiHiddenWindowName, HInstance);
utilWindowClass.lpszClassName := CDSiHiddenWindowName;
utilWindowClass.hInstance := HInstance;
utilWindowClass.lpfnWndProc := @DSiClassWndProc;
utilWindowClass.cbWndExtra := SizeOf(TMethod);
if Windows.RegisterClass(utilWindowClass) = 0 then
raise Exception.CreateFmt('Unable to register DSiWin32 hidden window class. %s',
[SysErrorMessage(GetLastError)]);
end;
Result := CreateWindowEx(WS_EX_TOOLWINDOW, CDSiHiddenWindowName, '', WS_POPUP,
0, 0, 0, 0, 0, 0, HInstance, nil);
if Result = 0 then
raise Exception.CreateFmt('Unable to create DSiWin32 hidden window. %s',
[SysErrorMessage(GetLastError)]);
SetWindowLong(Result, GWL_METHODDATA, Longint(TMethod(wndProcMethod).Data));
SetWindowLong(Result, GWL_METHODCODE, Longint(TMethod(wndProcMethod).Code));
Inc(GDSiWndHandlerCount);
finally LeaveCriticalSection(GDSiWndHandlerCritSect); end;
end; { DSiAllocateHWnd }

procedure DSiDeallocateHWnd(wnd: HWND);
begin
DestroyWindow(wnd);
EnterCriticalSection(GDSiWndHandlerCritSect);
try
Dec(GDSiWndHandlerCount);
if GDSiWndHandlerCount <= 0 then
Windows.UnregisterClass(CDSiHiddenWindowName, HInstance);
finally LeaveCriticalSection(GDSiWndHandlerCritSect); end;
end; { DSiDeallocateHWnd }

There are many differences between this code and Delphi version.



  • My code uses custom DefWindowProc method - DSiClassWndProc.
  • It reserves four extra bytes in each window of the DSiUtilWindow class (utilWindowClass.cbWndExtra setting).
  • It writes both parts of TMethod (code and data) directly into those four bytes of the hidden window's user data.
  • DSiClassWndProc retrieves those four bytes, reconstructs TMethod and calls it directly.
  • When all hidden windows are closed, window class gets unregistered (in DSiDeallocateHwnd).

I admit that this approach to message dispatching is slower than the Delphi's version, but usually that is not a problem - custom windows are usually created to process some small subset of messages only.


Acknowledgments


The AllocateHwnd problem is not something I have found by myself. It has been documented for years, but is not well known.


I'd like to thank to:


  • Arno Garrels on the ICS mailing list, who described the problem to me.
  • Francois Piette for providing ICS source code with custom AllocateHwnd solution. My approach is partially based on the Francois' code.
  • Alexander Grischenko, who wrote this solution from which I stole the idea of storing TMethod directly in window's extra data.


12 comments:

  1. Technical nit: cbWndExtra would be 8 (SizeOf(TMethod)), not the 4 that you mention in your final set of bullets. (The code pointer is 4 bytes, and the data pointer is another 4 bytes.)

    ReplyDelete
  2. None of this helps if you call code that calls the stock APIs, like TTimer or code that uses TTimer.

    It might just be time to nag CodeGear into fixing it properly! Then it would be fixed once and for all.

    Easy enough - only use the critical section when the multithreaded flag is set to true to prevent unneeded trips into the OS for single threaded Apps.

    ReplyDelete
  3. Glad you're using it! Thanks for the link. Great post btw. Can you get this into QC?

    ReplyDelete
  4. @joe: Of course. Stupid mistake. Sometimes I don't know how big my pointers are. :(

    @xepol: Very true.

    @steve: Indeed, I should add this to the QC. Will do so today.

    ReplyDelete
  5. About deallocate see this: http://delphi.about.com/od/windowsshellapi/l/aa093003c.htm

    procedure *****.DeallocateHWnd(Wnd: HWND);
    var
    Instance: Pointer;
    begin
    Instance := Pointer(GetWindowLong(Wnd, GWL_WNDPROC));
    if Instance <> @DefWindowProc then
    // make sure we restore the old, original windows procedure before leaving
    SetWindowLong(Wnd, GWL_WNDPROC, Longint(@DefWindowProc));
    FreeObjectInstance(Instance);
    DestroyWindow(Wnd);
    end;

    ReplyDelete
  6. @lucefer: I have no idea what information you're trying to convey.

    ReplyDelete
  7. Very useful and helpful. I do have some comments. First of all I think that you ought to set msg.Result to 0 before calling the window proc. If you study Classes.StdWndProc you'll see that done as:

    XOR EAX,EAX
    PUSH EAX

    I also believe that there is a thread-safety issue with the Delphi code which registers the window class in AllocateHWnd. Surely this is not thread-safe.

    I took your ideas and wrapped them up in a unit which uses WriteProcessMemory to redirect calls to the 4 miscreant routines to ones which are thread safe. Include this in your project (first unit after any memory manager units) and it sorts all the problems out.

    I'd happily share this but your blog software won't accept more than 4096 characters!

    ReplyDelete
  8. @David: Sorry it took so long for your comment to be published. The Blogger platform had some problems ...

    Yes, I probably ought to set msg.Result. I'll fix this.

    I'd be happy to publish your unit on my blog. My e-mail address is "primoz at gabrijelcic dot org".

    ReplyDelete
  9. >@lucefer: I have no idea what information you're trying to convey.
    read the source: http://delphi.about.com/od/windowsshellapi/l/aa093003c.htm
    You have the potential bug in DSiDeallocateHWnd
    >>
    At first glance everything looks fine. The window is destroyed and the memory occupied by our window procedure is freed. But... after we free up the memory occupied by our window procedure a few messages are still routed to the hidden window (yes, there are still messages routed to that window, including a bunch of WM_DESTROY and its relatives). So Windows will try to reference an unallocated memory space when trying to execute our window procedure and thus an exception will pop up. The solution we have found is to slightly alter the DeallocateHWnd code to change back the window procedure to the default one provided by Windows before we free up the memory for the code.
    >>
    isn't it?

    ReplyDelete
  10. Hi Gabr,

    Great post! What I'm failing to understand is: If the problem resides in MakeObjectInstance, why don't patch it (MakeObjectInstance) replacing it by another one, protected by a critical section? Using your solution, AllocateHWnd gets protected, but what about other places in VCL where MakeObjectInstance is called directly (like TWinControl.Create)?

    ReplyDelete
    Replies
    1. That is, of course, also possible.

      Delete