3

I have some code which i did not write, but there is a memory leak. The real strangeness is the memory only leaks if i zero a structure before returning it.

Reproducible minimum code

The leak is reproducible in Delphi 5 and Delphi 7.

First we have a structure:

type
   TLocalFile = packed record
      FileName: AnsiString;
   end;

This structure is the private member of a CollectionItem object:

TEntry = class(TCollectionItem)
private
   FLocalFile: TLocalFile;
end;

Then we have the owning collection, which has a function that can return a populated structure:

TEntries = class(TCollection)
protected
    function GetLocalFile: TLocalFile;
public
    procedure DoStuff;
end;

With the weirdness located in the GetLocalFile function:

function TEntries.GetLocalFile: TLocalFile;
var
    s: AnsiString;
begin
    //Only leaks if i initialize the returned structure
//  FillChar(Result, SizeOf(Result), 0);
    ZeroMemory(@Result, SizeOf(Result));

    s := 'Testing Leak';
    Result.Filename := s; //'Testing leak';  only leaks if i set the string through a variable
end;

In reality this function is passed a stream, and returns a populated structure, but that's not important now.

Next we have a method of the collection that will populate all it's entries's LocalFile structures:

procedure TEntries.DoStuff;
var
    x: Integer;
begin
    for X := 0 to Count-1 do
    begin
       (Items[X] as TEntry).FLocalFile := GetLocalFile;
    end;
end;

Finally, we construction a collection, add 10 items to it, have them DoStuff, then free the list:

procedure TForm1.Button1Click(Sender: TObject);
var
    list: TEntries;
    i: Integer;
    entry: TCollectionItem;
begin
    list := TEntries.Create(TEntry);
    try
        for i := 1 to 10 do
            entry := list.Add;

        list.DoStuff;
    finally
        list.Free;
    end;
end;

We created 10 items, we leak 9 AnsiStrings.

The horrifying confusing things

There are ways in which this code doesn't leak. It only leaks when using an intermediate string stack variable

Change:

function TEntries.GetLocalFile: TLocalFile;
var
   s: AnsiString;
begin
   s := 'Testing Leak';
   Result.Filename := s; //'Testing leak';  only leaks if i set the string through a variable
end;

to

function TEntries.GetLocalFile: TLocalFile;
begin
   Result.Filename := 'Testing leak'; //doesn't leak
end;

and it doesn't leak.

The other method is not to initialize the structure before returning it:

Remove the call to FillChar or ZeroMemory, and it doesn't leak:

function TEntries.GetLocalFile: TLocalFile;
var
    s: AnsiString;
begin
    //Only leaks if i initialize the returned structure
//  FillChar(Result, SizeOf(Result), 0);
//  ZeroMemory(@Result, SizeOf(Result));

    s := 'Testing Leak';
    Result.Filename := s; //'Testing leak';  only leaks if i set the string through a variable
end;

These are strange resolutions. Whether i use an intermediate stack variable, or not, whether i Zero the structure or not, should not have any effect on memory cleanup.

I doubt this is a bug in the compiler. Which mean that i (meaning the person who wrote this) is doing something fundamentally wrong. i assume it has something to do with TCollectionItemClass. But i cannot for the life of me figure out what.

Full code

unit FMain;

interface

uses
  Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
  Dialogs, StdCtrls;

type
    TLocalFile = packed record
        FileName: AnsiString;
    end;

    TEntry = class(TCollectionItem)
   private
        FLocalFile: TLocalFile;
    end;

    TEntries = class(TCollection)
    protected
        function GetLocalFile: TLocalFile;
    public
        procedure DoStuff;
    end;

  TForm1 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
  private
  public
  end;


var
  Form1: TForm1;

implementation

{$R *.dfm}

uses
    contnrs;

procedure TForm1.Button1Click(Sender: TObject);
var
    list: TEntries;
    i: Integer;
    entry: TCollectionItem;
begin
    list := TEntries.Create(TEntry);
    try
        for i := 1 to 10 do
        begin
            entry := list.Add;
        end;

        list.DoStuff;
    finally
        list.Free;
    end;
end;

{ TEntries }

procedure TEntries.DoStuff;
var
    x: Integer;
    entry: TEntry;
begin
    for X := 0 to Count-1 do
    begin
        entry := Items[X] as TEntry;
        entry.FLocalFile := GetLocalFile;
   end;
end;

function TEntries.GetLocalFile: TLocalFile;
var
    s: AnsiString;
begin
    //Only leaks if i initialize the returned structure
//  FillChar(Result, SizeOf(Result), 0);
    ZeroMemory(@Result, SizeOf(Result));

    s := 'Testing Leak';
    Result.Filename := s; //'Testing leak';  only leaks if i set the string through a variable
end;

end.

Oh, and don't forget to add FastMM4 to your project (if you don't already have it built-in), so you can detect the leaks.

enter image description here

manlio
  • 18,345
  • 14
  • 76
  • 126
