View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0037915 | FPC | Database | public | 2020-10-12 22:34 | 2021-01-14 16:55 |
| Reporter | Pedro Gimeno | Assigned To | Michael Van Canneyt | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | assigned | Resolution | open | ||
| Platform | x86_64 | OS | Linux | ||
| Product Version | 3.3.1 | ||||
| Summary | 0037915: Access Violation related to IndexDefs | ||||
| Description | I 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 Reproduce | 1. 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 | ||||
| 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 |
| related to | 0038352 | new | Packages | CSVDataset failure |
|
|
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). |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
> 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. |
|
|
> 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. |
|
|
> 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. |
|
|
>> 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. |
| 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 |
| 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 |
