1878311 - Enable SQLITE_DIRECT_OVERFLOW_READ optimization

Status: NEW

(Toolkit :: Storage, enhancement)

Description

[Ryan VanderMeulen [:RyanVM]]

SQLite 3.45 enabled a new optimization by default which may be beneficial to our builds as outlined in bug 1874951 comment 2. Unfortunately, it caused test failures in the web-platform-tests-privatebrowsing suite like the below:
TEST-UNEXPECTED-FAIL | /service-workers/cache-storage/cache-add.https.any.html | Cache.add with request with null body (not consumed) - promise_test: Unhandled rejection with value: object "[Exception... "File error: Corrupted" nsresult: "0x8052000b (NS_ERROR_FILE_CORRUPTED)" location: "<unknown>" data: no]"

In order to unblock the SQLite update, the SQLITE_DIRECT_OVERFLOW_READ option was set to 0 in the SQLite moz.build file. We should remove that so this optimization can be re-enabled once our VFS can be adapted to the changes.

Comment 1

[Marco Bonardo [:mak]]

Moving the needinfo from https://bugzilla.mozilla.org/show_bug.cgi?id=1874951#c13

Comment 2

[Harveer Singh]

I can see that we only encrypt/decrypt sqlite data if they are page size (currently, 8K)
https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/storage/ObfuscatingVFS.cpp#351 (write operation)
https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/storage/ObfuscatingVFS.cpp#319 (read)

From the sqlite3.c, I could see that when attempting to read from overflow pages (with SQLITE_DIRECT_OVERFLOW_READ enabled) sqlite could attempt to read in chunks of overflow size (which is PAGE_SIZE - kReservedBytes) and is smaller than PAGE_SIZE and that's why our read logic skipped to decrypt the payload.

https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/security/nss/lib/sqlite/sqlite3.c#68104 (amount of bytes to read)
https://searchfox.org/mozilla-central/rev/896042a1a71066254ceb5291f016ca3dbca21cb7/security/nss/lib/sqlite/sqlite3.c#68131 (sqlite read operation)

It seems to me that this could be a sqlite issue because when attempting to write the same page, sqlite did in PAGE_SIZE amounts however, when reading it read in PAGE_SIZE - kReservedBytes amount.

Comment 3

[Harveer Singh]

I have confirmed that by changing
(iAmt == OBFS_PGSZ || iAmt == OBFS_PGSZ + WAL_FRAMEHDRSIZE) && !p->inCkpt) condition to
(iAmt == OBFS_PGSZ || iAmt == OBFS_PGSZ + WAL_FRAMEHDRSIZE || iAmt == OBFS_PGSZ - kReservedBytes ) && !p->inCkpt); does make our tests pass.

Comment 4

[Jan Varga [:janv]]

Clearly, someone from sqlite team should take a look. I'm not sure if they still maintain it, but they even had own regression testing for the obfsvfs.c
See https://sqlite.org/mozilla/info/86ee454caac2a509

Comment 5

[D. Richard Hipp]

Oops. I'll make sure this is resolved in the next release. In the meantime, an obvious work-around for y'all is to add the -DSQLITE_DIRECT_OVERFLOW_READ=0 compile-time option when building SQLite.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Yeah, that's what we did for getting 3.45.1 landed :)