utelle / SQLite3MultipleCiphers

SQLite3 encryption extension with support for multiple ciphers
https://utelle.github.io/SQLite3MultipleCiphers/
MIT License
420 stars 77 forks source link

Opening and closing a database multiple times fails #145

Closed UWesthaus closed 8 months ago

UWesthaus commented 8 months ago

If a database file is opened and closed several times within a project, it fails after 2 or 3 successful connects.

I tracked this down to the function sqlite3mcTermCipherTables() which obviousley is freeing initializations performed by sqlite3mcRegisterCipher(). If --globalCipherCount; is added within the function everything seems to be working:

SQLITE_PRIVATE void sqlite3mcTermCipherTables() { size_t n; for (n = CODEC_COUNT_MAX+1; n > 0; --n) { if (globalCodecParameterTable[n].m_name[0] != 0) { int k; CipherParams* params = globalCodecParameterTable[n].m_params; for (k = 0; params[k].m_name[0] != 0; ++k) { sqlite3_free(params[k].m_name); } sqlite3_free(globalCodecParameterTable[n].m_params); --globalCipherCount; } } }

Please check if this fix is correct - since I'm not familiar with the internal structures used.

utelle commented 8 months ago

If a database file is opened and closed several times within a project, it fails after 2 or 3 successful connects.

I suspect that you establish your database connections in some uncommon way, because opening and closing database connections via sqlite3_open (or sqlite3_open_v2) and sqlite3_close (or sqlite3_close_v2) certainly can be done as often as you like.

I tracked this down to the function sqlite3mcTermCipherTables() which obviousley is freeing initializations performed by sqlite3mcRegisterCipher().

Well, there is only a single place where this function is called, namely sqlite3mc_shutdown which in turn is implicitly called from sqlite3_shutdown. The counter part is the function sqlite3mc_initialize which is implicitly called from sqlite3_initialize. Neither sqlite3mc_initialize nor sqlite3mc_shutdown should be called directly. Instead, functions sqlite3_initialize and sqlite3_shutdown should be used.

Currently, function sqlite3_initialize will be called implicitly, if the SQLite library was not yet initialized. The function sqlite3mc_shutdown is not called automatically, but has to be called to avoid memory leaks.

Typically, an application calls sqlite3_initialize just once on starting the application and sqlite3mc_shutdown on terminating the application. These functions are not meant to be called every time before opening resp after closing a database connection.

However, I suspect that you call these functions every time you open resp close a database connection.

Nevertheless, I agree that an application should not fail even if these functions are called several times. Therefore, thanks for reporting this issue.

If --globalCipherCount; is added within the function everything seems to be working:

Since function sqlite3mcTermCipherTables frees the memory for all registered cipher schemes, a simpler fix will be to just reset globalCipherCount at the end of the function.

The drawback of calling sqlite3_initialize and sqlite3_shutdown severeal times is twofold:

  1. it incurs unnecessary overhead,
  2. it requires to explicitly reregister non-builtin cipher schemes.

Therefore it is strongly recommended to call sqlite3_initialize and sqlite3_shutdown only once within a process.

Please check if this fix is correct - since I'm not familiar with the internal structures used.

In principle, the fix would work, but I will apply the simpler fix mentioned above.

UWesthaus commented 8 months ago

Thanks for your comments.

I totally agree that I have been using sqlite3_shutdown incorrectly. I added it to a function block closing a database, because otherwise I get memory leaks when closing the app. But correct usage will be to call sqlite3_shutdown only one time when closing the app, not when switching to another database.

I did not detect this, because in previous releases it was working (my app opens and closes a database only twice).

utelle commented 8 months ago

Commit f1543a77ad61b6b176830f118cd7df823d3e1a94 should fix the problem, so that the SQLite initialization and termination code can be called multiple times within an application without causing trouble.