Recent

Author Topic: Can this cause AV?  (Read 2268 times)

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Can this cause AV?
« on: March 01, 2024, 07:36:52 am »
I have a pretty big app which crashes sometimes, Im looking where is the bug and cant find it.

So I wonder if this might be the cause.

I have a class which has a method which frees the object (itself) and returns something.

Something like this:
Code: Pascal  [Select][+][-]
  1. type
  2.   tclass = class
  3.     somedata: string;
  4.     function add(s: string): tclass;
  5.     function finished: string;
  6.   end;
  7.  
  8. function tclass.add(s: string): tclass;
  9. begin
  10.   somedata += s;
  11.   result := self;
  12. end;
  13.  
  14. function tclass.finished: string;
  15. begin
  16.   result := somedata;
  17.   self.Free;
  18. end;
  19.  
  20. function things: tclass;
  21. begin
  22.   result := tclass.Create;
  23. end;
  24.  
  25. var
  26.   s: string;
  27.  
  28. begin
  29.   s := things.add('this').add('that').finished;
  30.   writeln(s);
  31.   readln;
  32. end.

Can this cause Access Violations?

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 10542
  • Debugger - SynEdit - and more
    • wiki
Re: Can this cause AV?
« Reply #1 on: March 01, 2024, 09:57:30 am »
In should be fine. The concept used in finish is ok.

Can you reproduce the crash at will, or does it happen very often (so you could do a few runs, and be sure it would happen)

1) Have you tried compiling without  optimization?

2) heaptrc -gh with env set HEAPTRC=keepreleased

3) If on Linux: run under valgrind --tool=memcheck

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Re: Can this cause AV?
« Reply #2 on: March 01, 2024, 10:18:52 am »
Thanks for confirming thats not it.

The code is pretty big, crashes happen on random, on different machines (not on my dev machine unfortunately, also not on virtual machines), hard to debug. Heaptrc tells something is freed TWICE (was released at -> freed again at), unfortunately I cant see which line is that, stack trace is just addresses. Optimizations on or off makes no difference.

In other apps stack trace contains some info about where it is, but not in this particular app. Just addresses, not much.

But yes, I can force it to crash by forcing it to do much work, so I keep digging.
« Last Edit: March 01, 2024, 10:21:15 am by Fibonacci »

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 10542
  • Debugger - SynEdit - and more
    • wiki
Re: Can this cause AV?
« Reply #3 on: March 01, 2024, 10:32:05 am »
You should try valgrind. If heaptrc can detect it, valgrind may find more. Valgrind keeps track of every alloc and free. And if any of them goes wrong (double), even if there is no crash, it will tell you. And it tells you the location of the alloc and the free.

For best results, you may need to recompile packages with debug info, if they aren't already.

Only drawback:
- valgrind is Linux only
- get a big coffee, it's gonna take a while. Under valgrind your app runs at a tiny fraction of its normal speed.

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11930
  • FPC developer.
Re: Can this cause AV?
« Reply #4 on: March 01, 2024, 10:36:41 am »
Tclass is a predefined type, so maybe that goes wrong here and there?

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Re: Can this cause AV?
« Reply #5 on: March 01, 2024, 10:38:48 am »
Tclass is a predefined type, so maybe that goes wrong here and there?

Nah, the class has a different name :D I named it tclass just for this example code

Thaddy

  • Hero Member
  • *****
  • Posts: 16139
  • Censorship about opinions does not belong here.
Re: Can this cause AV?
« Reply #6 on: March 01, 2024, 11:29:44 am »
Yes that can easily lead to stale pointers, so after self.free set self to nil.
Or call FreeAndNil(self). In this case I will allow it.

BTW TClass is a built-in so as Marco wrote it can get you into trouble.
TClass is a reference to TObject. Not a class.
« Last Edit: March 01, 2024, 12:10:20 pm by Thaddy »
If I smell bad code it usually is bad code and that includes my own code.

BrunoK

  • Hero Member
  • *****
  • Posts: 623
  • Retired programmer
Re: Can this cause AV?
« Reply #7 on: March 01, 2024, 12:46:23 pm »
But yes, I can force it to crash by forcing it to do much work, so I keep digging.
In debug mode, and with a bit of luck, adding a free counter to the class and code in the .finished method might help you figure out from where an eventual duplicate free is called.
Code: Pascal  [Select][+][-]
  1. uses
  2.   sysutils;
  3.  
  4. type
  5.   TTestClass = class
  6.     dbgDblFree: integer;
  7.  
  8.     somedata: string;
  9.     function add(s: string): TTestClass;
  10.     function finished: string;
  11.   end;
  12.  
  13. function TTestClass.add(s: string): TTestClass;
  14. begin
  15.   somedata += s;
  16.   result := self;
  17. end;
  18.  
  19. function TTestClass.finished: string;
  20. begin
  21.   result := somedata;
  22.   if dbgDblFree < 0 then
  23.     Raise Exception.Create('Double free'); // <- break point here
  24.   Dec(dbgDblFree);  // Double free detection
  25.   self.Free;
  26. end;
  27.  
  28. function things: TTestClass;
  29. begin
  30.   result := TTestClass.Create;
  31. end;
  32.  
  33. var
  34.   s: string;
  35.  
  36. begin
  37.   s := things.add('this').add('that').finished;
  38.   writeln(s);
  39.   readln;
  40. end.