Ian Boyd
  • 246,734
  • 253
  • 869
  • 1,219
  • 1
    Continuing from Mason's answer, it may seem odd that having the ZeroMemory call *before* the use of `Result` causes a leak, but that's because the result variable is actually allocated once in the calling scope so the leak that occurs when you blank out the structure and hence the managed string pointer within is actually from the previous iteration. – 500 - Internal Server Error Mar 20 '14 at 21:59
  • It's a shame that `Result` is a var parameter rather than an out parameter. – David Heffernan Mar 20 '14 at 22:08
  • @DavidHeffernan You mean conceptually? With the compiler doing to allocation and "initialization" before the call and passing the reference to the callee? – Ian Boyd Mar 20 '14 at 22:37
  • No. I mean literally. The return value **is** a var parameter. You can pass values in through it!!! As you have discovered. – David Heffernan Mar 20 '14 at 22:42
  • @DavidHeffernan I meant the fact that there literally isn't a `var` keyword. But i might *literally* make it a `var` parameter: `procedure GetLocalFile(var Result: TLocalFile);` – Ian Boyd Mar 20 '14 at 22:49
  • 1
    The compiler transforms your function into the procedure in your previous comments, and then compiles that. This is documented. – David Heffernan Mar 20 '14 at 22:51
  • @DavidHeffernan Source? – Ian Boyd Mar 21 '14 at 00:34
  • 2
    http://docwiki.embarcadero.com/RADStudio/en/Program_Control#Handling_Function_Results – David Heffernan Mar 21 '14 at 07:02

1 Answers1

9

AnsiString (and its Unicode counterpart) is reference-counted by the compiler. You can't simply zero memory containing a reference to it; you need to assign '' to it so that the compiler will generate code to decrement the refcount and release the memory correctly.

You'll have similar problems trying to block-clear data structures containing references to dynamic arrays, Interfaces, or (some) variants.

If you're not using a Delphi version recent enough to support the Default compiler magic expression, (I believe it was introduced in D2009,) the best way to clear out a record safely would be to call Finalize first, and then zero the memory as you're doing.

Mason Wheeler
  • 82,511
  • 50
  • 270
  • 477
  • So that brings up the useful question: is there an accepted technique for initializing a structure? I cannot tell you how many times i call `ZeroMemory(@foo, SizeOf(TFoo)); foo.cbSize := SizeOf(TFoo)` – Ian Boyd Mar 20 '14 at 22:31
  • Perhaps a Finalize before zeroing might be helpful. – Uwe Raabe Mar 20 '14 at 22:32
  • @IanBoyd: That looks like you're setting up a structure for a Windows API call. Since the Windows API doesn't use managed types (I'm assuming there are no interface members, because Windows COM APIs tend to do it differently) that idiom is safe. You want to be careful doing that on a record that contains Delphi-specific types, though. – Mason Wheeler Mar 20 '14 at 22:33
  • You're right, that is the typical idiom for calling the Win32 API. But it's now in my head that the way to initialize a structure in Delphi is to zero out the memory. It's not impossible to interop with Windows using a structure declared with `Variant`, `WideString` or `AnsiString`, as they're all just `VARIANT`, `PWideChar`, `PChar` inside. Is there a *"correct"* way to initialize a struct in Delphi? – Ian Boyd Mar 20 '14 at 22:35
  • `Result := Default(TLocalFile);` – David Heffernan Mar 20 '14 at 22:53
  • @IanBoyd: What David said, if you're using D2009 or later (I believe that's when `Default` was introduced.) But you tagged this delphi-7, so you basically have to either use ZeroMemory or do it by hand. You really should update, though; there have been a lot of improvements between D7 and XE. (And a few since then, but not much if you're a Windows developer.) – Mason Wheeler Mar 20 '14 at 22:59
  • Finalize(...) first, then FillChar, on legacy Delphi – David Heffernan Mar 20 '14 at 23:06
  • @David: Can `Finalize` be used on a record type? I was under the impression that it only worked on pointers. I might be wrong, though... – Mason Wheeler Mar 20 '14 at 23:10
  • `Finalize` can be used on any variable. It only has effect on variables of managed types. It is pointless on pointers therefore. Your advice to use zeroMemory without Finalize is exactly what caused Ian's leak. – David Heffernan Mar 20 '14 at 23:14
  • @DavidHeffernan: ...oh. I think, it was `Dispose` that I was thinking of, then. Nevermind. – Mason Wheeler Mar 20 '14 at 23:27
  • In fact you should add Finalize to your answer. Pass the record to it. – David Heffernan Mar 20 '14 at 23:31
  • Actually, @DavidHeffernan, you had a pre-2009 answer somewhere else. `const Default_TLocalFile: TLocalFile = ();` I was as surprised as anyone that it compiles, *and* that it works. – Ian Boyd Mar 21 '14 at 00:32
  • @IanBoyd: Yeah, that looks strange at first glance but it makes sense if you know how the const record syntax works. :P – Mason Wheeler Mar 21 '14 at 00:47