Thursday, January 10, 2008

Potential memory leaks by initializing a record

Delphi uses reference-counting with copy-on-write semantics [1][2] to reduce memory allocation for strings (not for WideString). A kind of memory leak was found by accident. Let us first look the following example:

type
TFoo = record
StringField: string;
end;

procedure CreateLeakTest;
var
Foo: TFoo;
begin
FillChar(Foo, SizeOf(Foo), 0);
Foo.StringField := 'Leak Test';
FillChar(Foo, SizeOf(Foo), 0); //<--- A leak!
end;

Initializing records with FillChar() is quite common in Delphi. By calling FillChar() twice, you might create a memory leak. Note that no leaks on ShortString. My assumption: FillChar() is unsafe to cleanup records with ref-counted fields.

function StringStatus(const S: string): string;
begin
Result := Format('Addr: %p, Refc: %d, Val: %s',
[Pointer(S), PInteger(Integer(S) - 8)^, S]);
end;

procedure Diagnose;
var
S: string;
Foo: TFoo;
begin
S := Copy('Leak Test', 1, 5); // Force to allocate a new string
WriteLn(StringStatus(S));
Foo.StringField := S;
WriteLn(StringStatus(Foo.StringField));
FillChar(Foo, SizeOf(Foo), 0);
WriteLn(StringStatus(S));
end;

The output of the above code looks as follows:
Addr: 00E249E8, Refc: 1, Val: Leak Test // A string buffer is allocated
Addr: 00E249E8, Refc: 2, Val: Leak Test // Its Refc is incremented
Addr: 00E249E8, Refc: 2, Val: Leak Test // Its Refc should equal 1 (unexpected)

After calling FillChar(), the StringField is pointed to nil. However the reference count of its previous string buffer hasn't been decremented, so that its reference count will NEVER go back to 0. In other words, this string buffer will not be deallocated before your program is terminated. This is a leak.

How to initialize a record in a safe way?

As ref-counted fields are not handled correctly by using FillChar(). The default way of initializing a record looks more like an abuse of FillChar. I suggest declare a const record with initial values instead of using FillChar.

const
EmptyRecordX: TRecordX = (
Field1: InitVal1;
Field2: InitVal2;
...
FidldN: InitValN
);

// In your application
var
Foo: TRecordX;
begin
Foo := EmptyRecordX; // instead of FillChar(Foo, SizeOf(Foo), 0);
//...

It is quite safe to initialize a record in this way, isn't it?

Alternative Solution

If you are too lazy to declare such empty record constants. The following function can help you as well. Note that it is a little bit tricky.

procedure InitRecord(out R; RecordSize: Integer);
begin
FillChar(R, RecordSize, 0);
end;

Thanks for the magic word "out". As it is in Help described "An out parameter, like a variable parameter, is passed by reference. With an out parameter, however, the initial value of the referenced variable is discarded by the routine it is passed to. The out parameter is for output only; that is, it tells the function or procedure where to store output, but doesn't provide any input. "

Let us see what the code actually has done to a record.

mov edx,[$0040c904]
mov eax,ebx
call @FinalizeRecord //<----- cleanup
mov edx,$0000000c
call InitializeRecord

Compile calls procedure FinalizeRecord(), so that a record will be completely finalized.

UPDATE #1: As Jonas Maebe recently described: "If you have local record which was declared but not yet used, a simple fillchar(rec,sizeof(rec),0) will set everything to 0/nil/empty. If it may have been used earlier and contains ref-counted fields, you first have to call finalize(rec). "[3] His argument is more understandable and closer to the point of the issue.

References

  1. Wikipedia: Copy-on-write
  2. A Brief History of Strings
  3. fpc-pascal maillist
 

6 comments:

Anonymous said...

Why do you use FillChar on AnsiString ?! you should use empty string instead.

using #0 on ansi string is not the right way either, it's not a null terminated string, however most string functions will stop on the first encounter of #0

L505 said...

See also:

http://www.mail-archive.com/fpc-pascal@lists.freepascal.org/msg12214.html
http://www.mail-archive.com/fpc-pascal@lists.freepascal.org/msg12211.html

Anonymous: because initializing the string with empty '' is tedious when you have to iterate through 10 record fields or 300 record fields... we/he are/is trying to find out ways to initialize a record without tediously going through each record field. An ansistring will stop on the first encounter of #0 if you cast to a pchar and such things when passing into functions that deal with pchars such as winapi.. but a ansistring can hold null values you are right about.

L505 said...

See also Jonas comment:

http://www.mail-archive.com/fpc-pascal@lists.freepascal.org/msg12217.html

L505 said...

In other words the idea is not initializing the string but initializing Pointers to Nil and such integers to Zero in the record. An ansistring field is really just a smart pointer.

Anonymous said...

Thanks - a useful article indeed.
I'm busy implementing your articles solution.

Anonymous said...

You can just use Rec := Default(TRec); and no hacks..