All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0037932FPCDatabasepublic2020-10-16 04:222021-01-14 16:55
ReporterPedro Gimeno Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status assignedResolutionopen 
Product Version3.3.1 
Summary0037932: Dangling pointer left behind when closing a TBufDataset
DescriptionI believe I've finally nailed the cause of 0037870. It's a dangling pointer.

The patch pretty much speaks for itself, but basically: FCurrentIndexBuf was not cleared when freeing the index that it was caching, leaving it as a dangling pointer, which could cause all sorts of errors when accessing the IndexFieldNames property in particular with the dataset closed, because it uses FCurrentIndexBuf internally. This causes a random crash in Lazarus when the dataset is opened and then closed in design mode.

With the patch, some of the problems reported in 37870 with TCSVDataset and TSQLDataset are no longer reproducible, nor are other tests I made with TBufDataset.
Steps To ReproduceI couldn't come up with a reproduction recipe, other than by using Lazarus. See 0037870 for details. Hopefully the patch is sufficiently clear as to what the problem is.

But you can easily patch the same area with some code that verifies that FCurrentIndexDef equals BufIndexDefs[i] sometimes, to verify that there's indeed a problem.
Additional InformationI've tried to respect the style of the surrounding code for consistency, despite abhorring it.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Relationships

related to 0037870 resolvedJuha Manninen Lazarus Access Violation after closing a dataset in the IDE 
related to 0038352 new Packages CSVDataset failure 

Activities

Pedro Gimeno

2020-10-16 04:22

reporter  

patch-dangling-pointer.diff (687 bytes)    
Index: packages/fcl-db/src/base/bufdataset.pas
===================================================================
--- packages/fcl-db/src/base/bufdataset.pas	(revision 47066)
+++ packages/fcl-db/src/base/bufdataset.pas	(working copy)
@@ -1499,7 +1504,11 @@
   if assigned(FParser) then FreeAndNil(FParser);
   For I:=FIndexes.Count-1 downto 0 do
     if (BufIndexDefs[i].IndexType in [itDefault,itCustom]) or (BufIndexDefs[i].DiscardOnClose) then
-       BufIndexDefs[i].Free
+       begin
+       if (FCurrentIndexDef = BufIndexDefs[i]) then
+         FCurrentIndexDef := nil;
+       BufIndexDefs[i].Free;
+       end
     else
        FreeAndNil(BufIndexDefs[i].FBufferIndex);
 end;
patch-dangling-pointer.diff (687 bytes)   

Pedro Gimeno

2020-10-16 04:32

reporter   ~0126340

Last edited: 2020-10-25 02:29

View 3 revisions

 I can't edit, but where I wrote FCurrentIndexBuf I meant FCurrentIndexDef.

ETA: I should have noted, 0037870 is a bit confusing because there are three independent crashes caused by the same revision.

- The access violation related to adding an IndexDef to IndexDefs is reported as 0037915.
- The access violation related to GetIndexFieldNames after closing the dataset is the subject of this report, and the main subject of 0037870.
- There is an additional access violation when removing a FieldDef from a TSQLQuery with the dataset open, and then closing it, which is not yet reported, but the large GDB backtrace in 0037870 corresponds to it.

At the time I didn't know these crashes were completely independent. Now that I've patched two of them, I've been able to check again and see the third in isolation.

ETA2: The 3 crashes are now reported as 0037915, 0037932 (this one) and 0037981.

LacaK

2020-10-16 07:23

developer   ~0126341

Why is there test: if (FCurrentIndexDef = BufIndexDefs[i]) then ?
I would say, that in InternalClose we can always set FCurrentIndexDef := nil;
Does it give sense to have FCurrentIndexDef<>nil when dataset is closed?

Pedro Gimeno

2020-10-16 12:32

reporter   ~0126343

When the index type of an IndexDef is itNormal, it isn't freed, it is kept. If FCurrentIndexDef points to it, I don't see a reason to clean it up.

Pedro Gimeno

2020-10-16 12:51

reporter   ~0126344

Actually, disregard the patch. That's not the proper fix, as it could be left dangling in case user-created indexes are removed by the user.

Instead, a notification needs to be added to the IndexDefs so that this check can be done at the time the removal is happening. I'm working on a new patch.

Pedro Gimeno

2020-10-16 15:11

reporter   ~0126346

This patch correctly invalidates FCurrentIndexDef when it matches the index removed from IndexDefs. It also fixes TCSVDataset, TBufDataset and TSQLQuery like the previous one did.
patch-37932-v2.diff (1,175 bytes)    
Index: packages/fcl-db/src/base/bufdataset.pas
===================================================================
--- packages/fcl-db/src/base/bufdataset.pas	(revision 47066)
+++ packages/fcl-db/src/base/bufdataset.pas	(working copy)
@@ -478,6 +478,9 @@
       private
         function GetBufDatasetIndex(AIndex : Integer): TBufDatasetIndex;
         function GetBufferIndex(AIndex : Integer): TBufIndex;
