Recent

Author Topic: Heaptrace - positive false ?!  (Read 2523 times)

af0815

  • Sr. Member
  • ****
  • Posts: 360
Heaptrace - positive false ?!
« on: March 05, 2019, 02:02:59 pm »
I work with heaptrace on a project and found some messages for lost blocks. This bloats up the heaptrace, and i found no reason why it claims thats blocks in ZEOS (from OPM).

My Version: Lazarus 2.0.1 r60486 FPC 3.2.0 i386-win32-win32/win64

Quote
Call trace for block $1196BCB0 size 23
  $006FE26E  ESCAPE,  line 126 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006FE8A9  TZURL__SETDATABASE,  line 229 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006E32DB  TZABSTRACTCONNECTION__SETDATABASE,  line 398 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/component/ZAbstractConnection.pas
  $0043EF1C  TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
  $0043E114  TFORMMAIN__FORMACTIVATE,  line 291 of frmmain.pas
  $0041F338  TCUSTOMFORM__ACTIVATE,  line 680 of ./include/customform.inc
  $0041F79B  TCUSTOMFORM__CMACTIVATE,  line 842 of ./include/customform.inc
  $0040E9F1
  $0052704B  TWINCONTROL__WNDPROC,  line 5419 of ./include/wincontrol.inc
  $00420C2B  TCUSTOMFORM__WNDPROC,  line 1477 of ./include/customform.inc
  $0052FC14  TCONTROL__PERFORM,  line 1581 of ./include/control.inc
  $00426852  TSCREEN__SETFOCUSEDFORM,  line 899 of ./include/screen.inc
  $0042315B  TCUSTOMFORM__SETFOCUSEDCONTROL,  line 2556 of ./include/customform.inc
  $00526E69  TWINCONTROL__WNDPROC,  line 5347 of ./include/wincontrol.inc
  $005BF94E  DELIVERMESSAGE,  line 112 of lclmessageglue.pas
  $0050ECA1  TWINDOWPROCHELPER__DOWINDOWPROC,  line 2511 of ./win32/win32callback.inc
Call trace for block $11934218 size 27
  $006FE26E  ESCAPE,  line 126 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006FE7F9  TZURL__SETHOSTNAME,  line 214 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006E325B  TZABSTRACTCONNECTION__SETHOSTNAME,  line 378 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/component/ZAbstractConnection.pas
  $0043EEFE  TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
  $0043E114  TFORMMAIN__FORMACTIVATE,  line 291 of frmmain.pas
  $0041F338  TCUSTOMFORM__ACTIVATE,  line 680 of ./include/customform.inc
  $0041F79B  TCUSTOMFORM__CMACTIVATE,  line 842 of ./include/customform.inc
  $0040E9F1
  $0052704B  TWINCONTROL__WNDPROC,  line 5419 of ./include/wincontrol.inc
  $00420C2B  TCUSTOMFORM__WNDPROC,  line 1477 of ./include/customform.inc
  $0052FC14  TCONTROL__PERFORM,  line 1581 of ./include/control.inc
  $00426852  TSCREEN__SETFOCUSEDFORM,  line 899 of ./include/screen.inc
  $0042315B  TCUSTOMFORM__SETFOCUSEDCONTROL,  line 2556 of ./include/customform.inc
  $00526E69  TWINCONTROL__WNDPROC,  line 5347 of ./include/wincontrol.inc
  $005BF94E  DELIVERMESSAGE,  line 112 of lclmessageglue.pas
  $0050ECA1  TWINDOWPROCHELPER__DOWINDOWPROC,  line 2511 of ./win32/win32callback.inc
