Memory corruption after GetMem in EnumWindows-CodePudding

While enumerating windows by their process ID and also window title I have two implementations of the callback function.

The first uses a fixed size char array as buffer and works fine (as far I can see).

With the other using PChar buffer and allocating it using GetMem I get exceptions on different internal GetMem after I leave my GetWindowHandleByPID function.

Where is my error in the second one?

TEnumWindowsInfo = record
  ProcessID: DWORD;
  HWND: THandle;
  TitleUp: string;
end;

function GetWindowHandleByPID(const hPID: THandle): THandle;
var
  EI: TEnumWindowsInfo;
begin
  EI.ProcessID := hPID;
  EI.HWND := 0;
  EI.TitleUp := 'BLA';
  EnumWindows(@OnEnumWindowsProc, Integer(@EI));
  Result := EI.HWND;
end;


function OnEnumWindowsProc(Wnd: DWORD; var EI: TEnumWindowsInfo): BOOL; stdcall;
var
  pidMatch, titleMatch: BOOL;
  buf: array [0..255] of Char;
  titleAsUpper: string;
begin
  // check PID: GetWindowThreadProcessID
  pidMatch := true;

  // check window title
  GetWindowText(Wnd, buf, 255);
  titleAsUpper := AnsiUpperCase(buf);
  titleMatch := EI.TitleUp = titleAsUpper;

  Result := not (pidMatch and titleMatch);  // continue if not found

  if not Result then
  begin
    EI.HWND := WND;
  end;
end;

function OnEnumWindowsProc_MEM_CORRUPT(Wnd: DWORD; var EI: TEnumWindowsInfo): BOOL; stdcall;
var
  pidMatch, titleMatch: BOOL;
  buffer: PChar;
  len: Integer;
  titleAsUpper: string;
begin
  // check PID: GetWindowThreadProcessID
  pidMatch := true;

  // check window title
  len := GetWindowTextLength(Wnd)   1; //  1 -> null-terminated
  if len > 1 then
  begin
    try
      GetMem(buffer, len);
      GetWindowText(Wnd, buffer, len);
      titleAsUpper := AnsiUpperCase(buffer);
    finally
      FreeMem(buffer);
    end;
    titleMatch := EI.TitleUp = titleAsUpper;
  end;

  Result := not (pidMatch and titleMatch);  // continue if not found

  if not Result then
  begin
    EI.HWND := WND;
  end;
end;

CodePudding user response：

I assume you are using Delphi2009 or later. In that case GetWindowText is Unicode and the Getmem should really be

GetMem(buffer, len*sizeof(char));

In the other case I guess you were lucky since you had a large enough buffer reserved on the stack.

CodePudding user response：

In OnEnumWindowsProc(), the buffer you are passing to GetWindowText() is allocated sufficiently for the size you are claiming to GetWindowText(). You are allocating 256 Chars but claiming 255 Chars, which means GetWindowText() can retrieve only 254 Chars, plus the null terminator. So, there is no chance for corrupting memory.

However, in OnEnumWindowsProc_MEM_CORRUPT(), you are not allocating enough memory for the buffer. GetMem() operates on bytes, not characters. In Delphi 2009 and later, Sizeof(Char) is 2 bytes, not 1 byte as your code is assuming. So, you are allocating only 1/2 of the bytes needed for the buffer, and then GetWindowText() ends up overflowing the buffer, corrupting memory. So, you need to multiple the allocated size by SizeOf(Char):

GetMem(buffer, len * SizeOf(Char));

Personally, I would not use GetMem() for this kind of code, I would use a dynamic Char array, or even a String instead, eg:

function OnEnumWindowsProc_MEM_CORRUPT(Wnd: DWORD; var EI: TEnumWindowsInfo): BOOL; stdcall;
var
  ...
  buffer: array of Char; // or String
  len: Integer;
  ...
begin
  ...

  len := GetWindowTextLength(Wnd);
  if len > 0 then
  begin
    // for a Char array:
    Inc(len);
    SetLength(buffer, len);
    GetWindowText(Wnd, PChar(buffer), len);

    // for a String:
    SetLength(buffer, len);
    GetWindowText(Wnd, PChar(buffer), len 1);

    ...
  end;

  ...
end;

On a side note:

When calling EnumWindows(), you should use LPARAM(...) instead of Integer(...) when type-casting the @EI address in the 2nd parameter, otherwise your code will not work correctly if compiled for a 64bit executable.

Also, you don't need to use AnsiUpperCase() followed by operator= to compare two strings case-insensitively. The RTL has functions for that purpose, such as SysUtils.SameText(), eg:

titleMatch := SameText(buffer, EI.TitleUp);