Complex problem when trying to free a StringList with objects crashes my app

Question

In a Delphi 10.4.2 win-32 VCL Application in Windows 10, I use this code to collect the handles and program paths of some windows:

function GetPathFromPID(const PID: Cardinal): string;
var
  hProcess: THandle;
  Path: array[0..MAX_PATH - 1] of Char;
begin
  hProcess := Winapi.Windows.OpenProcess(Winapi.Windows.PROCESS_QUERY_INFORMATION or Winapi.Windows.PROCESS_VM_READ, False, PID);
  if hProcess <> 0 then
  try
    if Winapi.PsAPI.GetModuleFileNameEx(hProcess, 0, Path, Winapi.Windows.MAX_PATH) = 0 then
      RaiseLastOSError;
    Result := Path;
  finally
    Winapi.Windows.CloseHandle(hProcess)
  end
  else
    RaiseLastOSError;
end;

function EnumWinProc(wHandle: Winapi.Windows.HWND; aList: TStringList): Winapi.Windows.Bool; stdcall;
var
  strPath: string;
  IsAppMainWin: Boolean;
  ProcId: Cardinal;
begin
  IsAppMainWin := IsWindowVisible(wHandle)              and // Visible
    (GetWindow(wHandle, Winapi.Windows.GW_OWNER) = 0)   and // Not owned by other windows
    (GetParent(wHandle) = 0)                            and // Does not have any parent
    (GetWindowLong(wHandle, Winapi.Windows.GWL_EXSTYLE) and Winapi.Windows.WS_EX_TOOLWINDOW = 0); // Not a tool window

  if IsAppMainWin then
  begin
    GetWindowThreadProcessID(wHandle, ProcId);

    try
      strPath := GetPathFromPID(ProcId);
    except
      strPath := 'UnknownProgramPath';
    end;

    aList.AddObject(strPath, TObject(wHandle));
  end;

  Result := True;
end;

procedure ClearList(List: TStringList);
// https://stackoverflow.com/questions/9148659/how-to-free-objects-in-stringlist-in-delphi-7
var
  i: Integer;
begin
  // crash occurs here!
  for i := 0 to pred(List.Count) do
    List.Objects[i].Free;
  List.Clear;
end;

procedure TformMain.OutputAllAppWindows;
begin
  var sl := TStringList.Create;
  try
    sl.OwnsObjects := True;

    EnumWindows(@EnumWinProc, Winapi.Windows.LPARAM(sl));

    for var i := 0 to sl.Count - 1 do
    begin
      CodeSite.Send('window handle', Winapi.Windows.HWND(sl.Objects[i]));
      CodeSite.Send('program-path sl[i]', sl[i]);
    end;
    ClearList(sl); // EDIT: forgot this line!
  finally
    sl.Free; 
  end;
end;

The crash is reported by EurekaLog:

2.1 Date : Sat, 31 Jul 2021 22:36:53 +0200
2.2 Address : 005509D2
2.5 Type : EInvalidPointer
2.6 Message : Application made attempt to free invalid or unknown memory block: $00010AE2 OBJECT [?] 0 bytes.
2.7 ID : B5002468
2.8 Count : 1
2.11 Sent : 0

This is at the top of the EurekaLog call stack:

