Do not drop read errors of the real file

[mtrossbach]

Hi Ulrich,

I ran into this while testing something else, and it looked serious enough to send you right away.

Type: Bug fix

When a read of the real file fails, mcIoRead doesn't always pass the error on. It returns early for SQLITE_IOERR_SHORT_READ, but any other error runs into the handler for the file type, and mcReadMainDb, mcReadMainJournal, mcReadSubJournal and mcReadWal all start with int rc = SQLITE_OK; and then assign to rc. The failed read reaches SQLite as a success, and SQLite reads on with whatever was left in the buffer.

If the file has no codec, that happens every single time, and statement journals are exactly that case: they are opened without a filename, so pMainDb stays 0 and mcReadSubJournal returns SQLITE_OK no matter what.

That matters because a statement journal holds the original pages of the main database, and ROLLBACK TO writes them back. I simulated a failing read there. SQLite took the untouched buffer for the old page content and wrote it into the database, the transaction was committed, and PRAGMA integrity_check then reported "Rowid out of order". The application never saw an error, and that is the part I find worrying.

With an encrypted main database you usually notice it a moment later, because decrypting the untouched buffer fails. Without a checksum in the cipher scheme, or with an unencrypted database, the error is simply gone.

The fix is one line, returning every error of the real file right away:

-  if (rc == SQLITE_IOERR_SHORT_READ)
+  if (rc != SQLITE_OK)
   {
     return rc;
   }

A short read still takes the same path, since it isn't SQLITE_OK either, and a successful read is unaffected.

How I tested it

I put a VFS below the SQLite3 Multiple Ciphers VFS that lets exactly one operation fail, a read, a write, half a write, a truncate or a memory allocation, and moved that single fault step by step through a workload of a temporary table, a sort, savepoints and a VACUUM. After every run I checked PRAGMA integrity_check and whether the tables were in the state before or after the statement that failed.

Without the change, a failing read of the statement journal repeatedly left the main database corrupted, and the application saw no error. With the change, the read error is reported, the transaction is rolled back and the database stays consistent. I ran this with page sizes of 4 KiB and 1 KiB.

I also let one read of the main database file fail at every position of a query, for an unencrypted database and for the chacha20, sqlcipher and aes256cbc schemes. Whenever the page really has to be read, the error now reaches the application, and no query returned a wrong result without an error.

make, the SQL tests and cryptotest pass, and there are no new compiler warnings.

Best regards,
Markus

[utelle]

Ouch, that is indeed a critical bug. And it is actually surprising that no one seems to have encountered problems with it so far—at least, I haven't heard of any error reports. Thank you very much for discovering the issue. Fortunately, the fix is ​​very simple.