Does TMonitor.GetBoundsRect have an access violation bug in Delphi 2007 triggered by VNC?

Question

This is a crash report I get from customers running one of our big products built in Delphi 2007, that I am unable to reproduce and which doesn't even have any non-VCL code directly involved via the call stack.

The VCL Source code for TMonitor in Delphi 2007 is very simple (Forms.pas)

TMonitor.GetBoundsRect calls win32 API GetMonitorInfo after initializing a struct's cbSize so that the Windows API knows what size to return.

I have been over and over this code and I can't see how this level could crash, unless the Self pointer is invalid, although I suspect it might actually be the THintWindow level malfunctioning, and it is only at the TMonitor.GetBoundsRect that this malfunction becomes a visible crash because somehow it has gotten a pointer to an invalid TMonitor object and invoked a method on it.

function TMonitor.GetBoundsRect: TRect;
var
  MonInfo: TMonitorInfo;
begin
  MonInfo.cbSize := SizeOf(MonInfo);
  GetMonitorInfo(FHandle, @MonInfo);  // <----- CRASH.
  Result := MonInfo.rcMonitor;
end;

Crash call stack via madExcept is only reproduceable on client systems:

main thread ($70):
004f5004 +010 myapp.exe Forms    6361  +2 TMonitor.GetBoundsRect
004f4fe2 +00a myapp.exe Forms    6352  +1 TMonitor.GetWidth
004e022f +06b myapp.exe Controls 9929  +7 THintWindow.ActivateHint
004e0457 +017 myapp.exe Controls 9970  +1 THintWindow.ActivateHintData
004f95fa +31a myapp.exe Forms    8923 +56 TApplication.ActivateHint
004f8fc1 +02d myapp.exe Forms    8728  +8 TApplication.HintTimerExpired
004f61e6 +022 myapp.exe Forms    7083  +3 HintTimerProc
7e4196c2 +00a USER32.dll                    DispatchMessageA
004f7f00 +0fc myapp.exe Forms    8105 +23 TApplication.ProcessMessage
004f7f3a +00a myapp.exe Forms    8124  +1 TApplication.HandleMessage
004f822f +0b3 myapp.exe Forms    8223 +20 TApplication.Run
0155cf77 +2f3 myapp.exe myapp    368  +41 initialization

All the code above in the entire call stack is VCL source code, none of it is mine.

Aside from disabling all Hints in the entire application, is there anything I could do to further troubleshoot this or work around it?

Update: Registers and disasm, and exception address:

exception class   : EAccessViolation
exception message : Access violation at address 004F5004 in module 'hirepnt.exe'. Read of address 00000004.

cpu registers:
eax = 00000000
ebx = 0012fccc
ecx = 7e42ac2c
edx = 0012fccc
esi = 07b694a8
edi = 0012fd14
eip = 004f5004
esp = 0012fc90
ebp = 0012fd1c

