Recent

Author Topic: Class Destruction: I Discovered The Solution But What Was The Problem?  (Read 2794 times)

Bazzao

  • Full Member
  • ***
  • Posts: 178
  • Pies are squared.
I haven't created a class since my Dephi days 12 years ago.

So, just for the exercise, I thought I'd create a TRect variant from scratch and not using TRect as a ParentClass.

My Trect (TMyTrect) has anchor points (None, the 4 corners, and Centre).

All coded and a tester to test the correctness of the code within the class (280000 tests).

Then I thought Pens & Brushes are always associated with Rects, so I add a creation of them within MyRect.

Assigning a colour to both proved they were created OK, and destruction worked.

All good. All working.

I then decided to try out TMyRect in a form.

Code: Pascal  [Select][+][-]
  1. MyRect.Brush:=Form1.Canvas.Brush;
  2. MyRect.Pen:=Form1.Canvas.Pen;

Both worked OK, but when Form1 was closed (deploying the Destructor) an exception error occured.

A search provided no clue.

By chance I included in Form1.Close the following:

Code: Pascal  [Select][+][-]
  1. MyRect.Brush:=nil;
  2. MyRect.Pen:=nil;

That prevented the error on Close, so I moved that code to the Destructor:

Code: Pascal  [Select][+][-]
  1. destructor TMyRect.Free;
  2. begin
  3.   Brush:=nil;
  4.   Pen:=nil;
  5.   Brush.Free;
  6.   Pen.Free;
  7. end;

... and that worked.

So I've found the solution, but still do not understand the problem.

Without too heavy an explanation, can someone please tell me the problem?

Thanks,

Bazza
Bazza

Lazarus 2.0.10; FPC 3.2.0; SVN Revision 63526; x86_64-win64-win32/win64
Windows 10.

Thaddy

  • Hero Member
  • *****
  • Posts: 14169
  • Probably until I exterminate Putin.
No that doesn't work!
Why: activate heaptrc and look at the results! You have memory leaks. See https://wiki.freepascal.org/heaptrc
In this case, reluctantly because it is a bad habit, use FreeAndNil.

Reason: you can call free on a nil pointer, but you should only call free on an assigned pointer. You should call free first and then set the pointer to nil.
Compiling with heaptrc will reveal that immediately. It also shows (see my comment above) that there is something else wrong in the code, which is not necessarily your code, it can be in the LCL itself.

What FreeAndNil does is actually preventing use-after-free, it is a bandage, not a cure..... But it works in your case.

The importance is explained here: https://www.purehacking.com/blog/lloyd-simon/an-introduction-to-use-after-free-vulnerabilities

FreeAndNil uses an intermediate local variable assignment to do everything in the right order, but, again, there must be something wrong if you actually need it.
« Last Edit: July 10, 2019, 09:08:59 am by Thaddy »
Specialize a type, not a var.

PascalDragon

  • Hero Member
  • *****
  • Posts: 5444
  • Compiler Developer
@Thaddy: how about explaining Bazzao the real problem they have instead of some completely unrelated stuff?

I then decided to try out TMyRect in a form.

Code: Pascal  [Select][+][-]
  1. MyRect.Brush:=Form1.Canvas.Brush;
  2. MyRect.Pen:=Form1.Canvas.Pen;
When doing an assignment of class instances you only copy a reference to the class instance. The form (or more correctly the canvas) still has that reference and as the canvas was the one that created the pen and brush it's also going to free it. So when you free it as well in the destructor of TMyRect you essentially free it twice (it doesn't matter if you or the canvas frees the two first, because one of the two will free an already freed object).

Now if you do this:

By chance I included in Form1.Close the following:

Code: Pascal  [Select][+][-]
  1. MyRect.Brush:=nil;
  2. MyRect.Pen:=nil;

That prevented the error on Close, so I moved that code to the Destructor:

