Recent

Author Topic: Reference counting problem, memory leak, nulling result record with string  (Read 2481 times)

prof7bit

  • Jr. Member
  • **
  • Posts: 63
Hello,

I have encountered something I wasn't aware of before and it took me 2 days to debug it and strip it down to a minimal working example (was a large project, happened across multiple units).

This is the most minimal version i could come up with that demonstrates the effect:

FPC version is 3.2.3 (fixes branch from a week ago)

Code: Pascal  [Select][+][-]
  1. program test2;
  2. uses
  3.   Classes, sysutils;
  4.  
  5. type
  6.   TSomeRec = record
  7.     SomeString: String;
  8.   end;
  9.  
  10. function GetRec(L: String): TSomeRec;
  11. begin
  12.   FillByte(Result{%H-}, SizeOf(Result), 0);  // without this no memory leak!
  13.   Result.SomeString := L;
  14. end;
  15.  
  16. var
  17.   SL: TStringList;
  18.   R: TSomeRec;
  19.   I: Integer;
  20.   S: String;
  21.  
  22. begin
  23.   SL := TStringList.Create;
  24.  
  25.   for I := 0 to 10 do
  26.   begin
  27.     SL.Add(Format('str number %d', [I]));
  28.   end;
  29.  
  30.   for S in SL do
  31.   begin
  32.     R := GetRec(S);
  33.   end;
  34.  
  35.   SL.Free;
  36. end.
  37.  

If you compile this with heaptrc unit it will complain about 10 unfreed memory blocks and these are the 10 strings created by the format function.

I found that when I remove the FillByte the leak disappears.

The reson for it being there in the first place is this is a complicated record containing lots of fields and i wanted to initialize it entirely into a defined state before I begin filling some of its fields later in that function.

Question 1:
I am not even sure whether I am supposed to initialize result records, they live on the stack and could contain random data? The compiler complains if I do not initialize result variables.

Question 2:
Am I even allowed to ever null a string variable (in a record) in such a crude way or will this always cause potential problems?

Question 3:
Or is this a compiler bug?

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 9522
  • FPC developer.
Insert finalize(result) before the fillbyte.

Bart

  • Hero Member
  • *****
  • Posts: 4415
    • Bart en Mariska's Webstek
I don't get a leak report form heaptrace with that code.
Tested on windows (32-bit fpc) with 3.2.0 and 3.3.1 r49115 (2021-04-03 21:42:11).

What happens if you use default intrinsic instead of fillbyte?
Code: Pascal  [Select][+][-]
  1. function GetRec(L: String): TSomeRec;
  2. begin
  3.   Result := Default(TSomeRec);
  4.   Result.SomeString := L;
  5. end;

Bart

prof7bit

  • Jr. Member
  • **
  • Posts: 63
I don't get a leak report form heaptrace with that code.
Tested on windows (32-bit fpc) with 3.2.0 and 3.3.1 r49115 (2021-04-03 21:42:11).
What happens if you use default intrinsic instead of fillbyte?
Code: Pascal  [Select][+][-]
  1. function GetRec(L: String): TSomeRec;
  2. begin
  3.   Result := Default(TSomeRec);
  4.   Result.SomeString := L;
  5. end;

Bart

Default() fixes the problem. Again I have learnt something new today, after 10+ years of using it I have never come across this intrinsic. It also looks much more elegant to use that instead of Fillbyte.

I'm on Linux, 64Bit, 3.2.3 compiled with fpcupdeluxe


prof7bit

  • Jr. Member
  • **
  • Posts: 63
Insert finalize(result) before the fillbyte.
That also seems to work.

Bart

  • Hero Member
  • *****
  • Posts: 4415
    • Bart en Mariska's Webstek
I don't get a leak report form heaptrace with that code.

Sorry, tested accidentally with shortstrings.
I also got the leak with longstrings.

Bart

Thaddy

  • Hero Member
  • *****
  • Posts: 10809
Code: Pascal  [Select][+][-]
  1. function GetRec(const L: String): TSomeRec;
The const for L prevents the refcount going haywire.
This should work with your original code.
« Last Edit: August 04, 2021, 02:30:18 pm by Thaddy »

engkin

  • Hero Member
  • *****
  • Posts: 2924
What did prof7bit do wrong?

prof7bit

  • Jr. Member
  • **
  • Posts: 63
I think I have understood what is going on.

I found that if I put a WriteLn(Result.SomeString); at the top of my GetRec() function like so:

Code: Pascal  [Select][+][-]
  1. function GetRec(L: String): TSomeRec;
  2. begin
  3.   WriteLn('Result: ' + Result.SomeString);
  4.   FillByte(Result{%H-}, SizeOf(Result), 0);  // without this no memory leak!
  5.   Result.SomeString := L;
  6. end;
  7.  

then it will give the following output:

Code: [Select]
Result:
Result: str number 0
Result: str number 1
Result: str number 2
Result: str number 3
Result: str number 4
Result: str number 5
Result: str number 6
Result: str number 7
Result: str number 8
Result: str number 9

which leads me to the assumption that the compiler will treat this function call as if Result was passed as a var argument:

Code: Pascal  [Select][+][-]
  1. procedure GetRec(L: String; var Result: TSomeRec);
  2. begin
  3.   FillByte(Result{%H-}, SizeOf(Result), 0);  // sabotage FPC reference counting!
  4.   Result.SomeString := L;
  5. end;
  6.  

and call it like so:

Code: Pascal  [Select][+][-]
  1.   for S in SL do
  2.   begin
  3.     GetRec(S, R);
  4.   end;
  5.  

This will produce the exact same symptoms. So what happens is I erroneously assumed I get a brand new fresh result variable which which I am free to do what I want before I return it, that it is created and owned by the function that returns it but in reality the caller of the function will provide the result variable and in this case it still contains a valid string from the previous iteration.

Result contains a valid string with a valid reference count, it is not some random garbage. I MUST use the proper fpc way of assigning a new string, only this will ensure it will decref the previously contained string.

FillByte would wipe out the pointer to a perfectly valid string that still has reference count without first dealing with the refcount and disposing it if not used anymore.

The problem becomes much clearer when looking at the problematic code when written like so:
Code: Pascal  [Select][+][-]
  1. procedure GetRec(L: String; var Result: TSomeRec);
  2. begin
  3.   // there is a valid string in here already, we cannot just wipe it with FillByte
  4.   // because if we do so FPC has no chance to properly decref the old string.
  5.   FillByte(Result{%H-}, SizeOf(Result), 0);  // leaves a string dangling!
  6.   Result.SomeString := L;
  7. end;
  8.  
  9. var
  10.   SL: TStringList;
  11.   R: TSomeRec;
  12.   I: Integer;
  13.   S: String;
  14.  
  15. begin
  16.   SL := TStringList.Create;
  17.  
  18.   for I := 0 to 10 do
  19.   begin
  20.     SL.Add(Format('str number %d', [I]));
  21.   end;
  22.  
  23.   for S in SL do
  24.   begin
  25.     GetRec(S, R);
  26.   end;
  27.  
  28.   SL.Free;
  29. end.
  30.  

In the first iteration we assign a string to R.SomeString. In the second and all following iterations we null the pointer with methods outside of fpc's control, thereby leave the memory of the previous string dangling and assign a new string to the record field.

Finalize() or a proper assignment (not raw byte copying) will take care of the previous contents of the result variable and orderly decref and dispose any valid strings that might still be in there.

Lessons learned
:
  • The result variable might contain valid managed types that originate from somewhere inside the caller of the function or from previous calls in a loop, never null it with FillByte()
  • Since 3.0.0 there is a compiler intrinsic Default() that is much more elegant for zeroing records than the usage of FillByte(). Using this will involve the assignment operator instead of raw byte copying and therefore take care of all the reference counting needs when overwriting the contents of managed types.

« Last Edit: August 04, 2021, 04:19:27 pm by prof7bit »

PascalDragon

  • Hero Member
  • *****
  • Posts: 3316
  • Compiler Developer
This will produce the exact same symptoms. So what happens is I erroneously assumed I get a brand new fresh result variable which which I am free to do what I want before I return it, that it is created and owned by the function that returns it but in reality the caller of the function will provide the result variable and in this case it still contains a valid string from the previous iteration.

Result contains a valid string with a valid reference count, it is not some random garbage. I MUST use the proper fpc way of assigning a new string, only this will ensure it will decref the previously contained string.

FillByte would wipe out the pointer to a perfectly valid string that still has reference count without first dealing with the refcount and disposing it if not used anymore.

Correct. Record types that contain managed types (as well as records that use management operators) are considered managed types themselves and for these the Result is always passed as a hidden var-parameter and the compiler does not initialize those for you (thus the same will also happen if your result type is a String or an array).

 

TinyPortal © 2005-2018