View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0037932 | FPC | Database | public | 2020-10-16 04:22 | 2020-10-25 09:48 |
| Reporter | Pedro Gimeno | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | new | Resolution | open | ||
| Product Version | 3.3.1 | ||||
| Summary | 0037932: Dangling pointer left behind when closing a TBufDataset | ||||
| Description | I 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 Reproduce | I 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 Information | I've tried to respect the style of the surrounding code for consistency, despite abhorring it. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | |||||
| FPCOldBugId | |||||
| FPCTarget | |||||
| Attached Files |
| ||||
| related to | 0037870 | resolved | Juha Manninen | Lazarus | Access Violation after closing a dataset in the IDE |
|
|
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;
|
|
|
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. |
|
|
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? |
|
|
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. |
|
|
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. |
|
|
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
---------------------------------------------------------------------}
|
|
|
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
---------------------------------------------------------------------}
|
| 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 |
