From d2f7dfa6190f529f7e543380b6663074e9078207 Mon Sep 17 00:00:00 2001 From: stephan Date: Wed, 26 Feb 2025 03:03:08 +0000 Subject: [PATCH] More work on the sahpool digest fix. New/fixed versions can read legacy (no digest) files but the reverse is only possible in limited circumstances (when files originated from a legacy version). The burning question is whether the real fix would be to remove this digest check altogether, as it only applies in a very limited context, and the fact that it was broken for some 18 months unnoticed suggests that its value might not be worth the CPU cycles. FossilOrigin-Name: 0df62b776c68bebb0e187b353b6f29b0a41a29f0a1c8d6728fa6b9f7ce0d13f7 --- ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js | 52 ++++++++++++------- ext/wasm/tests/opfs/sahpool/digest-worker.js | 9 ++-- ext/wasm/tests/opfs/sahpool/digest.html | 2 +- manifest | 16 +++--- manifest.uuid | 2 +- 5 files changed, 48 insertions(+), 33 deletions(-) diff --git a/ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js b/ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js index 08b77c9c1e..94c890850d 100644 --- a/ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js +++ b/ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js @@ -79,23 +79,39 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ capi.SQLITE_OPEN_MAIN_JOURNAL | capi.SQLITE_OPEN_SUPER_JOURNAL | capi.SQLITE_OPEN_WAL; - const FLAG_COMPUTE_DIGEST_V2 = 1 ? capi.SQLITE_OPEN_MEMORY : 0 + const FLAG_COMPUTE_DIGEST_V2 = capi.SQLITE_OPEN_MEMORY /* Part of the fix for https://github.com/sqlite/sqlite-wasm/issues/97 - Summary: prior to versions 3.49.1 and 3.50 computeDigest() always - computes a value of [0,0], so it does not actually do anything - useful. Fixing it invalidates old persistent files, so we - instead only fix it for files created or updated since the bug - was discovered and fixed. + Summary: prior to versions 3.49.2 and 3.50.0 computeDigest() + always computes a value of [0,0] due to overflows, so it does not + do anything useful. Fixing it invalidates old persistent files, + so we instead only fix it for files created or updated since the + bug was discovered and fixed. This flag determines whether we use the broken legacy computeDigest() or the v2 variant. We only use this flag for newly-created/overwritten files. Pre-existing files have the broken digest stored in them so need to continue to use that. - This flag is stored in the same space as the - SQLITE_OPEN_... flags and we must be careful here to not use an + What this means, in terms of db file compatibility between + versions: + + - DBs created with versions older than this fix (<=3.49.1) + can be read by post-fix versions. Such DBs which are written + to in-place (not replaced) by newer versions can still be read + by older versions, as the affected digest is only modified + when the SAH slot is assigned to a given filename. + + - DBs created with post-fix versions will, when read by a pre-fix + version, be seen as having a "bad digest" and will be + unceremoniously replaced by that pre-fix version. When swapping + back to a post-fix version, that version will see that the file + entry is missing the FLAG_COMPUTE_DIGEST_V2 bit so will treat it + as a legacy file. + + This flag is stored in the same memory as the variour + SQLITE_OPEN_... flags and we must be careful here to not use a flag bit which is otherwise relevant for the VFS. SQLITE_OPEN_MEMORY is handled by sqlite3_open_v2() and friends, not the VFS, so we'll repurpose that one. If we take a @@ -659,7 +675,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ const fileDigest = new Uint32Array(HEADER_DIGEST_SIZE / 4); sah.read(fileDigest, {at: HEADER_OFFSET_DIGEST}); const compDigest = this.computeDigest(this.#apBody, flags); - console.warn("getAssociatedPath() flags",flags.toString(16), "compDigest", compDigest); + //warn("getAssociatedPath() flags",'0x'+flags.toString(16), "compDigest", compDigest); if(fileDigest.every((v,i) => v===compDigest[i])){ // Valid digest const pathBytes = this.#apBody.findIndex((v)=>0===v); @@ -691,16 +707,16 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ toss("Path too long:",path); } if(path && flags){ - /* When re-writing files, update their digest, if needed, - to v2. We continue to use v1 for the (!path) case - (empty files) because there's little reason not to - use a digest of 0 for empty entries. */ + /* When creating or re-writing files, update their digest, if + needed, to v2. We continue to use v1 for the (!path) case + (empty files) because there's little reason not to use a + digest of 0 for empty entries. */ flags |= FLAG_COMPUTE_DIGEST_V2; } this.#apBody.fill(0, enc.written, HEADER_MAX_PATH_SIZE); this.#dvBody.setUint32(HEADER_OFFSET_FLAGS, flags); const digest = this.computeDigest(this.#apBody, flags); - console.warn("setAssociatedPath(",path,") digest",digest); + //console.warn("setAssociatedPath(",path,") digest",digest); sah.write(this.#apBody, {at: 0}); sah.write(digest, {at: HEADER_OFFSET_DIGEST}); sah.flush(); @@ -725,18 +741,18 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ See the docs for FLAG_COMPUTE_DIGEST_V2 for more details. */ computeDigest(byteArray, fileFlags){ - let h1 = 0xdeadbeef; - let h2 = 0x41c6ce57; if( fileFlags & FLAG_COMPUTE_DIGEST_V2 ){ + let h1 = 0xdeadbeef; + let h2 = 0x41c6ce57; for(const v of byteArray){ h1 = Math.imul(h1 ^ v, 2654435761); h2 = Math.imul(h2 ^ v, 104729); } + return new Uint32Array([h1>>>0, h2>>>0]); }else{ /* this is what the buggy legacy computation worked out to */ - h1 = h2 = 0; + return new Uint32Array([0,0]); } - return new Uint32Array([h1>>>0, h2>>>0]); } /** diff --git a/ext/wasm/tests/opfs/sahpool/digest-worker.js b/ext/wasm/tests/opfs/sahpool/digest-worker.js index 5396252a83..4308566675 100644 --- a/ext/wasm/tests/opfs/sahpool/digest-worker.js +++ b/ext/wasm/tests/opfs/sahpool/digest-worker.js @@ -48,7 +48,7 @@ const runTests = function(sqlite3, poolUtil){ sql: "insert into t(a) values(?)", bind: n++ }); - log("Record count: ",db.selectValue("select count(*) from t")); + log(fname,"record count: ",db.selectValue("select count(*) from t")); }finally{ db.close(); } @@ -59,7 +59,7 @@ const runTests = function(sqlite3, poolUtil){ sql: "insert into t(a) values(?)", bind: n++ }); - log("Record count: ",db.selectValue("select count(*) from t")); + log(fname,"record count: ",db.selectValue("select count(*) from t")); }finally{ db.close(); } @@ -74,15 +74,14 @@ const runTests = function(sqlite3, poolUtil){ sql: "insert into t(a) values(?)", bind: n++ }); - log("Record count: ",db.selectValue("select count(*) from t")); + log(fname2,"record count: ",db.selectValue("select count(*) from t")); }finally{ db.close(); } }; globalThis.sqlite3InitModule().then(async function(sqlite3){ - log("sqlite3 version:",sqlite3.capi.sqlite3_libversion(), - sqlite3.capi.sqlite3_sourceid()); + log("sqlite3 version:",sqlite3.version); const sahPoolConfig = { name: 'opfs-sahpool-digest', clearOnInit: false, diff --git a/ext/wasm/tests/opfs/sahpool/digest.html b/ext/wasm/tests/opfs/sahpool/digest.html index c4ba6cc1aa..fdcd98ec13 100644 --- a/ext/wasm/tests/opfs/sahpool/digest.html +++ b/ext/wasm/tests/opfs/sahpool/digest.html @@ -36,7 +36,7 @@ *********************************************************************** This is a bugfix test for the OPFS SAHPool VFS. It requires setting up - a database created using v3.49.0 or older, then runnig it again with + a database created using v3.49.0 or older, then running it again with a newer version. */ (function(){ diff --git a/manifest b/manifest index 638b314db7..21e7317364 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Add\sa\stest\sapp\sto\sassist\sin\svalidating\sthe\sSAHPool\sdigest\scalculation\sfix. -D 2025-02-03T17:21:54.248 +C More\swork\son\sthe\ssahpool\sdigest\sfix.\sNew/fixed\sversions\scan\sread\slegacy\s(no\sdigest)\sfiles\sbut\sthe\sreverse\sis\sonly\spossible\sin\slimited\scircumstances\s(when\sfiles\soriginated\sfrom\sa\slegacy\sversion).\sThe\sburning\squestion\sis\swhether\sthe\sreal\sfix\swould\sbe\sto\sremove\sthis\sdigest\scheck\saltogether,\sas\sit\sonly\sapplies\sin\sa\svery\slimited\scontext,\sand\sthe\sfact\sthat\sit\swas\sbroken\sfor\ssome\s18\smonths\sunnoticed\ssuggests\sthat\sits\svalue\smight\snot\sbe\sworth\sthe\sCPU\scycles. +D 2025-02-26T03:03:08.171 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md e108e1e69ae8e8a59e93c455654b8ac9356a11720d3345df2a4743e9590fb20d @@ -645,7 +645,7 @@ F ext/wasm/api/sqlite3-api-worker1.c-pp.js 5cc22a3c0d52828cb32aad8691488719f47d2 F ext/wasm/api/sqlite3-license-version-header.js 0c807a421f0187e778dc1078f10d2994b915123c1223fe752b60afdcd1263f89 F ext/wasm/api/sqlite3-opfs-async-proxy.js 3774befd97cd1a5e2895c8225a894aad946848c6d9b4028acc988b5d123475af F ext/wasm/api/sqlite3-vfs-helper.c-pp.js 3f828cc66758acb40e9c5b4dcfd87fd478a14c8fb7f0630264e6c7fa0e57515d -F ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js d1d17004bfee7a9159b56888b83345c6219262448297a81966faae4fd28f749c +F ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js c18540f86e2d10a48d67f933edb05e29a0e2c904dbdc996f3c765a8b63c49aea F ext/wasm/api/sqlite3-vfs-opfs.c-pp.js 9b86ca2d8276cf919fbc9ba2a10e9786033b64f92c2db844d951804dee6c4b4e F ext/wasm/api/sqlite3-vtab-helper.c-pp.js e809739d71e8b35dfe1b55d24d91f02d04239e6aef7ca1ea92a15a29e704f616 F ext/wasm/api/sqlite3-wasm.c 6f9d8529072d072359cd22dc5dfb0572c524684686569cfbd0f9640d7619fc10 @@ -700,8 +700,8 @@ F ext/wasm/tester1.c-pp.js 005a94be87b888ca33aff130aeaef58045781ebfb92e559063d14 F ext/wasm/tests/opfs/concurrency/index.html 657578a6e9ce1e9b8be951549ed93a6a471f4520a99e5b545928668f4285fb5e F ext/wasm/tests/opfs/concurrency/test.js d08889a5bb6e61937d0b8cbb78c9efbefbf65ad09f510589c779b7cc6a803a88 F ext/wasm/tests/opfs/concurrency/worker.js 0a8c1a3e6ebb38aabbee24f122693f1fb29d599948915c76906681bb7da1d3d2 -F ext/wasm/tests/opfs/sahpool/digest-worker.js cdf4bb6dad5e274985e4b0022d0d85f9e782df03352044d8c7c22532845d7304 -F ext/wasm/tests/opfs/sahpool/digest.html 83d0c2f1dd6fe83e9088c603091514ac8549ee04a4632d8cc51d27fd094f05b3 +F ext/wasm/tests/opfs/sahpool/digest-worker.js f8320caaf6368ee8c59f259de5bebe68232f678390dc773ee61c466e171881dc +F ext/wasm/tests/opfs/sahpool/digest.html 08122dabf8ef56ac7d40f43660e178811166842bb47b37cbb11003030bb9ce09 F ext/wasm/wasmfs.make 68999f5bd8c489239592d59a420f8c627c99169bbd6fa16a404751f757b9f702 F magic.txt 5ade0bc977aa135e79e3faaea894d5671b26107cc91e70783aa7dc83f22f3ba0 F main.mk 8cfe182232ac7bbc87530792db6f31c09f2a2f35e9887d0412978746efe42ea9 @@ -2211,8 +2211,8 @@ F tool/version-info.c 3b36468a90faf1bbd59c65fd0eb66522d9f941eedd364fabccd7227350 F tool/warnings-clang.sh bbf6a1e685e534c92ec2bfba5b1745f34fb6f0bc2a362850723a9ee87c1b31a7 F tool/warnings.sh 49a486c5069de041aedcbde4de178293e0463ae9918ecad7539eedf0ec77a139 F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f -P 9234c33f92d92bfddc6211c9c587f1072e70837c0ffe1416ef7d84d59bacd364 -R 176b17e8248aa3df480fb81ba267bf4c +P a1e304b8020025cc73a658bd8c7697d59b4f3ad96cac0a3e36553a3207d13dc6 +R 428e6412102dca0ea6c390f44246e72c U stephan -Z 9279d9607c28db362f757a9d03a072e6 +Z eddb8c14b5cfddbfe410d5840cc6ef5c # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index fef85d18dd..2130fc3e97 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -a1e304b8020025cc73a658bd8c7697d59b4f3ad96cac0a3e36553a3207d13dc6 +0df62b776c68bebb0e187b353b6f29b0a41a29f0a1c8d6728fa6b9f7ce0d13f7