Code: Pascal  [Select][+][-]
  1. destructor TMyRect.Free;
  2. begin
  3.   Brush:=nil;
  4.   Pen:=nil;
  5.   Brush.Free;
  6.   Pen.Free;
  7. end;

... and that worked.
You release your reference to the class instance. And calling Free on a Nil class reference is a no-op.

As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):

Code: Pascal  [Select][+][-]
  1. constructor TMyRect.Create;
  2. begin
  3.   Brush := TBrush.Create(Nil);
  4.   Pen := TPen.Create(Nil);
  5. end;

And where you currently assign the references:
Code: Pascal  [Select][+][-]
  1. MyRect.Brush.Assign(Form1.Canvas.Brush);
  2. MyRect.Pen.Assign(Form1.Canvas.Pen);

Alternatively you accept that your TMyRect merely has copies of the references and don't free them. You could also use a property setter and check whether the owner of the Brush and Pen instances is already set to something and then set a Boolean (for each of the two) that does stop the destructor from calling Free in that case.

In any case you don't need the assignments to Nil anymore.

Thaddy

  • Hero Member
  • *****
  • Posts: 14169
  • Probably until I exterminate Putin.
@Thaddy: how about explaining Bazzao the real problem they have instead of some completely unrelated stuff?
Well, you just explained it is not unrelated stuff  8-) You are much better in explaining, but at least I gave it a try... :'(
Specialize a type, not a var.

PascalDragon

  • Hero Member
  • *****
  • Posts: 5444
  • Compiler Developer
Yes, it is unrelated, cause as you might notice I didn't mention FreeAndNil at all. In fact more often than not that procedure is simply overused. One needs to learn when it is useful to use.

devEric69

  • Hero Member
  • *****
  • Posts: 648
For a ***newbie*** (or someone who is slowly getting back with Pascal), with a view to pragmatic progress, I really think that the best thing - in order to stay as far away as possible "pointer's Access Violations" - is to always to start to code with, IMHO:

Code: Pascal  [Select][+][-]
  1. if Assigned(oMyObject) then
  2.   FreeAndNil(oMyObject);

Reminder: the "Assign();" method, is not implemented everywhere, so newbies will wonder why it is not implemented with this TClass? Answer: it is up to you, the developer, to implement it... probably when you'll be more comfortable with pointer handling.


For information, there is a small memento on the destruction of objects and concerning memory management here: https://forum.lazarus.freepascal.org/index.php/topic,45667.msg323422.html#msg323422 .
use: Linux 64 bits (Ubuntu 20.04 LTS).
Lazarus version: 2.0.4 (svn revision: 62502M) compiled with fpc 3.0.4 - fpDebug \ Dwarf3.

lucamar

  • Hero Member
  • *****
  • Posts: 4219
Code: Pascal  [Select][+][-]
  1. if Assigned(oMyObject) then
  2.   FreeAndNil(oMyObject);

Sorry but that is redundant. Both Free and FreeAndNIl already test first whether the the object is assigned. That's why they are "safe".

One may want to do it to avoid the subsequent call but nothing else.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus/FPC 2.0.8/3.0.4 & 2.0.12/3.2.0 - 32/64 bits on:
(K|L|X)Ubuntu 12..18, Windows XP, 7, 10 and various DOSes.

Akira1364

  • Hero Member
  • *****
  • Posts: 561
Uh, why has no one pointed out how wrong their destructor implementation is?

Code: Pascal  [Select][+][-]
  1. destructor TMyRect.Free;
  2. begin
  3.   Brush:=nil;
  4.   Pen:=nil;
  5.   Brush.Free;
  6.   Pen.Free;
  7. end;

Do not ever call your destructor "Free". Free is an existing procedure, that itself will properly call any available destructor for the class.

Also, setting any class to "nil" and then calling "Free" means the call to "Free" is useless, because the implementation of "Free" looks like this:

Code: Pascal  [Select][+][-]
  1. procedure TObject.Free;
  2.  
  3.   begin
  4.      // the call via self avoids a warning
  5.      if self<>nil then
  6.        self.destroy;
  7.   end;

Always call your destructors "Destroy", and be sure to add the "override" modifier in their declaration. So something like this, overall:

Code: Pascal  [Select][+][-]
  1. type
  2.   TMyRect = class
  3.   public
  4.     destructor Destroy; override;
  5.   end;
  6.  
  7.   destructor TMyRect.Destroy;
  8.   begin
  9.     Brush.Free;
  10.     Pen.Free;
  11.     inherited Destroy;    
  12.   end;

then you can write code like the following, and it will work as you expect, without memory leaks:

Code: Pascal  [Select][+][-]
  1. program Example;
  2.  
  3. uses UnitWhereTMyRectIsDeclared;
  4.  
  5. var R: TMyRect;
  6.  
  7. begin
  8.   R := TMyRect.Create;
  9.   // Do stuff...
  10.   R.Free;
  11. end.

« Last Edit: July 10, 2019, 03:16:06 pm by Akira1364 »

Thaddy

  • Hero Member
  • *****
  • Posts: 14169
  • Probably until I exterminate Putin.
Yes, it is unrelated, cause as you might notice I didn't mention FreeAndNil at all. In fact more often than not that procedure is simply overused. One needs to learn when it is useful to use.
Then again, I explained what the issue was.... It is not often I disagree with you. But if you say so..., OK?
Specialize a type, not a var.

marcov

  • Administrator
  • Hero Member
  • *
  • Posts: 11353
  • FPC developer.
1. form1 is owner of pen, and destroys it.
2. you assigned pen to tmyrect and free it.

-> it gets destroyed twice -> exception

now you NIL it so the tmyrect.free doesn't do anything, but tmyrect simply shouldn't free things that the form still owns..

Akira1364

  • Hero Member
  • *****
  • Posts: 561
Sorry but that is redundant. Both Free and FreeAndNIl already test first whether the the object is assigned. That's why they are "safe".

One may want to do it to avoid the subsequent call but nothing else.

I've never understood why people seem to write that kind of thing so often, as it is indeed redundant like you said.

Even worse though as far as redunancy is this, which I see all the time:

Code: Pascal  [Select][+][-]
  1. if SomeVariable is TSomeClass then
  2.   (SomeVariable as TSomeClass).DoSomething;

The checked "as" cast there is completely pointless, as you have already checked the validity of the cast by using "is", and furthermore "as" adds performance overhead that can very quickly "add up" compared to a hard-cast which adds none at all.

In scenarios like that, always write this instead, people:

Code: Pascal  [Select][+][-]
  1. if SomeVariable is TSomeClass then
  2.   TSomeClass(SomeVariable).DoSomething;
« Last Edit: July 10, 2019, 07:26:38 pm by Akira1364 »

PascalDragon

  • Hero Member
  • *****
  • Posts: 5444
  • Compiler Developer
Uh, why has no one pointed out how wrong their destructor implementation is?

Code: Pascal  [Select][+][-]
  1. destructor TMyRect.Free;
  2. begin
  3.   Brush:=nil;
  4.   Pen:=nil;
  5.   Brush.Free;
  6.   Pen.Free;
  7. end;

Do not ever call your destructor "Free". Free is an existing procedure, that itself will properly call any available destructor for the class.

