Page 1 of 2

potential critical crash in ZDbcCachedResultSet.pas

Posted: 23.01.2020, 10:36
by 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'.

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 23.01.2020, 11:38
by 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

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 03.02.2020, 18:44
by marsupilami
Hello serbod,

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

Jan

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 06.05.2020, 11:24
by 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.

Re: potential critical crash in ZDbcCachedResultSet.pas

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

Image

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 08.05.2020, 21:48
by 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.

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 28.05.2020, 09:46
by serbod
aehimself wrote: 08.05.2020, 21:48 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.

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 21.09.2020, 07:55
by 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?

Re: potential critical crash in ZDbcCachedResultSet.pas

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

Michal

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 21.09.2020, 17:20
by marsupilami
serbod wrote: 21.09.2020, 07:55 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.
serbod wrote: 21.09.2020, 07:55It 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.
serbod wrote: 21.09.2020, 07:55 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

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 22.09.2020, 12:22
by 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.

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 22.09.2020, 13:14
by serbod
Simple example attached.

Multiple TZConnections to same base doesn't works.

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 22.09.2020, 14:37
by 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

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 23.09.2020, 06:20
by 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?

Re: potential critical crash in ZDbcCachedResultSet.pas

Posted: 23.09.2020, 12:40
by marsupilami
EgonHugeist wrote: 23.09.2020, 06:20 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.