1878311 - Enable SQLITE_DIRECT_OVERFLOW_READ optimization

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, 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 :)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

FYI, no change in status with SQLite 3.46.0.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(In reply to D. Richard Hipp from comment #5)

Oops. I'll make sure this is resolved in the next release.

Hi Richard, I'm just wondering if there's any updates on this? It appears that the newly-released 3.47.0 is still impacted.

Comment 9

[D. Richard Hipp]

New version of obfsvfs.c published at https://sqlite.org/mozilla/file/obfsvfs.c. See the diff at https://sqlite.org/mozilla/info/6c21f43a0595de24

If you pair this new obfsvfs.c with the latest trunk check-in of SQLite (https://sqlite.org/src/info/6e7d498cda1d8fa7) then your problem should go away: You can still compile SQLite using -DSQLITE_DIRECT_OVERFLOW_READ=1 however that optimization will be omitted when reading/writing to an obfuscated database, as it does not work in that context.

Without these patches, you should build with -DSQLITE_DIRECT_OVERFLOW_READ=0. The direct-overflow-read optimization does reads of less than a whole page. But obfsvfs.c uses the number of bytes being read to determine whether or not it should deobfuscate. So obviously the obfuscation and the direct-overflow-read optimization do not play well together. There is some performance advantage to using direct-overflow-read, but not that much. So you are not losing out by disabling it.

Comment 10

[Marco Bonardo [:mak]]

Thank you!
Our version of that code is in https://searchfox.org/mozilla-central/rev/360b702e1ec793c49dc66122e50fe580d43350b3/storage/ObfuscatingVFS.cpp#447-450
Harveer, do you have time to look into this, so we can remove the non-default config?

Comment 11

[Harveer Singh]

ok testing. will update back soon.

Comment 12

[Harveer Singh]

I don't see this working for me. Made this revision and had all sort of tests broken in private browsing.
https://hg.mozilla.org/try/rev/cb683dbe838c184da25066583fa1f43b921d7c27

Comment 13

[Jan Varga [:janv]]

(In reply to Harveer Singh from comment #12)

I don't see this working for me. Made this revision and had all sort of tests broken in private browsing.
https://hg.mozilla.org/try/rev/cb683dbe838c184da25066583fa1f43b921d7c27

You need to adapt https://sqlite.org/mozilla/info/6c21f43a0595de24 to https://searchfox.org/mozilla-central/rev/360b702e1ec793c49dc66122e50fe580d43350b3/storage/ObfuscatingVFS.cpp

Comment 14

[Harveer Singh]

yes, seems like I missed to include that file in my patch. I will amend the patch and try again.

Comment 15

[Harveer Singh]

Yes, I can confirm that after including changes made in obfsvfs.c to ObfusactingVFS.cpp, all the wpt privatebrowsing tests passed.
So, seems like it worked.

Comment 16

[Jan Varga [:janv]]

(In reply to Harveer Singh from comment #15)

Yes, I can confirm that after including changes made in obfsvfs.c to ObfusactingVFS.cpp, all the wpt privatebrowsing tests passed.
So, seems like it worked.

Can you share a try push ?

Comment 17

[Harveer Singh]

It was this[https://treeherder.mozilla.org/jobs?repo=try&revision=05339a0f777594665014f88963daedb1a04235de&selectedTaskRun=SbDXux2MSveXg5HBAWuGug.0] that I created but I see that it was missing changes in moz.build which enables back the optimization. I am not sure how the patch lost the changes in moz.build because I just amended to change the previous patch which was missing changes in ObfuscatingVFS.cpp

Okay, so I created a new push which just runs privatebrowsing wpt: https://treeherder.mozilla.org/jobs?repo=try&revision=5a28126105e3dbdf5f81340dfbb7c327076434f1

Let's see how this does now.

Comment 18

[Harveer Singh]

marco, I see that with these sqlite changes our tests are now passing, try. So we could probably revert back to enable read optimization in moz.build. How we plan to move forward with this; so all in all we have changes in four files, ObfuscatingVFS.cpp, moz.build and couple sqlite files.

Comment 19

[Marco Bonardo [:mak]]

(In reply to Harveer Singh from comment #18)

How we plan to move forward with this; so all in all we have changes in four files, ObfuscatingVFS.cpp, moz.build and couple sqlite files.

We can make this depend on us updating to the next Sqlite version.
You could start attaching a patch with changes on our side (excluding sqlite3.c and sqlite3.h).
Bug 1926321 will likely be updated once a new Sqlite 3.47.x version is out (we can wait), we will vendor that in tree. Then we can land your patch on top of 3.47.x.

Comment 20

[Harveer Singh]

sure, I can create a patch for my side of the things and attach here.

Comment 21

[Harveer Singh]

Attachment: Bug 1878311: Enable sqlite optimization 'SQLITE_DIRECT_OVERFLOW_READ' back.r=#dom-storage-reviewers

Comment 22

[Marco Bonardo [:mak]]

Could you please check and finalized the patch on top of 3.47.1 that just landed?

Comment 23

[Harveer Singh]

Attachment: Bug 1878311: Sync obfsDeviceCharacteristics method with sqlite obfsvfs.c.r=#dom-storage-reviewers

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9814549b623
Sync obfsDeviceCharacteristics method with sqlite obfsvfs.c.r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/7e5f0db99f79
Enable sqlite optimization 'SQLITE_DIRECT_OVERFLOW_READ' back.r=dom-storage-reviewers,janv

Comment 25

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/a9814549b623
https://hg.mozilla.org/mozilla-central/rev/7e5f0db99f79