potential critical crash in ZDbcCachedResultSet.pas

[serbod]

In methods:
TZAbstractCachedResultSet.InsertRow
TZAbstractCachedResultSet.UpdateRow
TZAbstractCachedResultSet.DeleteRow

in "except" block used very unsafe code, that cause crash in some time:

Code: Select all

FRowsList[RowNo - 1] := FInitialRowsList[FInitialRowsList.Count - 1];

if FInitialRowsList.Count returns 0 then will be exception 'List index (-1) out of range'.

[marsupilami]

Hello,

I created a ticket on our bug tracker: https://sourceforge.net/p/zeoslib/tickets/407/
This ensures it will not be forgeotten. Thank you for reporting the bug to us.

Best regards,

Jan

[marsupilami]

Hello serbod,

do you have an example where FInitialRowsList is empty and that code can be reached?
Best regards,

Jan

[serbod]

do you have an example where FInitialRowsList is empty and that code can be reached?

It's from logs on production server without debugging, occuring sometimes. Can't reproduce.

[serbod]

Due to security restriction, I do not have access to that server.
Screenshot from log file (will be discarded in 30 days):

[aehimself]

Without a reproduceable test case, a screenshot of a stack trace is useless, unfortunately. It can be caused by data corruption in the database itself, a really special if-all-stars-are-aligned scenario or simply a unique data structure.

Devs need to know what parameters were passed in those stack traces to find the root cause. And yeah, sometimes it might contain sensitive information.

I guess you know the location where the exception is raised. What I would do it to wrap that block around in a Try ... Except block and then log as much information as I can to identify the situation before re-raising the exception. Dump record IDs, internal component states and everything else what you can think of. Hell, save it to a separate log file for easier review.
Usually, in about 5-10 occurrences you'll recognize a pattern to be able to make a test case.

[serbod]

I guess you know the location where the exception is raised. What I would do it to wrap that block around in a Try ... Except block and then log as much information as I can to identify the situation before re-raising the exception. Dump record IDs, internal component states and everything else what you can think of. Hell, save it to a separate log file for easier review.
Usually, in about 5-10 occurrences you'll recognize a pattern to be able to make a test case.

It's production server in restricted area, I don't have any access. And can't update executable files, they must be tested and approved. It takes couple of months as minimum.

On developer server it not reproduced so far.

Next release will dump all SQL requests to logs and more detailed stacktrace.

[serbod]

After adding simple check to prevent 'out of range' exception

Code: Select all

if (FInitialRowsList.Count > 0) and (FCurrentRowsList.Count > 0) then

got feedback with bit different stacktrace

15:58:07.754 ERROR [00619029] ZAbstractDataset.TZAbstractDataset.InternalUpdate (Line 388, "..\..\src\component\ZAbstractDataset.pas" + 10) + $E
15:58:07.754 ERROR [00543871] ZDbcUtils.RaiseSQLException (Line 339, "..\..\src\dbc\ZDbcUtils.pas" + 3) + $E
15:58:07.754 ERROR [004A7C8D] JclHookExcept.DoExceptNotify (Line 353, "JclHookExcept.pas" + 27) + $0
15:58:07.754 ERROR [004029D9] JclCharsets.JclCharsetInfos (Line 1139, "JclCharsets") + $E
15:58:07.754 ERROR [00402FF5] JclCharsets.@FreeMem (Line 2466, "C:\Delphi7\Source\RTL\sys\system.pas" + 10) + $0
15:58:07.754 ERROR [004043B0] JclUnicode.TObject.FreeInstance (Line 8366, "JclUnicode" + 2) + $2
15:58:07.754 ERROR TDMSQLite.RFObjectModify() RFObj CID=48 ID=11158: EZDatabaseError($0061902E): Row buffer is not assigned

Looks like, it attempts to update previously disposed row.

It take too much time to debug such things on production, so i change to generationg SQL myself instead of by dataset.

Some questions:
- is single TZConnection is thread-safe for multiple TZQuery or TZReadOnlyQuery from different threads?
- what is best practice for accessing SQLite from multiple threads with Zeos?

[miab3]

There is no good practice in multiple access to SQLite, especially for write access.
https://stackoverflow.com/questions/510 ... iple-users

Michal

[marsupilami]

After adding simple check to prevent 'out of range' exception

Code: Select all

if (FInitialRowsList.Count > 0) and (FCurrentRowsList.Count > 0) then

got feedback with bit different stacktrace

15:58:07.754 ERROR [00619029] ZAbstractDataset.TZAbstractDataset.InternalUpdate (Line 388, "..\..\src\component\ZAbstractDataset.pas" + 10) + $E
15:58:07.754 ERROR [00543871] ZDbcUtils.RaiseSQLException (Line 339, "..\..\src\dbc\ZDbcUtils.pas" + 3) + $E
15:58:07.754 ERROR [004A7C8D] JclHookExcept.DoExceptNotify (Line 353, "JclHookExcept.pas" + 27) + $0
15:58:07.754 ERROR [004029D9] JclCharsets.JclCharsetInfos (Line 1139, "JclCharsets") + $E
15:58:07.754 ERROR [00402FF5] JclCharsets.@FreeMem (Line 2466, "C:\Delphi7\Source\RTL\sys\system.pas" + 10) + $0
15:58:07.754 ERROR [004043B0] JclUnicode.TObject.FreeInstance (Line 8366, "JclUnicode" + 2) + $2
15:58:07.754 ERROR TDMSQLite.RFObjectModify() RFObj CID=48 ID=11158: EZDatabaseError($0061902E): Row buffer is not assigned

