Releasing reserved bytes on decrypting

[utelle]

When a not-encrypted database is encrypted, your code uses SQLite's function sqlite3RunVacuum to reserve the bytes per page required for storing nonce and authentication, and to implicitly perform the encryption.

In principle the same procedure is used to decrypt an encrypted database. However, your code shows the below comment and does not touch the reserved space.

/* Truncating the reserved space is dangerous (may corrupt the DB)
   sqlite3BtreeSetPageSize(pBt, sqlite3BtreeGetPageSize(pBt), 0, 0); */

Have you actually experienced database corruption on trying to remove the reserved space on decrypting? If yes, under which circumstances?

For a test I uncommented the call to sqlite3BtreeSetPageSize, but it had no effect at all. The decrypted database still had 32 bytes of reserved space per page.

Inspecting the code of the SQLite function sqlite3RunVacuum the reason gets clear: the reserved space is determined by a call to sqlite3BtreeGetOptimalReserve. And the strategy seems to be to never decrease the number of reserved bytes.

Experimentally, I copied the code of function sqlite3RunVacuum into file sqleet.c, renamed the function, and applied a small modification (namely passing the value nRes as a parameter instead of determining it with sqlite3BtreeGetOptimalReserve). Then I modified your function sqlite3_rekey_v2 to call the new function with 0 on decrypting and 32 on encrypting for the parameter nRes. At least in my experiments I did not experience any problems.

Would you consider to apply this change?

[resilar]

Thanks for your work. The comment is indeed inaccurate and IIRC I had to apply similar patch in sqlite3RunVacuum to get it to actually truncate the pages. This resulted in corrupted databases in my experiments, but it is possible that I did something wrong. The corruption might have happened after performing write operations on the decrypted database.

Would you consider to apply this change?

The downside is that it will affect negatively the forward & backward compatibility across SQLite versions because sqlite3RunVacuum tends to get updated now and then. But an intuitively working sqlite3_rekey_v2 decrypting is so big win that I am willing to merge such a patch if it does not cause any major issues (like corrupted databases). This means that sqleet will have to target only the latest version of SQLite3 in the future, but I guess that is completely acceptable. Especially now that the versioning has been changed to match SQLite3 version numbers.

Feel free to send me a PR if you have the patch ready. However, I'd prefer it if you save the modified version of sqlite3RunVacuum in vacuum.c and include that in sqleet.c. This would probably help a bit with keeping the function up to date when SQLite3 releases a new version.

[utelle]

AFAICT the adjusted version of sqlite3RunVacuum seems to work without problems, but I agree that more thorough tests should be conducted to be sure that database corruption is unlikely to happen.

It is true that sqlite3RunVacuum gets updated from time to time (changes occurred in August 2017, June 2017, and several in 2016). OTOH SQLite claims that the database format is almost completely compatible among all 3.x versions. So I assume that executing a VACUUM command should usually not corrupt a database file, even if the database file was created with a later version.

Nevertheless, on upgrading the included SQLite version changes to sqlite2RunVacuum - if any - should be also incorporated. This slightly increases the effort to upgrade to a new SQLite version, but it's certainly worth it.

[resilar]

Yes. Due to the stable database format, it is unlikely that using mismatching versions of SQLite3 and sqlite3RunVacuum somehow corrupts the database or works incorrectly. I am more worried about the private internals of SQLite3 changing so that sqleet's modified vacuum code no longer compiles straight out of the box. But such a forward/backward compatibility break is not a deal-breaker for this.

---

Btw, I figured out why I got corrupted databases before. It was because I manipulated the return value of sqlite3BtreeGetOptimalReserve by overwriting pBt->pageSize, pBt->usableSize and pBt->optimalReserve (since sqlite3BtreeSetPageSize had no effect). This seemed to work in some cases, but in other cases corrupted the DB.

[utelle]

I am more worried about the private internals of SQLite3 changing so that sqleet's modified vacuum code no longer compiles straight out of the box.

Well, that can happen for other reasons, too. I had to adjust my wxSQLite3 encryption extension several times in the course of time, since for example the codec handling changed. This is to be expected in the future again. Since the encryption extension is usually bundled with a certain SQLite version, it can make the upgrading process a bit more difficult.

Yes, tampering with pagesize and optimal reserve can have bad side effects. In conjunction with sqlite3RunVacuum calling the function sqlite3BtreeSetPageSize has no effect, since sqlite3RunVacuum calls the function internally with different values. Therefore the adjusted vacuum function is needed.