stack dump:
0012fc90  94 fc 12 00 28 00 00 00 - 2c ac 42 7e 70 b3 6f 02  ....(...,.B~p.o.
0012fca0  fc fc 12 00 01 00 00 00 - 70 b3 6f 02 cc 53 4f 00  ........p.o..SO.
0012fcb0  01 00 00 00 01 00 00 00 - 3c 56 4f 00 14 fd 12 00  ........<VO.....
0012fcc0  a8 94 b6 07 00 00 00 00 - e7 4f 4f 00 00 00 00 00  .........OO.....
0012fcd0  4b 64 4f 00 14 fd 12 00 - a8 94 b6 07 00 00 00 00  KdO.............
0012fce0  34 02 4e 00 4c fd 12 00 - c4 5c 40 00 1c fd 12 00  4.N.L....\@.....
0012fcf0  40 fd 12 00 97 fd 12 00 - 0c 23 4d 00 e0 01 00 00  @........#M.....
0012fd00  6d 01 00 00 e0 01 00 00 - 6d 01 00 00 41 02 00 00  m.......m...A...
0012fd10  80 01 00 00 34 fd 12 00 - 10 cc 6e 02 40 fd 12 00  ....4.....n.@...
0012fd20  5d 04 4e 00 d8 fd 12 00 - 0c 23 4d 00 f0 fd 12 00  ].N......#M.....
0012fd30  e0 01 00 00 6d 01 00 00 - 41 02 00 00 7c 01 00 00  ....m...A...|...
0012fd40  f4 fd 12 00 00 96 4f 00 - 00 00 00 00 0c fe 12 00  ......O.........
0012fd50  c4 5c 40 00 f4 fd 12 00 - 8c fe 12 00 c4 61 4f 00  .\@..........aO.
0012fd60  40 41 6f 02 00 00 00 00 - 00 00 00 00 28 05 00 00  @Ao.........(...
0012fd70  15 02 00 00 00 00 00 00 - 00 00 00 00 d9 04 00 00  ................
0012fd80  ac 01 00 00 84 00 00 e0 - 01 00 00 6d 01 00 00 41  ...........m...A
0012fd90  02 00 00 7c 01 00 00 01 - 4f 00 00 00 69 00 00 00  ...|....O...i...
0012fda0  51 00 00 00 6b 00 00 00 - e0 01 00 00 61 01 00 00  Q...k.......a...
0012fdb0  f0 84 22 06 0c 23 4d 00 - e0 01 00 00 6d 01 00 00  .."..#M.....m...
0012fdc0  56 05 00 00 18 00 00 ff - fe ff ff ff fe ff ff ff  V...............

disassembling:
[...]
004f4ff6        push    edi
004f4ff7        add     esp, -$28
004f4ffa        mov     ebx, edx
004f4ffc 6360   mov     dword ptr [esp], $28
004f5003 6361   push    esp
004f5004      > mov     eax, [eax+4]
004f5007        push    eax
004f5008        mov     eax, [$1644560]
004f500d        mov     eax, [eax]
004f500f        call    eax
004f5011 6362   mov     edi, ebx
[...]

I would personally initialize the `TMonitorInfo` structure e.g. by `ZeroMemory`. — TLama, Mar 06 '13 at 16:20
I wonder if it's this: http://qc.embarcadero.com/wc/qcmain.aspx?d=53932 — Warren P, Mar 06 '13 at 16:21
@WarrenP Can we see the disasm and register reports from the ME bug report? — David Heffernan, Mar 06 '13 at 16:31
@David, I was thinking the same some time ago. Look at [`this post`](http://stackoverflow.com/a/15105323/960757) and what happened on Windows XP. The uninitialized members should have been ignored, I'd say, but instead it ended up with access violation at the Windows API function call. — TLama, Mar 06 '13 at 16:31
@TLama I cannot see how that relates to this post. All you need to do is initialize `cbSize`. — David Heffernan, Mar 06 '13 at 16:35
@David, that was just reaction to your comment about paranoia. In that post I've expected the same, I just set the necessary members and the size of a structure one. The function call succeeded on my Windows 7, but failed with access violation on Windows XP, so what is then paranoia ? — TLama, Mar 06 '13 at 16:38
How do you know that EAX=Self? Is `mov eax, [eax+4]` idiomatically a VTABLE lookup+dereference? — Warren P, Mar 06 '13 at 16:40
Because the register calling convention uses `EAX` to pass the first parameter. The first parameter to an instance method is `Self`. — David Heffernan, Mar 06 '13 at 16:43
Ah. I should already have known that by now, a Delphi programmer these many years. :-) — Warren P, Mar 06 '13 at 16:46
It's the most trival implementation of a `For I` loop from 0 to `MonitorCount-1`, checking `Monitors[I].Handle = Handle` then for each, and returning the object that matches. Note that it has no check for the incoming parameter being 0, which apparently it can be on systems running UltraVNC. — Warren P, Mar 06 '13 at 16:57
Yeah, take a look at the implementation in modern Delphi. I suspect therein lies the problem. — David Heffernan, Mar 06 '13 at 16:58
Bjoern Ahrens' comment in the report you linked explains why the instance is nil, I think... — Sertac Akyuz, Mar 06 '13 at 17:01
@SertacAkyuz The fix is pretty lame also (in both the report and the modern VCL). It assumes that monitor handles are not re-used. Seems like a pretty poor wrapping of the underlying Win32 API. I personally would avoid anything that use TMonitor and just call the underlying Win32 API. It's not exactly hard to do that. — David Heffernan, Mar 06 '13 at 17:04
@David - It looks like the hint system is using that, not particularly the developer. — Sertac Akyuz, Mar 06 '13 at 17:05
@SertacAkyuz Yeah, so avoiding TMonitor functions isn't going to help here. — David Heffernan, Mar 06 '13 at 17:06
I could monkey-patch the VCL (via a code hook) or recompile the VCL. :-) Given how gross those options are, I think the QC workaround is the best path. I will #ifdef it for this version so it doesn't pollute the codebase when I move it up. — Warren P, Mar 06 '13 at 17:39

