View Issue Details

IDProjectCategoryView StatusLast Update
0037915FPCDatabasepublic2020-10-19 09:14
ReporterPedro Gimeno Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Platformx86_64OSLinux 
Product Version3.3.1 
Summary0037915: Access Violation related to IndexDefs
DescriptionI was getting access violations in Lazarus when doing certain database component operations. I reported it as issue 0037870 and after bisection and further investigation, it became clear that the issue was introduced in FPC commit r38353 and that it isn't related to Lazarus. I was asked to report it in the FPC project, so here we go.

This problem is reproducible in any version starting at r38353. It is not reproducible in r38352 or earlier. Unfortunately, the diff of commit r38353 is about 1700 lines, and is quite complex.
Steps To Reproduce1. Create an sqlite database, e.g. sqlite3 /tmp/database.sqlite "CREATE TABLE T(F INTEGER);"
2. Run the following program:

{$mode objfpc}{$H+}

uses
  db, sqldb, sqlite3conn;

var
  Conn: TSQLite3Connection;
  Tran: TSQLTransaction;
  SQLQ: TSQLQuery;

begin
  Conn := TSQLite3Connection.Create(nil);
  Conn.Name := 'Conn';
  Conn.DatabaseName := '/tmp/database.sqlite';

  Tran := TSQLTransaction.Create(nil);
  Tran.Name := 'Tran';
  Tran.Database := Conn;

  SQLQ := TSQLQuery.Create(nil);
  SQLQ.Name := 'SQLQ';
  SQLQ.Database := Conn;
  SQLQ.SQL.Text := 'SELECT * FROM T';

  SQLQ.Open;

  SQLQ.IndexDefs.Add('IdxSomethingSomethingBlah', '', []);

  SQLQ.Close;  // crash happens here
end.


Output is:
An unhandled exception occurred at $00000000004E5012:
EAccessViolation: Access violation
  $00000000004E5012

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 

Activities

Pedro Gimeno

2020-10-14 18:26

reporter   ~0126301

Here's a simpler self-contained test case that doesn't need a connection, transaction or auxiliary file:
{$mode objfpc}{$H+}

uses
  db, BufDataset;

var
  BDS: TBufDataset;

begin
  BDS := TBufDataset.Create(nil);
  BDS.Name := 'BDS';
  BDS.FieldDefs.Add('F', ftInteger);
  BDS.CreateDataset();

  BDS.Open();

  BDS.IndexDefs.Add('x', '', []);

  BDS.Close(); // crashes here
end.

(ignore the warning about the abstract method).

jamie philbrook

2020-10-14 20:53

reporter   ~0126308

although I don't have fpc installed with debug info I did see something, I did follow the asm trail ..

the indexdefs member is returning a nil pointer but it does get created during the constructor of the TbufDataSet.create;
this pointer is the next step to accessing the member ADD so it must be the instance pointer.

 I believe its the OPEN that is causing it, maybe resetting it. I didn't get deep enough for that.

Pedro Gimeno

2020-10-14 22:21

reporter   ~0126313

Last edited: 2020-10-14 22:37

View 2 revisions

I've debugged the last test case. I presume that it's the same as the OP's.

When the crash happens, there are 3 indexes: two have been automatically created as the dataset was opened; the third is the result of adding an IndexDef.

This patch is pretty self-explanatory as to where the problem is:

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)
@@ -1449,6 +1449,7 @@
   i,r  : integer;
   iGetResult : TGetResult;
   pc : TRecordBuffer;
+  BufInd: TBufIndex;
 
 begin
   FOpen:=False;
@@ -1468,13 +1469,17 @@
         end;
 
   for r := 0 to FIndexes.Count-1 do
-    with FIndexes.BufIndexes[r] do
-      if IsInitialized then
-        begin
-        pc:=SpareRecord;
-        ReleaseSpareRecord;
-        FreeRecordBuffer(pc);
-        end;
+    begin
+    BufInd := FIndexes.BufIndexes[r];
+    if Assigned(BufInd) then
+      with BufInd do
+        if IsInitialized then
+          begin
+          pc:=SpareRecord;
+          ReleaseSpareRecord;
+          FreeRecordBuffer(pc);
+          end;
+    end;
 
   if Length(FUpdateBuffer) > 0 then
     begin


So, bottom line: when closing the dataset, there is a BufIndex that is nil when the corresponding index has been created by adding an IndexDef manually.

I don't know if the lack of a BufIndex in this case is expected behaviour, therefore I'm not proposing the above as a patch. I can tell that with the patch, the above test case no longer crashes; it finishes normally.

HOWEVER:

Even with the patch, and with the test case above working, the Lazarus IDE keeps crashing, therefore these two are separate issues, even if they were introduced in the same commit. I'll open a separate issue for the second one. EDIT: Actually, I'm still not sure if this is a Lazarus issue, so I won't open one; I'll use 0037870 if necessary.

Sven Barth

2020-10-14 22:25

manager   ~0126314

The documentation for TCustomBufDataset states that IndexDefs should be considered readonly ( https://www.freepascal.org/docs-html/fcl/bufdataset/tcustombufdataset.indexdefs.html ). Use TCustomBufDataset.AndIndex instead.

Pedro Gimeno

2020-10-15 11:34

reporter   ~0126323

> The documentation for TCustomBufDataset states that IndexDefs should be considered readonly

And the property is read-only: you can't set IndexDefs := something. But the documentation doesn't say that the returned object is read-only.

IndexDefs is a published property, hence exposed to the IDE, and currently, adding an index through the IDE crashes the IDE in turn when the dataset is closed. It didn't in versions previous to r38353.

If AddIndex is the way to add indices, could it be that IndexDefs.Add should be redirected to call AddIndex? Because that's the only interface in the IDE that users have to add or remove indices. Why else would IndexDefs be a published property?

Please consider the patch above at least; it's a very basic protection against nil which is beneficial in any case. The only problem I see with it, is that it may insinuate to people reading the code that it's normal for some buffers to be nil at that point, and I'm still not sure if that's the case.

LacaK

2020-10-15 14:50

developer   ~0126334

> The only problem I see with it, is that it may insinuate to people reading the code that it's normal for some buffers to be nil at that point

Exactly. It should be investigated deeper what is happening there.

Sven Barth

2020-10-17 13:52

manager   ~0126367

> And the property is read-only: you can't set IndexDefs := something. But the documentation doesn't say that the returned object is read-only.

Then the documentation needs clarification, because that's what the documentation wants to express. There are no other properties without a setter that casually remark that it's readonly.

> If AddIndex is the way to add indices, could it be that IndexDefs.Add should be redirected to call AddIndex?

No, because the internal code of the dataset needs to be able to call the Add method to really add a new index object.

> Because that's the only interface in the IDE that users have to add or remove indices. Why else would IndexDefs be a published property?

The dataset code correctly handles the case of the streaming system setting up the index defs. That's simply different from adding an index at runtime.

@LacaK: There is no need for investigation, because adding an index using IndexDefs.Add by itself does not set up the added object in a way that the TBufDataset can use it correctly, mainly by not setting up BufferIndex.

Pedro Gimeno

2020-10-17 16:05

reporter   ~0126370

Last edited: 2020-10-17 16:12

View 2 revisions

>> If AddIndex is the way to add indices, could it be that IndexDefs.Add should be redirected to call AddIndex?

> No, because the internal code of the dataset needs to be able to call the Add method to really add a new index object.

In that case, there's something broken in the whole concept of IndexDefs as used by the r38353 patch. It's a published property, and as such, it is exposed to the IDE, and within the IDE, it's possible to add and delete IndexDefs after setting Active (another published property) to True. From the point of view of the IDE, that's not run time, that's design time. And currently, if the IDE user adds an IndexDef in design mode with the Active property set to True, and then sets Active to False, that causes an error window to appear: 'Access violation with value "(False)"' (which is how I found this issue and could replicate it in code), and the rest of InternalClose from that point on is not executed (edit: potentially causing leaks).

If the internal code needs to be able to rely on the Add method to add index definitions, maybe the exposed IndexDefs should be a sort of proxy object instead of exposing the internals, such that when an IndexDef is added, it does the right thing regardless of whether the dataset is open or not.

Issue History

Date Modified Username Field Change
2020-10-12 22:34 Pedro Gimeno New Issue
2020-10-13 07:18 LacaK Relationship added related to 0037870
2020-10-14 18:26 Pedro Gimeno Note Added: 0126301
2020-10-14 20:53 jamie philbrook Note Added: 0126308
2020-10-14 22:21 Pedro Gimeno Note Added: 0126313
2020-10-14 22:25 Sven Barth Note Added: 0126314
2020-10-14 22:37 Pedro Gimeno Note Edited: 0126313 View Revisions
2020-10-15 11:34 Pedro Gimeno Note Added: 0126323
2020-10-15 14:50 LacaK Note Added: 0126334
2020-10-17 13:52 Sven Barth Note Added: 0126367
2020-10-17 16:05 Pedro Gimeno Note Added: 0126370
2020-10-17 16:12 Pedro Gimeno Note Edited: 0126370 View Revisions