+      protected
+        procedure Notify(Item: TCollectionItem;Action: TCollectionNotification);
+          override;
       Public
         Constructor Create(aDataset : TDataset); override;
         // Does not raise an exception if not found.
@@ -976,6 +979,14 @@
     Result:=Nil;
 end;
 
+procedure TCustomBufDataset.TBufDatasetIndexDefs.Notify(Item: TCollectionItem;
+  Action: TCollectionNotification);
+begin
+  if (Action in [cnDeleting, cnExtracting])
+      and (Item = TCustomBufDataset(Dataset).FCurrentIndexDef) then
+    TCustomBufDataset(Dataset).FCurrentIndexDef := nil;
+end;
+
 { ---------------------------------------------------------------------
     TCustomBufDataset
   ---------------------------------------------------------------------}
patch-37932-v2.diff (1,175 bytes)   

Pedro Gimeno

2020-10-17 19:07

reporter   ~0126374

Sorry, the previous patch does not call the inherited notify. When I checked how bad that was, I realized that it was not calling the observers, and that adding an observer was probably cleaner, therefore I did just that.

Version 3 attached.
patch-37932-v3.diff (1,670 bytes)    
Index: packages/fcl-db/src/base/bufdataset.pas
===================================================================
--- packages/fcl-db/src/base/bufdataset.pas	(revision 47066)
+++ packages/fcl-db/src/base/bufdataset.pas	(working copy)
@@ -474,10 +474,12 @@
       end;
 
       { TBufDatasetIndexDefs }
-      TBufDatasetIndexDefs = Class(TIndexDefs)
+      TBufDatasetIndexDefs = Class(TIndexDefs, IFPObserver)
       private
         function GetBufDatasetIndex(AIndex : Integer): TBufDatasetIndex;
         function GetBufferIndex(AIndex : Integer): TBufIndex;
+        procedure FPOObservedChanged(ASender: TObject; Operation:
+          TFPObservedOperation; Data: Pointer);
       Public
         Constructor Create(aDataset : TDataset); override;
         // Does not raise an exception if not found.
@@ -961,6 +963,7 @@
 constructor TCustomBufDataset.TBufDatasetIndexDefs.Create(aDataset: TDataset);
 begin
   inherited Create(aDataset,aDataset,TBufDatasetIndex);
+  FPOAttachObserver(Self);
 end;
 
 function TCustomBufDataset.TBufDatasetIndexDefs.FindIndex(const IndexName: string): TBufDatasetIndex;
@@ -976,6 +979,14 @@
     Result:=Nil;
 end;
 
+procedure TCustomBufDataset.TBufDatasetIndexDefs.FPOObservedChanged(ASender:
+  TObject; Operation: TFPObservedOperation; Data: Pointer);
+begin
+  if (Operation = ooDeleteItem)
+      and (Data = Pointer(TCustomBufDataset(Dataset).FCurrentIndexDef)) then
+    TCustomBufDataset(TIndexDefs(ASender).Dataset).FCurrentIndexDef := nil;
+end;
+
 { ---------------------------------------------------------------------
     TCustomBufDataset
   ---------------------------------------------------------------------}
patch-37932-v3.diff (1,670 bytes)   

Issue History

Date Modified Username Field Change
2020-10-16 04:22 Pedro Gimeno New Issue
2020-10-16 04:22 Pedro Gimeno File Added: patch-dangling-pointer.diff
2020-10-16 04:32 Pedro Gimeno Note Added: 0126340
2020-10-16 04:49 Pedro Gimeno Note Edited: 0126340 View Revisions
2020-10-16 07:23 LacaK Note Added: 0126341
2020-10-16 12:32 Pedro Gimeno Note Added: 0126343
2020-10-16 12:51 Pedro Gimeno Note Added: 0126344
2020-10-16 15:11 Pedro Gimeno Note Added: 0126346
2020-10-16 15:11 Pedro Gimeno File Added: patch-37932-v2.diff
2020-10-17 19:07 Pedro Gimeno Note Added: 0126374
2020-10-17 19:07 Pedro Gimeno File Added: patch-37932-v3.diff
2020-10-25 02:29 Pedro Gimeno Note Edited: 0126340 View Revisions
2020-10-25 09:48 Juha Manninen Relationship added related to 0037870
2021-01-14 15:46 Juha Manninen Relationship added related to 0038352
2021-01-14 16:55 Michael Van Canneyt Assigned To => Michael Van Canneyt
2021-01-14 16:55 Michael Van Canneyt Status new => assigned