Call trace for block $1196BF30 size 23
  $006FE26E  ESCAPE,  line 126 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006FE9C9  TZURL__SETPASSWORD,  line 249 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006E335B  TZABSTRACTCONNECTION__SETPASSWORD,  line 418 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/component/ZAbstractConnection.pas
  $0087875B  TDMSQLCON__DATAMODULECREATE,  line 300 of C:/data/Nope/hugo/common/dmconnection_sql.pas
  $0047156D
  $00471492
  $00878669  GETCONNECTIONSQLG16H,  line 280 of C:/data/Nope/hugo/common/dmconnection_sql.pas
  $0043EED9  TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
  $0043E114  TFORMMAIN__FORMACTIVATE,  line 291 of frmmain.pas
  $0041F338  TCUSTOMFORM__ACTIVATE,  line 680 of ./include/customform.inc
  $0041F79B  TCUSTOMFORM__CMACTIVATE,  line 842 of ./include/customform.inc
  $0040E9F1
  $0052704B  TWINCONTROL__WNDPROC,  line 5419 of ./include/wincontrol.inc
  $00420C2B  TCUSTOMFORM__WNDPROC,  line 1477 of ./include/customform.inc
  $0052FC14  TCONTROL__PERFORM,  line 1581 of ./include/control.inc
  $00426852  TSCREEN__SETFOCUSEDFORM,  line 899 of ./include/screen.inc
Call trace for block $1196ABB0 size 17
  $006FE26E  ESCAPE,  line 126 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006FE939  TZURL__SETUSERNAME,  line 239 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/core/ZURL.pas
  $006E331B  TZABSTRACTCONNECTION__SETUSER,  line 408 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/component/ZAbstractConnection.pas
  $0087874B  TDMSQLCON__DATAMODULECREATE,  line 299 of C:/data/Nope/hugo/common/dmconnection_sql.pas
  $0047156D
  $00471492
  $00878669  GETCONNECTIONSQLG16H,  line 280 of C:/data/Nope/hugo/common/dmconnection_sql.pas
  $0043EED9  TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
  $0043E114  TFORMMAIN__FORMACTIVATE,  line 291 of frmmain.pas
  $0041F338  TCUSTOMFORM__ACTIVATE,  line 680 of ./include/customform.inc
  $0041F79B  TCUSTOMFORM__CMACTIVATE,  line 842 of ./include/customform.inc
  $0040E9F1
  $0052704B  TWINCONTROL__WNDPROC,  line 5419 of ./include/wincontrol.inc
  $00420C2B  TCUSTOMFORM__WNDPROC,  line 1477 of ./include/customform.inc
  $0052FC14  TCONTROL__PERFORM,  line 1581 of ./include/control.inc
  $00426852  TSCREEN__SETFOCUSEDFORM,  line 899 of ./include/screen.inc

and the code from ZEOS

