Lazarus

Programming => General => Topic started by: Bazzao on July 10, 2019, 05:48:39 am

Title: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Bazzao on July 10, 2019, 05:48:39 am
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
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Thaddy on July 10, 2019, 08:48:59 am
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.
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: PascalDragon on July 10, 2019, 09:21:16 am
@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.
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Thaddy on July 10, 2019, 09:28:44 am
@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... :'(
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: PascalDragon on July 10, 2019, 09:35:18 am
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.
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: devEric69 on July 10, 2019, 12:50:49 pm
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 (https://forum.lazarus.freepascal.org/index.php/topic,45667.msg323422.html#msg323422) .
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: lucamar on July 10, 2019, 02:22:43 pm
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.
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Akira1364 on July 10, 2019, 03:09:44 pm
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.

Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Thaddy on July 10, 2019, 04:26:19 pm
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?
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: marcov on July 10, 2019, 04:27:38 pm
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..
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Akira1364 on July 10, 2019, 07:24:41 pm
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;
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: PascalDragon on July 11, 2019, 11:26:13 am
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.
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Bazzao on July 12, 2019, 06:01:57 am
@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


Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Bazzao on July 12, 2019, 09:06:10 am

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
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: PascalDragon on July 12, 2019, 09:08:14 am

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  :-[
Title: Re: Class Destruction: I Discovered The Solution But What Was The Problem?
Post by: Bazzao on July 12, 2019, 09:42:59 am
I always store snippets of code in a tree-note type text file for future reference. Defaulting .pas files to Notepad2 means I can easily troll through old Delphi code (where I got the destructor name wrong).  :D

Bazza