Looks like, it attempts to update previously disposed row.

Thank you for the feedback. I forwarded your reply to the ticket on SourceForge.

It take too much time to debug such things on production, so i change to generationg SQL myself instead of by dataset.

Depending on how you do this, this might solve your problem - or not. If you use a separate query for doing writes and use the dataset for reading only, this might work. If you use TZUpdateSQL, this most probably will not work because then you will still have the cached dataset.
If you decide to use the dataset for reading only, it makes sense to switch to TZReadOnlyQuery. Or it might make sense for you to use the DBC layer API.

Some questions:
- is single TZConnection is thread-safe for multiple TZQuery or TZReadOnlyQuery from different threads?
- what is best practice for accessing SQLite from multiple threads with Zeos?

If you work multi threaded the rule is that you need one TZConnection per thread. A TZConnection object cannot be shared between threads. SQLite has the same limitation. See here: https://sqlite.org/threadsafe.html

Best regards,

Jan

[serbod]

I was able to create working example application, that reproduce error in short time (about 5 minutes).

And got detailed stacktrace:

TZRowAccessor.CheckColumnConvertion(???,???)
TZRowAccessor.IsNull(1)
TZGenericCachedResolver.FormWhereClause($3F9976C,$3F4EE28)
TZGenericCachedResolver.FormUpdateStatement($3F9976C,$3F4EE28,$3F4EECC)
TZGenericCachedResolver.PostUpdates(Pointer($3F4ECD0) as IZCachedResultSet,???,$3F4EE28,$3F4EECC)
TZSQLiteCachedResolver.PostUpdates(Pointer($3F4ECD0) as IZCachedResultSet,utModified,$3F4EE28,$3F4EECC)
TZAbstractCachedResultSet.PostRowUpdates(???,???)
TZAbstractCachedResultSet.PostUpdates
TZAbstractCachedResultSet.UpdateRow
TZAbstractDataset.InternalUpdate
TZAbstractDataset.InternalPost

Soon, will reduce code to minimum and publish sources of example application.

[serbod]

Simple example attached.

Multiple TZConnections to same base doesn't works.

[miab3]

@serbod,

What effect (error) should be expected?
For me, on Delphi 10.4.1-Win32 - Zeos 7.3.1-beta r6829 - Sqlite 3.33.0 after ~ 10 minutes he came to:
Main Count: ~ 50,000
Back Count: ~ 7240
The file test.db3 is then ~ 64MB in size
And the StatusObject table ~ 967k records.
And I stopped him to see nothing suspicious.

ADD:
For the remaining identical (as above) Delphi 7 throws an error with Main Count: 87 but only run from Delphi 7 in debugger mode.
However, the Delphi 2007 goes without error.

This could be a debugger bug or conflict for Delphi 7.

ADD2:
However, in Delphi 10.4.1 there are also errors in debug mode.

Michal

[EgonHugeist]

Hello Sebot,
lorbs, we should be able to connect to sqlite using different threads. Patch done R6835 (SVN)\testing-7.3 see https://sourceforge.net/p/zeoslib/code-0/6835/. No clue if i'll merge it to 7.2-fixes. Please test and use one connection/thread. Sharing one connection, writing/reading in other threads sounds wrong to me. As i said before: The reported issue is impossible (in normal aps) looking to the code you mentioned. For Example:

Code: Select all

procedure TZAbstractCachedResultSet.InsertRow;
var TempRow: PZRowBuffer;
  Succeeded: Boolean;
begin
  CheckUpdatable;

  { Creates a new row. }
  TempRow := FRowAccessor.RowBuffer;
  FRowAccessor.Alloc;
  FRowAccessor.MoveFrom(FInsertedRow);
  FRowAccessor.RowBuffer^.UpdateType := utInserted;
  FRowAccessor.RowBuffer^.Index := GetNextRowIndex;

  AppendRow(FRowAccessor.RowBuffer); [b]<============= FInitialRowsList and FCurrentRowsList do get a copy of the rowbuffer[/b]

  { Posts non-cached updates. }
  if not FCachedUpdates then begin
    Succeeded := False;
    try
      PostUpdates;
      Succeeded := True;
    finally //EH no reraising of an Exception required -> keep original stack frame i.e. MadExcept
      if not Succeeded then begin [b]<=========== only in case of fails the previously added rows where removed again [/b]
        { Restore the previous state. See AppendRow}
        FRowAccessor.DisposeBuffer(FInitialRowsList[FInitialRowsList.Count - 1]);
        FInitialRowsList.Delete(FInitialRowsList.Count - 1);
        FRowAccessor.DisposeBuffer(FCurrentRowsList[FCurrentRowsList.Count - 1]);
        FCurrentRowsList.Delete(FCurrentRowsList.Count - 1);
        FRowAccessor.RowBuffer := TempRow;
      end;
    end;
  end;
  FRowsList.Add(FRowAccessor.RowBuffer);
  FRowAccessor.ClearBuffer(FInsertedRow, True);
  LastRowNo := FRowsList.Count;
  MoveAbsolute(LastRowNo);
end;

So yet i can't see a regression on Zeos side... Others?

[marsupilami]

No clue if i'll merge it to 7.2-fixes.

If it is simple, I vote for merging. Otherwise it has to go to the known issues list.