More complicated would be to link with a debug version of FPC.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 10542
  • Debugger - SynEdit - and more
    • wiki
Re: Can this cause AV?
« Reply #8 on: March 01, 2024, 01:03:34 pm »
Yes that can easily lead to stale pointers, so after self.free set self to nil.
Why? "Self := nil" would not set the callers variable to nil. And the local variable "Self" itself is definitely not used after that (part of the concept is, that it freeing it is the last thing in the method.


In debug mode, and with a bit of luck, adding a free counter to the class and code in the .finished method might help you figure out from where an eventual duplicate free is called.
As long as he calls free after the "dec(dbgDblFree)" the field itself becomes trashed, and its value no longer exists. A 2nd call would not see it.

However, a test run without "free" (leaking all the objects) would make the counter work.

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Re: Can this cause AV?
« Reply #9 on: March 01, 2024, 01:05:21 pm »
Or call FreeAndNil(self). In this case I will allow it.

Thanks Thaddy!

BTW TClass is a built-in so as Marco wrote it can get you into trouble.
TClass is a reference to TObject. Not a class.

I already said its just for the sample code. I wont use it for sample codes anymore though ;)



@BrunoK: Thanks for the suggestion, but there is absolutely no chance to double call ".finished". I think the problem is much deeper. Its multithreaded app with many critical sections, at some point memory gets corrupted, sometimes earlier, sometimes a little later.

Thaddy

  • Hero Member
  • *****
  • Posts: 16139
  • Censorship about opinions does not belong here.
Re: Can this cause AV?
« Reply #10 on: March 01, 2024, 01:09:18 pm »
Why? "Self := nil" would not set the callers variable to nil. And the local variable "Self" itself is definitely not used after that (part of the concept is, that it freeing it is the last thing in the method.
No. Look at the implementation of freeandnil. That is for a reason.
If I smell bad code it usually is bad code and that includes my own code.

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Re: Can this cause AV?
« Reply #11 on: March 01, 2024, 01:12:32 pm »
No. Look at the implementation of freeandnil. That is for a reason.

"Self" is temporary local pointer to the class, what is the point of setting it to nil if its never used anymore? Isnt it like setting local Integer "i := 0" at the end of a function?
« Last Edit: March 01, 2024, 01:42:31 pm by Fibonacci »

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 10542
  • Debugger - SynEdit - and more
    • wiki
Re: Can this cause AV?
« Reply #12 on: March 01, 2024, 01:44:29 pm »
@BrunoK: Thanks for the suggestion, but there is absolutely no chance to double call ".finished". I think the problem is much deeper. Its multithreaded app with many critical sections, at some point memory gets corrupted, sometimes earlier, sometimes a little later.

valgrind also has 2 thread checkers (DRD, Helgrind). They do report a lot of false positives. But as a last resort...

Fibonacci

  • Hero Member
  • *****
  • Posts: 594
  • Internal Error Hunter
Re: Can this cause AV?
« Reply #13 on: March 01, 2024, 01:45:47 pm »
Its Windows only app %)

BrunoK

  • Hero Member
  • *****
  • Posts: 623
  • Retired programmer
Re: Can this cause AV?
« Reply #14 on: March 01, 2024, 01:52:11 pm »
EDIT : ignore this message, it is wrong !
EDIT : ignore this message, it is wrong !
EDIT : ignore this message, it is wrong !
In debug mode, and with a bit of luck, adding a free counter to the class and code in the .finished method might help you figure out from where an eventual duplicate free is called.
As long as he calls free after the "dec(dbgDblFree)" the field itself becomes trashed, and its value no longer exists. A 2nd call would not see it.
dbgDblFree trashed : not so much.

Slightly modified test code to show that :
Code: Pascal  [Select][+][-]
  1. program p;
  2. uses
  3.   SysUtils;
  4. type
  5.  
  6.   { TTestClass }
  7.  
  8.   TTestClass = class
  9.     somedata: string;
  10.     dbgDblFree: integer;
  11.     function add(s: string): TTestClass;
  12.     function finished: string;
  13.     destructor Destroy; override;
  14.   end;
  15.  
  16. var
  17.   vTestInst: TTestClass;
  18.   s1: string;
  19.  
  20.   function TTestClass.add(s: string): TTestClass;
  21.   begin
  22.     somedata += s;
  23.     Result := self;
  24.   end;
  25.  
  26.   function TTestClass.finished: string;
  27.   begin
  28.     s1 := HexStr(IntPtr(Self), SizeOf(IntPtr));
  29.     Result := somedata;
  30.     self.Free;
  31.   end;
  32.  
  33.   destructor TTestClass.Destroy;
  34.   begin
  35.     Dec(dbgDblFree);  // Double free detection
  36.     if dbgDblFree < 0 then
  37.       raise Exception.Create('Double free'); // <- break point here
  38.     inherited Destroy;
  39.   end;
  40.  
  41.   function things: TTestClass;
  42.   begin
  43.     Result := TTestClass.Create;
  44.     vTestInst := Result; // Store it to do a double free ~bk
  45.   end;
  46.  
  47. var
  48.   s: string;
  49. begin
  50.   s := things.add('this').add('that').finished;
  51.   writeln(s);
  52.   s := vTestInst.finished;
  53.   writeln(s);
  54.   readln;
  55. end.
win10, FPC 3.2.2
« Last Edit: March 01, 2024, 01:56:32 pm by BrunoK »

 

TinyPortal © 2005-2018