score 4 · Accepted Answer · edited Mar 07 '13 at 19:10

It's most likely qc #53932 (also #25466 #57953), and patching Forms.pas does fix it. In my case, using D7, it only got triggered by using remote access, and the problem/fix is the same as for D2007.

Specifically, the patch is to change TScreen.FindMonitor. This is the D7 change, not sure if it needs any edits for D2007.

function TScreen.FindMonitor(Handle: HMONITOR): TMonitor;
var
  I: Integer;
begin
  Result := nil;
  for I := 0 to MonitorCount - 1 do
    if Monitors[I].Handle = Handle then
    begin
      Result := Monitors[I];
//      break;
      Exit;
    end;
  //if we get here, the Monitors array has changed, so we need to clear and reinitialize it
  for i := 0 to MonitorCount-1 do
    TMonitor(Monitors[i]).Free;
  fMonitors.Clear;
  EnumDisplayMonitors(0, nil, @EnumMonitorsProc, LongInt(FMonitors));
  for I := 0 to MonitorCount - 1 do
    if Monitors[I].Handle = Handle then
    begin
      Result := Monitors[I];
      Exit;
    end;
end;

My customer got back to me and they WERE using RealVNC (not ultraVNC, but close enough) when this happened. — Warren P, Mar 07 '13 at 17:37
Just to mention how I compiled the Forms.pas into my project: 1. Copy the Forms.pas into the Project directory and add it as a file 2. Change FindMonitor function 3. Compile the project WITHOUT Runtime Packages — Lumo, Jul 22 '20 at 12:13

score 1 · Answer 2 · answered Nov 22 '22 at 21:21

The problem is quite simple to reproduce as follows:

Create an app with a main form with a panel and set a hint for the panel.
Run the application.
Hover your mouse over the panel to see the hint.
Switch to another user on your computer without closing the app.
Switch back to the original user and hover your mouse over the panel
You get the access violation:

Access violation at address 0048A74 in module 'App.exe'.Read of address 00000004.

My workaround consists in processing the WM_WTSSESSION_CHANGE message.

procedure TITMainForm.WMWTSSESSION_CHANGE(
                          var msg : TMessage);
begin
  //  Destroy and recreate screen to trigger call to GetMonitors.

  FORMS.screen.Free;
  FORMS.screen := TScreen.Create(NIL);
end;

Alternatively to switching users you can disconnect or attach an additional monitor. — dummzeuch, Nov 23 '22 at 08:50
Indeed! But it is much simpler to switch user if you already have other user accounts created of course. — Emmanuel Ichbiah, Nov 23 '22 at 11:19

Does TMonitor.GetBoundsRect have an access violation bug in Delphi 2007 triggered by VNC?

2 Answers2

Linked