Also, setting any class to "nil" and then calling "Free" means the call to "Free" is useless, because the implementation of "Free" looks like this:
Did you read their whole message? They wrote that setting Brush and Pen to Nil solves their problem which is because those two were copied references from the form's canvas. So they fixed a symptom and I tried to explain the real cause of their problem.
(You are right though that the destructor should be called Destroy, I didn't spot that)

1. form1 is owner of pen, and destroys it.
2. you assigned pen to tmyrect and free it.

-> it gets destroyed twice -> exception

now you NIL it so the tmyrect.free doesn't do anything, but tmyrect simply shouldn't free things that the form still owns..
That's what I wrote in my post as well.

Bazzao

  • Full Member
  • ***
  • Posts: 178
  • Pies are squared.
@Thaddy: how about explaining Bazzao the real problem they have instead of some completely unrelated stuff?

When doing an assignment of class instances you only copy a reference to the class instance. The form (or more correctly the canvas) still has that reference and as the canvas was the one that created the pen and brush it's also going to free it. So when you free it as well in the destructor of TMyRect you essentially free it twice (it doesn't matter if you or the canvas frees the two first, because one of the two will free an already freed object).

You release your reference to the class instance. And calling Free on a Nil class reference is a no-op.

As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):

Code: Pascal  [Select][+][-]
  1. constructor TMyRect.Create;
  2. begin
  3.   Brush := TBrush.Create(Nil);
  4.   Pen := TPen.Create(Nil);
  5. end;

And where you currently assign the references:
Code: Pascal  [Select][+][-]
  1. MyRect.Brush.Assign(Form1.Canvas.Brush);
  2. MyRect.Pen.Assign(Form1.Canvas.Pen);

Alternatively you accept that your TMyRect merely has copies of the references and don't free them. You could also use a property setter and check whether the owner of the Brush and Pen instances is already set to something and then set a Boolean (for each of the two) that does stop the destructor from calling Free in that case.

In any case you don't need the assignments to Nil anymore.

Yes, Thaddy sometimes goes off on a tangent, but he does give valuable information.

Now, PascalDragon, your reply I understood, which was the purpose of this thread. I shall give your untested ideas a firm go.

Bazza


Bazza

Lazarus 2.0.10; FPC 3.2.0; SVN Revision 63526; x86_64-win64-win32/win64
Windows 10.

Bazzao

  • Full Member
  • ***
  • Posts: 178
  • Pies are squared.

As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):

Code: Pascal  [Select][+][-]
  1. constructor TMyRect.Create;
  2. begin
  3.     Brush := TBrush.Create(Nil);
  4.     Pen := TPen.Create(Nil);
  5. end;

Nope. (Nil) not liked.
Quote
Error: Wrong number of parameters specified for call to "Create"

The following code, in conjunction with the Assign(Form1.Canvas.Brush) etc inside the unit, works, I even threw in a Font for good measure ...
Code: Pascal  [Select][+][-]
  1. public
  2.     Brush        :TBrush;
  3.     Font         :TFont;
  4.     Pen          :TPen;
  5.     constructor Create; virtual;
  6.     destructor Destroy; override;
  7.  
  8. destructor TMyRect.Destroy;
  9. begin
  10.   Brush.Free;
  11.   Font.Free;
  12.   Pen.Free;
  13.   inherited Destroy;
  14. end;
  15.  
  16. constructor TMyRect.Create;
  17. begin
  18.   Brush:=TBrush.Create; // (Nil) is not an option.
  19.   Font:=TFont.Create; // (Nil) is not an option.
  20.   Pen:=TPen.Create; // (Nil) is not an option.
  21.   Clear;
  22. end;
  23.  

All good. Many thanks to all.

B
Bazza

Lazarus 2.0.10; FPC 3.2.0; SVN Revision 63526; x86_64-win64-win32/win64
Windows 10.

PascalDragon

  • Hero Member
  • *****
  • Posts: 5444
  • Compiler Developer

As a real solution you either need to create copies of the brush and pen, e.g. something like this (not tested):

Code: Pascal  [Select][+][-]
  1. constructor TMyRect.Create;
  2. begin
  3.     Brush := TBrush.Create(Nil);
  4.     Pen := TPen.Create(Nil);
  5. end;

Nope. (Nil) not liked.
Quote
Error: Wrong number of parameters specified for call to "Create"
I didn't remember whether these types have an Owner or not. Apparently they don't  :-[

 

TinyPortal © 2005-2018