Code: Pascal  [Select]
  1. // escape the ';' char to #9 and LineEnding to ';'
  2. function Escape(const S: string): string; {$IFDEF WITH_INLINE}inline;{$ENDIF}
  3. begin
  4.   Result := ReplaceChar(';', #9, S);   // <<--- Line 126 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
  5.   Result := StringReplace(Result, LineEnding, ';', [rfReplaceAll]);
  6. end;
  7.  
and
Code: Pascal  [Select]
  1. {**
  2.   Replace chars in the string
  3.   @param Source a char to search.
  4.   @param Target a char to replace.
  5.   @param Str a source string.
  6.   @return a string with replaced chars.
  7. }
  8. function ReplaceChar(const Source, Target: Char; const Str: string): string;
  9. var
  10.   P: PChar;
  11.   I:Integer;
  12. begin
  13.   Result := Str;
  14.   UniqueString(Result);
  15.   P := Pointer(Result);
  16.   for i := 0 to Length(Str) - 1 do
  17.   begin
  18.     if P^ = Source then
  19.       P^ := Target;
  20.     Inc(P);
  21.   end;
  22. end;
  23.  
  24.  

Any hint whats wrong here.
regards
Andreas

Thaddy

  • Hero Member
  • *****
  • Posts: 9181
Re: Heaptrace - positive false ?!
« Reply #1 on: March 05, 2019, 02:18:26 pm »
The most likely is that you included the heaptrc unit yourself: you should not. You should select the option in Lazarus or compile with FPC with the -gh command-line option.
It is highly unlikely(actually nay impossible) that heaptrace returns false positives, because it is a memory manager that registers use and free.

Even the silly UniqueString that you use should not matter.
« Last Edit: March 05, 2019, 02:24:24 pm by Thaddy »
also related to equus asinus.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #2 on: March 05, 2019, 02:29:31 pm »
Line 126, "replace" is creating a new string. That is where the memory for that string is allocated.

Well you would assume it is allocated inside "replace", but that may be hidden by optimization.

This string is then passed back out. And at some point the string is stored, and leaked.
This can be in a global variable, or the field of some object that is itself leaked.

The line that they all have in common is
TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
so that would be the next bit to examine.

Thaddy

  • Hero Member
  • *****
  • Posts: 9181
Re: Heaptrace - positive false ?!
« Reply #3 on: March 05, 2019, 02:33:56 pm »
I think calling uniquestring from passed const parameters is duplicate code... result is a copy anyway. But I guess using the heaptrc unit by hand is a more likely candidate...
also related to equus asinus.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #4 on: March 05, 2019, 02:44:26 pm »
I think calling uniquestring from passed const parameters is duplicate code... result is a copy anyway.
No, its not.

Result would still be a "copy on write" pointer to the "const string".
But pchar access, does not trigger "copy on write", so UniqueString is needed.

But that should not be the issue.

af0815

  • Sr. Member
  • ****
  • Posts: 360
Re: Heaptrace - positive false ?!
« Reply #5 on: March 05, 2019, 02:51:36 pm »
The most likely is that you included the heaptrc unit yourself: you should not. You should select the option in Lazarus or compile with FPC with the -gh command-line option.
It is highly unlikely(actually nay impossible) that heaptrace returns false positives, because it is a memory manager that registers use and free.

Even the silly UniqueString that you use should not matter.
Heaptrace is only invoked by Lazarus via the Option menu.

This code is from ZEOS, not me. I found only, this is not logical form me. And this issues are new for me.
regards
Andreas

af0815

  • Sr. Member
  • ****
  • Posts: 360
Re: Heaptrace - positive false ?!
« Reply #6 on: March 05, 2019, 03:08:29 pm »
The line that they all have in common is
TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
so that would be the next bit to examine.
The line is
Quote
SQL.HostName:= coConnection;     // <<------------------

a full sample, but i see it is always with a constant

Code: Pascal  [Select]
  1. ......
  2. const
  3.   coConnection = '192.168.1.111';
  4.   coDatabase = 'my-db';
  5. ......
  6.  
  7. procedure TDMSQLCon.DataModuleCreate(Sender: TObject);
  8. begin
  9.   DebugLn('TDMSQLCon.DataModuleCreate start');
  10.   try
  11.      DebugLn('normal SQL');
  12.     // Nothing to do yet
  13.      SQLCon.Connected:=false;
  14.      //
  15.      SQLCon.Protocol := 'FreeTDS_MsSQL>=2005';
  16.      SQLCon.HostName:= coConnection;     // <<------------------
  17.      SQLCon.User:= 'XXXXl';
  18.      SQLCon.Password := 'YYYYYYYY';
  19.      SQLCon.Database:= coDatabase;
  20.      //
  21.   finally
  22.      //
  23.   end;
  24.   DebugLn('TDMSQLCon.DataModuleCreate end');
  25. end;
  26.  

So it is not a global variable, it is a constant.
regards
Andreas

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #7 on: March 05, 2019, 05:11:35 pm »
Question: Is the leak list in your first post complete?

There are no other leaks? (Assuming heaptrc does not terminate the output early due to any other error).
If there are other leaks, then they may hold the key to this leak.
E.g., if SQLCon (or some of its related/contained objects) were leaked, you would leak those strings too.

Otherwise, another path of investigation is to go for potential errors in the compiler.
Does the issue happen on 32/64bit? With 3.0.4 too? On other OS? (If it happens on linux, you can use valgrind (requires cmem) too verify them).

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #8 on: March 05, 2019, 05:33:57 pm »
Also, strange:
Quote
Code: [Select]
  $006E32DB  TZABSTRACTCONNECTION__SETDATABASE,  line 398 of C:/data/lazdev/LazXXX/OPM/packages/zeosdbo/src/component/ZAbstractConnection.pas
  $0043EF1C  TFORMMAIN__STARTUP,  line 1251 of frmmain.pas
But that is not on the same line as the call to "SetHostname".

So while I would still think that some memory got leaked, I could not be sure, that the line numbers are 100% correct. (valgrind (linux only) may shed some light)



Code: [Select]
Call trace for block $11934218 size 27That might be the size of your hostname string (including header). 13 chars, possible 12 bytes header (not sure, see "TAnsiRec"), one terminating #0 == 26. Not sure where the 1 byte diff comes from, and only if TAnsiRec  12 bytes.



---
EDIT

I can also confirm, that UniqueString is usually optimized in a way that would hide its caller in the stacktrace. So if the string leaked was created by it, it would skip "Escape" in the trace.
« Last Edit: March 05, 2019, 05:48:46 pm by Martin_fr »

Thaddy

  • Hero Member
  • *****
  • Posts: 9181
Re: Heaptrace - positive false ?!
« Reply #9 on: March 05, 2019, 05:52:07 pm »
Apart from martin being a bit off here... This code, but commented now should help you:
Code: Pascal  [Select]
  1. function ReplaceChar(const Source, Target: Char; const Str: string): string;
  2. var
  3.   P: PChar;
  4.   I:Integer;
  5. begin
  6.   Result := Str;
  7.   UniqueString(Result); // Not necessary
  8.   P := Pointer(Result);// this points to the wrong location: the string descriptor part
  9.   for i := 0 to Length(Str) - 1 do // so this makes no sense at all
  10.   begin
  11.     if P^ = Source then
  12.       P^ := Target;
  13.     Inc(P);
  14.   end;
  15. end;
Pascal strings have a payload at negative offset of the content. You should not access them like that. Period.
I can only replicate leaks when I call Uniquestring.... Maybe it is not safe to call Uniquestring from inside a method?
 
« Last Edit: March 05, 2019, 05:54:29 pm by Thaddy »
also related to equus asinus.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #10 on: March 05, 2019, 06:08:25 pm »
Code: Pascal  [Select]
  1.   UniqueString(Result); // Not necessary
  2.   P := Pointer(Result);// this points to the wrong location: the string descriptor part

I know I have your devil smileys coming.... But none the less, both your comments are plain wrong.
For the 2nd comment, find TAnsiString's definition in the compiler source code, and read the comment above that.

Yes omitting "UniqueString" may remove the leak. But at the cost of other bugs.

Nevertheless, using "UniqueString" (from the code shown sofar) is not the culprit for the leak. Unless there is a bug in the compiler, but then it would be that bug....
« Last Edit: March 05, 2019, 06:12:35 pm by Martin_fr »

af0815

  • Sr. Member
  • ****
  • Posts: 360
Re: Heaptrace - positive false ?!
« Reply #11 on: March 05, 2019, 06:23:20 pm »
Question: Is the leak list in your first post complete?
No, it is not complete. It is a very, very long heaptrace file and Lazarus need a lot of time to create it. But this was the first (on top of the file) and not clear issue (for me).
The project itself is to great and a closed source applikation. It is complex, it use frames in frames at runtime and build a lot of parts dynamicly. It is Win32 only (running on Win10/64 with latest patches). 

I try to make it simply to test, why this happens. With a simple testapp, i didn't see a problem with zeos and this code posted above. But i am searching for an example (or soloution)
regards
Andreas

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #12 on: March 05, 2019, 07:25:15 pm »
In that case probably look further down in the list.
If you leak some object, that often leaks other data.

The string, may have been created at that point, but it could then have been assigned to other objects later.

If you try to find this, you have to find where the string is read directly from the field.
From what I see the getter uses UnEscape which creates a new unique string, at a different memory location.

But if anything directly accesses FDataBase/FHostName, then that could hold an to the strings memory and leak it.

-----------------
Actually just looked at it:

There is a bug in  UnEscape/ReplaceChar.
I was not able to construct a simple case where this would end in a leak.
There are however plenty of cases where this could lead to random and/or crashing behaviour.


You need a very good understanding of what "const x:string" does. Which is the opposite of what most people expect.
One thing it does is, to prevent ref-counting.


In ReplaceChar
Quote
Result := ReplaceChar(#9, ';', Result);
The value returned by ReplaceChar is assigned to the same variable that is passed in as "Str".

Note also that "Result" variables (for managed types, such as string) are not actually returned, but passed by the caller to the function.


Code: Pascal  [Select]
  1. function ReplaceChar(const Source, Target: Char; const Str: string): string;
  2. begin
  3.   Result := Str;
  4.  

So Result and Str in ReplaceChar are the same string (pointing to the same memory).


But then
  Result := Str;
modifies Result (decrease the ref count of the old value). And by that it modifies "Str". But Str is const, and therefore you are not allowed to modify it.


And yes this is documented https://www.freepascal.org/docs-html/ref/refsu67.html
See the "side effect" example.
The param is modified via the global var. And in the example it is the global var that points to the same string. Above it is result, that points to the same string...

So there is a bug in zeos.

ASerge

  • Hero Member
  • *****
  • Posts: 1411
Re: Heaptrace - positive false ?!
« Reply #13 on: March 05, 2019, 07:41:17 pm »
Code: Pascal  [Select]
  1. function ReplaceChar(const Source, Target: Char; const Str: string): string;
  2. begin
  3.   Result := Str;
  4.  
So Result and Str in ReplaceChar are the same string (pointing to the same memory).

But then
  Result := Str;
modifies Result (decrease the ref count of the old value). And by that it modifies "Str". But Str is const, and therefore you are not allowed to modify it.
All this does not matter, because the next is UniqueString(Result).

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5699
    • wiki
Re: Heaptrace - positive false ?!
« Reply #14 on: March 05, 2019, 08:08:47 pm »
All this does not matter, because the next is UniqueString(Result).
And if at that point "Str" is a dangling pointer, then how does that help.

In fact, assuming that the Result/Str starts with a refcount of 1

   Result := Str;
Does
  dec-ref(result) // and result := nil
  result := str
  inc-ref(result)

or maybe
  dec-ref(result) // and result := nil
  inc-ref(str)
  result := str

In both cases, after the dec-ref, str is a dangling pointer.
The inc-ref (of the ref count in the not yet overwritten memory to which str points) does not change that str is a dangling pointer.

In Uniquestring, if result has a refcount of 1, then nothing happens. (with the consequence that result still is the same as "Str", and changes affect both)

Obviously in the original post its a mem leak, rather than the original Str being modified, so that would be a different bug.

--------------------
If the refcount was greater than 1 at the beginning, then the code inside ReplaceChar should work as expected.

---------------------
It all depends on what exactly is passed from the caller.
It is possible that (by pure luck) all scenarios are such that they will work out as indented. (that is the ref-count is never 1)

--------------------
EDIT

Actually, in the example I found (UnEscape), it should always work, because the prior "Result := StringReplace..." makes sure refcount is at least 2.

--------------------
EDIT

Actually,  I was wrong, but in a different way.

Afaik, a temp variable is passed in, and not actually the outer result. So the const var is not modified, as I thought.

« Last Edit: March 05, 2019, 08:42:24 pm by Martin_fr »