VLD Reports Botan Sqlite3 Memory Leaks with Botan 1.10.5 Amalgamation

[corytrese]

When using this solution, encryption works very very well but it Visual Leak Detector and CLR report leaks of memory under all compilers I tested:

https://vld.codeplex.com/

Here is a call stack from Visual Leak Detector under Visual Studio

---------- Block 9719 at 0x04DF60C0: 65536 bytes ---------- Call Stack: e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2428): HeroesOfSteel.win32.exe!Botan::`anonymous namespace'::do_malloc + 0xC bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2481): HeroesOfSteel.win32.exe!Botan::Locking_Allocator::alloc_block + 0xB bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2391): HeroesOfSteel.win32.exe!Botan::Pooling_Allocator::get_more_core + 0x13 bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2304): HeroesOfSteel.win32.exe!Botan::Pooling_Allocator::allocate e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (514): HeroesOfSteel.win32.exe!Botan::MemoryRegion::allocate + 0x1B bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (540): HeroesOfSteel.win32.exe!Botan::MemoryRegion::resize + 0xC bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (509): HeroesOfSteel.win32.exe!Botan::MemoryRegion::init + 0x42 bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (663): HeroesOfSteel.win32.exe!Botan::SecureVector::SecureVector + 0x5F bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (4725): HeroesOfSteel.win32.exe!Botan::Twofish::Twofish + 0x6A bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (4723): HeroesOfSteel.win32.exe!Botan::Twofish::clone + 0x6D bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (13156): HeroesOfSteel.win32.exe!Botan::CMAC::clone + 0x35 bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2065): HeroesOfSteel.win32.exe!Botan::Algorithm_Factory::make_mac + 0xF bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (7814): HeroesOfSteel.win32.exe!Botan::MAC_Filter::MAC_Filter + 0x27 bytes e:\cocos2d-x-2.2\storytellercore\classes\codec.cpp (41): HeroesOfSteel.win32.exe!Codec::InitializeCodec + 0x3B bytes e:\cocos2d-x-2.2\storytellercore\classes\codec.cpp (16): HeroesOfSteel.win32.exe!Codec::Codec e:\cocos2d-x-2.2\storytellercore\classes\codec.cpp (164): HeroesOfSteel.win32.exe!InitializeNewCodec + 0x2B bytes e:\cocos2d-x-2.2\storytellercore\classes\codecext.c (105): HeroesOfSteel.win32.exe!sqlite3CodecAttach + 0x9 bytes e:\cocos2d-x-2.2\storytellercore\classes\codecext.c (131): HeroesOfSteel.win32.exe!sqlite3_key + 0x13 bytes e:\cocos2d-x-2.2\storytellercore\classes\cppsqlite3.cpp (1223): HeroesOfSteel.win32.exe!CppSQLite3DB::key + 0x14 bytes

My Botan Build was automatically generated Fri Sep 6 15:07:13 2013 UTC by

• Digital@Void running './configure.py --no-autoload --enable-modules=twofish,xts,pbkdf2,cmac,sha1 --gen-amalgamation --cc=msvc --os=win32 --cpu=x86 --disable-shared --disable-asm'

All total the amalgamation generates around 1800 leaked objects for my connection and initial set of queries. I am guessing that each query is leaking a little bit of the key, or the algo?

I would love to get this fixed / explained.

EDITS: Added link to VLD, updated title, added specific CLR versions and compilers.

[corytrese]

Here is another example leak:

---------- Block 9732 at 0x04D8A378: 8 bytes ---------- Call Stack: e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.h (9067): HeroesOfSteel.win32.exe!Botan::PKCS5_PBKDF2::clone + 0x7 bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (2077): HeroesOfSteel.win32.exe!Botan::Algorithm_Factory::make_pbkdf + 0xF bytes e:\cocos2d-x-2.2\storytellercore\classes\win32\botan_all.cpp (10936): HeroesOfSteel.win32.exe!Botan::get_pbkdf + 0x2A bytes e:\cocos2d-x-2.2\storytellercore\classes\codec.cpp (57): HeroesOfSteel.win32.exe!Codec::GenerateWriteKey + 0xA bytes e:\cocos2d-x-2.2\storytellercore\classes\codec.cpp (171): HeroesOfSteel.win32.exe!GenerateWriteKey e:\cocos2d-x-2.2\storytellercore\classes\codecext.c (106): HeroesOfSteel.win32.exe!sqlite3CodecAttach + 0x27 bytes e:\cocos2d-x-2.2\storytellercore\classes\codecext.c (131): HeroesOfSteel.win32.exe!sqlite3_key + 0x13 bytes

[corytrese]

I found this bug report against VLD for sqlite3

http://vld.codeplex.com/discussions/401811

But that isn't behind sqlite3_key which seems to be the starting point of every leak VLD is seeing.

[OlivierJG]

This project is getting progressively more embarassing... and I don't currently use it so it just sits here.

Your first report is curious, as it indicates that Codec's m_cmac is leaking. It should be owned by m_macPipe, so it's not immediately obvious why that's happening. Since this obviously isn't exception-safe, maybe there's an exception being generated there? Did you change the algorithms?

Your second report is a dumb leak of "PBKDF *pbkdf" on codec.cpp:56, which should be wrapped by a smart pointer.

I'll accept any patches if you are up to providing them and they fix these issues for you.

[corytrese]

Please do not take my report as a negative.

I would categorize this project as "brilliant" and "amazing" -- the word "embarrassing" never once entered my mind.

I configured the key sizes differently than the example, but I reproduced the report with Visual Leak Detector on both the provided sample and my production configuration.

I will continue to investigate and report any solutions or clues I find that are actionable. I have had issues with VLD incorrectly reporting memory leaks on more exotic types of smart pointers in the past.

In the five hours since my post, I'm no closer to a leak free runtime on Win32 but Instruments on iOS compiled with XCode reports no leaks in Botan/BotanSqlite3/Sqlite3

If you'd like to reach me directly I'm cory.trese@gmail.com and https://twitter.com/corytrese

I'm a huge fan and thanks again for all your hard work on this project as well as Botan (and your contributions on Stack Overflow, which have been tremendously helpful as well.)

[OlivierJG]

Ok, let me know what you discover then. It would be useful to know if m_cmac is leaked for every Codec created, and Codec is leaking any other members.

Could you also confirm that deleting/smart-pointerizing fixes the pbkdf leak whenever you get a chance to test it?

[corytrese]

It looks like the common element is ...

codec.cpp (14): HeroesOfSteel.win32.exe!Codec::Codec + 0x6B bytes
codec.cpp (164): HeroesOfSteel.win32.exe!InitializeNewCodec + 0x2B bytes
codecext.c (105): HeroesOfSteel.win32.exe!sqlite3CodecAttach + 0x9 bytes
codecext.c (131): HeroesOfSteel.win32.exe!sqlite3_key + 0x13 bytes
cppsqlite3.cpp (1223): HeroesOfSteel.win32.exe!CppSQLite3DB::key + 0x14 bytes

This leak test is against the "main menu" in my application which is loading only one encrypted database.

The code is pushing the library to the maximum logical limit -- multiple threads, several different keys, using 'attach' statements with and without keys. The library holds up under heavy load and, aside from the win32 leak detection tools, performs trouble free on nearly 10 platforms.

That said -- I am out of my depth a bit here. I'm looking at Codec::Coded (codec.cpp) and it doesn't seem to have a deconstructor like I would expect. It contains a line ' m_cmac = new MAC_Filter(MAC_STR);' but I cannot find the matching 'delete m_cmac' which I think is what you are asking about.

win32\botan_all.cpp (7495): HeroesOfSteel.win32.exe!Botan::Core_Engine::find_mac + 0x7 bytes
win32\botan_all.cpp (1832): HeroesOfSteel.win32.exe!Botan::`anonymous namespace'::engine_get_algo<Botan::MessageAuthenticationCode> + 0x35 bytes
win32\botan_all.cpp (1859): HeroesOfSteel.win32.exe!Botan::`anonymous namespace'::factory_prototype<Botan::MessageAuthenticationCode> + 0x1F bytes
win32\botan_all.cpp (2006): HeroesOfSteel.win32.exe!Botan::Algorithm_Factory::prototype_mac + 0x1C bytes
classes\win32\botan_all.cpp (2064): HeroesOfSteel.win32.exe!Botan::Algorithm_Factory::make_mac + 0x10 bytes
win32\botan_all.cpp (7814): HeroesOfSteel.win32.exe!Botan::MAC_Filter::MAC_Filter + 0x27 bytes
codec.cpp (41): HeroesOfSteel.win32.exe!Codec::InitializeCodec + 0x3B bytes
codec.cpp (16): HeroesOfSteel.win32.exe!Codec::Codec
codec.cpp (164): HeroesOfSteel.win32.exe!InitializeNewCodec + 0x2B bytes
codecext.c (105): HeroesOfSteel.win32.exe!sqlite3CodecAttach + 0x9 bytes
codecext.c (131): HeroesOfSteel.win32.exe!sqlite3_key + 0x13 bytes

I'm still investigating and will continue to do so. Please forgive me if this update adds nothing new for you.

[OlivierJG]

m_cmac is appended to m_macPipe, and so should be deleted when m_macPipe goes out of scope (so, when the codec is deleted). The only way this might not be the case is if an exception is thrown before it's assigned to the pipe (that this is possible is a bug, but I assume you've checked if you get an error there already?). The other possibility is that the Codec isn't getting deleted, but then you should be seeing much more leakage than the cmac. Assuming you can rule out these two possibilities, obviously I'm missing something (or there is a bug in VLD, but we're not going there yet). As I said, I don't actually use this for anything anymore, but if you can reproduce the leak using the simple test_sqlite3 app, or create a simple app that exposes this leak, I should be able to fix it.

[corytrese]

Here we are -- morning time again. Amazing what 8 hours of diagnostic data and an e-mail from Jack Lloyd and Olivier de Gaalon can do!

I have divided this issue into two sub-issues and dealt with them individually. First, leaking large parts of Botan (m_macPipe, Algo Factory, etc) is one issue. Second, leaking pbkdf inside of void Codec::GenerateWriteKey(const char *userPassword, int passwordLength)

First issue:

As I have stated above, my usage of botansqlite3 is much more sophisticated than the included test scripts. I'm connecting to multiple databases with different keys and using 'attach' between encrypted databases to unencrypted and encrypted databases.

In this usage scenario the code in Codec may be non-optimal? This function might initialize Botan repeatedly in more complex usage of botansqlite3? It appears that it does.

void InitializeBotan() {
    LibraryInitializer::initialize();
}

I have removed that line from codec.cpp and moved the initializer and deinitalizer to my AppDelegate (top level class) like this

AppDelegate::AppDelegate()
{
    LibraryInitializer::initialize();
}

AppDelegate::~AppDelegate()
{
    LibraryInitializer::deinitialize();
}

This was proposed by Jack Lloyd when he said "[multiple initialization] is a supported option but means that no cleanup occurs unless you explicitly call Library_State::deinitialize.

After making that change, VLD only reports the second issue for 3 leaks. They all look like this:

classes\win32\botan_all.h (3708): HeroesOfSteel.win32.exe!Botan::SHA_160::clone + 0x49 bytes
classes\win32\botan_all.cpp (13265): HeroesOfSteel.win32.exe!Botan::HMAC::clone + 0x35 bytes
classes\win32\botan_all.h (9068): HeroesOfSteel.win32.exe!Botan::PKCS5_PBKDF2::clone + 0x35 bytes
classes\win32\botan_all.cpp (2077): HeroesOfSteel.win32.exe!Botan::Algorithm_Factory::make_pbkdf + 0xF bytes
classes\win32\botan_all.cpp (10936): HeroesOfSteel.win32.exe!Botan::get_pbkdf + 0x2A bytes
classes\codec.cpp (57): HeroesOfSteel.win32.exe!Codec::GenerateWriteKey + 0xA bytes
classes\codec.cpp (172): HeroesOfSteel.win32.exe!GenerateWriteKey
classes\codecext.c (106): HeroesOfSteel.win32.exe!sqlite3CodecAttach + 0x27 bytes
classes\codecext.c (131): HeroesOfSteel.win32.exe!sqlite3_key + 0x13 bytes
classes\cppsqlite3.cpp (1223): HeroesOfSteel.win32.exe!CppSQLite3DB::key + 0x14 bytes

In GenerateWriteKey I see this line:

PBKDF *pbkdf = get_pbkdf(PBKDF_STR);

I'm just guessing at this, but it does make VLD stop reporting leaks ... adding one line #73

delete pbkdf;

I do not honestly know what that does tho, my encryption keeps working and no leaks are reported.

Last question I guess -- does that delete call make sense? Or am I just doing something dumb that happens to reduce the errors?

[OlivierJG]

As to the first, I was unaware that sqlite3_activate_see could be called multiple times. I'd have to look into sqlite3's source to find out what the correct behavior is here. Of course it will leak for the one time it's called, which would be nice to fix. Possibly this will require more invasive changes to sqlite3 source to do properly and invisibly.

As to the second, I already pointed out that leak in previous comments :). Delete is fine, but smart pointer is better.

[OlivierJG]

BTW, the reason for not using the LibraryInitializer RAII class is because the end-user may be using Botan outside of the SQLite3 codec, in which case it may deinitialize the Botan library too soon. Of course the same would be the case if I attempt to call it anywhere else, probably the only solution here is to add a "deactivate" function that the end-user must call, or otherwise let them say "I'll manage the Botan state", as you've done here.

[corytrese]

I think sqlite3_activate_see could be called multiple times if I am using multiple database connections inside the same application perhaps? I am definitely doing this. As for looking into sqlite3's source code, I will undertake that and see if I can save you the effort.

As you noted, I failed to understand what you were saying about the "dumb leak" -- sorry for the repetition, this is all very new to me.

I'm wholly unqualified to make such a call for the average user, but I will probably use the library patched to require the process to manage Botan state. I might even try to patch Codec::Codec to assert if Botan isn't initialized (if I can figure out how to add such a check.)

Questions:

1. Would you accept an interim patch that fixed it using 'delete' instead of a smart pointer?
2. Would the requirements of not using LibraryInitializer RAII be more clear in "test_sqlite.cpp" if we added LibraryInitializer::deinitialize(); and a comment?

At the present time, all my leak and memory profiling is coming back "No Leaks Detected" after the following:

1. Following your initial suggestion fixing the leak of "PBKDF *pbkdf" on codec.cpp:56
2. Moving to controlling LibraryInitializer::initialize(); and LibraryInitializer::deinitialize(); in my application

Thank you very much for your help and for putting this wonderful library under the Botan license.

[OlivierJG]

1. Would you accept an interim patch that fixed it using 'delete' instead of a smart pointer?

I would, but really, just std::auto_ptr wrapping should do (since obviously I'm not jumping to c++11's unique_ptr here). (Be nice to also do the same for the s2k pointer, for anyone still using Botan 1.8.)

1. Would the requirements of not using LibraryInitializer RAII be more clear in "test_sqlite.cpp" if we added LibraryInitializer::deinitialize(); and a comment?

Some notes on this issue:

1. Calling LibraryInitializer::initialize() multiple times is a waste of time, but it does not leak any more memory than calling it once, as Botan will delete the old state.
2. Botan's global state will leak (as it does now) if LibraryInitializer::deinitialize() is not called, but this is really a cosmetic issue, as it'll almost always need to last the lifetime of the program anyhow.
3. User's of botansqlite3 should not have any requirement to directly interface with Botan (ie, they wouldn't want to call LibraryInitializer::deinitialize()).

Now, the fix is either to add another compile config to let users decide to control Botan's lifetime themselves (and then by default call ::deinitialize from botansqlite3), or to offer control over Botan's lifetime by adding initialize_botansqlite3() and deinitialize_botansqlite3().

Both of these options introduce an RTFM requirement to some users, which is sightly bothersome... I guess I'd lean towards the former option just because it's less surprising for those already using botansqlite3.

Any preference here?