randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.58k stars 568 forks source link

Windows 11 botan prevents software from closing #3710

Open Christian-Kr opened 1 year ago

Christian-Kr commented 1 year ago

Hello to everyone,

I am writing an application which uses botan's argon2 for hash creation and checking. The application will be written with qt to be portable across windows, Mac and linux (just because I don't like to do coding on windows...).

I use argon like cited below. This code worked for all platforms with botan2. Now that I compiled with botan3 the application won't close anymore (the process in the background of the whole application persists), but only on windows. On Linux and Mac the application will close. Commenting out the botany call lets close the application.

The repository with the application is located here: https://github.com/Christian-Kr/QualificationMatrix

I am not sure whether this is a bug, or there is something special with windows I have to take care of when using botan. But for me this looks like a bug cause it works on two platforms und with botan2. I also don't know if there will be threats created in the background which prevents the software for closing the application.

Thanks for everyone in advance for having a look into the botan library.

Ah...I compiled botan2/3 on windows 11 with mingw.

bool QMAMSManager::checkPasswordHash(const QString &pw, const QString &hash)
{
    auto tmpStdHash = hash.toStdString();
    return Botan::argon2_check_pwhash(pw.toStdString().c_str(), pw.length(), tmpStdHash);
}
Christian-Kr commented 1 year ago

Update: I compiled botan3 with the flag --without-os-feature=threads and everything is working as expected. So I would suggest, this is a os related problem with threads.

I got the workaround from #2582. So maybe this symptom is a result of the bug described there.

randombit commented 1 year ago

2582 is very likely the underlying issue. We still don't know if there is anything we can do here; this seems like maybe a MinGW bug, or at least MinGW threads behaving in a way we don't expect.

randombit commented 1 year ago

As a hack/test, can you try this patch [haven't compiled this, may need minor tweaks]

diff --git a/src/lib/utils/mem_ops.cpp b/src/lib/utils/mem_ops.cpp
index 64cd07ec5..91a491d77 100644
--- a/src/lib/utils/mem_ops.cpp
+++ b/src/lib/utils/mem_ops.cpp
@@ -15,8 +15,18 @@
    #include <botan/internal/locking_allocator.h>
 #endif

+#if defined(BOTAN_HAS_THREAD_UTILS)
+   #include <botan/internal/thread_pool.h>
+#endif
+
 namespace Botan {

+void shut_down_thread_pool() {
+#if defined(BOTAN_HAS_THREAD_UTILS)
+   Thread_Pool::global_instance().shutdown();
+#endif
+}
+
 BOTAN_MALLOC_FN void* allocate_memory(size_t elems, size_t elem_size) {
    if(elems == 0 || elem_size == 0) {
       return nullptr;
diff --git a/src/lib/utils/mem_ops.h b/src/lib/utils/mem_ops.h
index b270e93ac..8092a5026 100644
--- a/src/lib/utils/mem_ops.h
+++ b/src/lib/utils/mem_ops.h
@@ -44,6 +44,11 @@ class Allocator_Initializer final {
       Allocator_Initializer() { initialize_allocator(); }
 };

+/**
+* Only call this on exit!
+*/
+void BOTAN_UNSTABLE_API shut_down_thread_pool();
+
 /**
 * Scrub memory contents in a way that a compiler should not elide,
 * using some system specific technique. Note that this function might

and calling Botan::shut_down_thread_pool() before process exit.

Christian-Kr commented 1 year ago

The patch worked.

Do you need this information just to find the root cause, or might this be a possible code addition, so devs using the lib can force the shutdown by themselves?

randombit commented 1 year ago

Well, that it worked is at least a hint of some kind. The part that is confusing to me is this: Thread_Pool's destructor already calls shutdown, and we have a static var that is the global thread pool. But it seems like on MinGW, at least in some contexts, this destructor is never called, and when this happens we end up in a deadlock. We do have a CI build on MinGW, and it works / does not hang - so it seems somehow dependent on some other factors that we have not been able to pin down.

But of course the status quo, for people/applications that do hit this problem, absolutely sucks because there is no fix short of disabling the thread pool entirely, which is not great. So maybe, at least until/unless we can figure out what the actual problem is, we do need something like above patch.

randombit commented 1 year ago

@Christian-Kr BTW as an additional datapoint - are you using botan via a DLL, or no?

Christian-Kr commented 1 month ago

Sry, for not answering that long: I used the dll-file as far as I remember.