You say that the crash occurs in `ClearList`, but you never call `ClearList`, so that cannot be true. Or, perhaps you *are* calling `ClearList` before you free the `TStringList` (in your actual code)? If so, the bug is clear: your `ClearList` will free each object in the list, but will not clear the pointer to it. Hence, these pointers become dangling. And since you have set `OwnsObjects = True`, the string list's destructor (invoked at `Free`) will attempt to destroy the "objects" pointed to by these dangling pointers. Either free the objects yourself, or let the dtor do it. — Andreas Rejbrand, Jul 31 '21 at 21:50
Oh, and also: the "objects" you are putting in the list aren't Delphi objects at all -- they are merely HWNDs (integers) which are definitely NOT pointing to Delphi objects. So there isn't anything to free. You mustn't free an object pointer which isn't an object pointer! Not twice, not even once! When you do `aList.AddObject(strPath, TObject(wHandle));` you tell Delphi, "I know `wHandle` is an integer, and not an object, but please pretend that this integer is an object. I promise I will never trust you when you later on tell me it is an object". And then, a bit later, you have forgotten ... — Andreas Rejbrand, Jul 31 '21 at 21:53
...this and treat the "objects" like objects: `.Free` and `OwnsObjects = True`. (Recall that, in Delphi, an object variable is a native-sized integer variable whose value is either `nil` or the address of a Delphi object on the heap. So every object variable is an integer (an address), but a "random" integer most certainly isn't a pointer to a valid object on the heap!) — Andreas Rejbrand, Jul 31 '21 at 21:54
@AndreasRejbrand Sorry, I forgot to add the `ClearList` line from my code. See the edited Q. — user1580348, Jul 31 '21 at 21:59
So, the tl;dr version: (1) You are putting integers as objects, so you mustn't treat these "objects" as objects, because they aren't. In particular, there isn't anything to free, and you mustn't free these "objects". Don't run a dtor on a "random" pointer. (2) IF you had indeed been putting real objects in the list, you would still have had a bug, because you would first have freed them yourself without `nil`ing their references, so the string list's dtor would then try to "destroy" the things pointed to by these dangling pointers. Again very bad. — Andreas Rejbrand, Jul 31 '21 at 22:01
@AndreasRejbrand The crash also occurs without the `ClearList` routine, simply when calling `sl.Free`! — user1580348, Jul 31 '21 at 22:03
Yes, that is because you set `OwnsObjects` to `True`. This tells the Delphi RTL "hey, mr String List, when you are freed, please also free all these objects whose pointers I have been giving you". And since the Delphi RTL is rather obedient, it will try to run the destructor on each of these HWNDs (which aren't pointers to Delphi objects!). It will thus try to run the dtor on a "random" pointer. Crash. More succinctly: when you tell the string list that it owns the objects, you also tell it that the "pointers" are objects. But they aren't. They are just integers (HWNDs) that you saved in... — Andreas Rejbrand, Jul 31 '21 at 22:05
...`TObject` variables by using an unsafe typecast (`TObject(...)`). — Andreas Rejbrand, Jul 31 '21 at 22:07
Essentially, you are doing `TObject(random integer).Free`. Compare with this situation: If you had `aList.AddObject(strPath, TFrog.Create);` you would create one new `TFrog` object on each iteration and put an address to the `TFrog` heap object in the list. Then you would really need to free these frog objects, EITHER by telling the RTL to do it ( `OwnsObjects = True`), OR by doing it yourself. Not both. — Andreas Rejbrand, Jul 31 '21 at 22:08
@AndreasRejbrand When I remove both statements `sl.OwnsObjects := True;` and `ClearList(sl)` then there is no crash. Is this the solution? — user1580348, Jul 31 '21 at 22:11
Correct, that's the solution. In Delphi, you don't need to free integers! — Andreas Rejbrand, Jul 31 '21 at 22:11
So when packaging `Integers` into a `TStringList` by `AddObject` in Delphi then there is no need to free these "objects" because, in reality, they are not objects? — user1580348, Jul 31 '21 at 22:15
Correct. In fact, you mustn't free them, because trying to do so would cause a crash. (Specifically, you would give the RTL a random pointer to some part of your computer's RAM and tell the RTL that "this is the address of a Delphi object, I promise, please treat it as such and do the destruction procedure on it". And if that was some random memory in your RAM (maybe related to some completely different part of your app), then it isn't good to start randomly altering it!) — Andreas Rejbrand, Jul 31 '21 at 22:18
Why don't you put the paths and the HWNDs as key-value pairs in the stringlist, like this ``aList.Add(strPath + '=' + IntToStr(wHandle));``? Or use a generic ``TDictionary`` — Delphi Coder, Jul 31 '21 at 22:36
@DelphiCoder Yes, that would be a possibility - but then I would have to convert them back to `Integers` which could be another source for problems... — user1580348, Jul 31 '21 at 22:38
@DelphiCoder Yes, `TDictionary` would be another possibility. — user1580348, Jul 31 '21 at 22:42
Linking to the source of that code: [How to get applications from Windows Task manager (Applications Tab) + their locations on HDD from Delphi](https://stackoverflow.com/a/14192197/4299358). This question's code could be drastically reduced for the actual problem. — AmigoJack, Aug 01 '21 at 07:11

score 4 · Accepted Answer · answered Jul 31 '21 at 22:35

There are two problems here:

First, you are putting integers as objects in the string list, so you mustn't treat these "objects" as objects, because they aren't.

In particular, there isn't anything to free, and you mustn't free these "objects"; that would be equivalent to doing TObject(some random pointer).Free.

Second, if you had indeed been putting real objects in the list, you would still have had a bug, because you would first have freed them yourself without niling their references, so the string list's destructor would then try to "destroy" the things pointed to by these dangling pointers. Again, bad.

Plus the indecision of using both `pred(List.Count)` and `sl.Count - 1`. — AmigoJack, Aug 01 '21 at 08:44

Complex problem when trying to free a StringList with objects crashes my app

1 Answers1