Fix a potential crash while hashing for metrics or LRCLib upload

This isn't so much "fixing a crash that I know can happen" as much as
"significantly increasing the defensiveness of this code which appears
to cause crashes for some users but which I cannot reproduce".

In particular, the docs do not specify that the BCrypt destruction calls
will fail when handed a nullptr, but they might. Also because since we
weren't zero-initialising the pointer members, they could have had random
values that then didn't get overwritten if an error was encountered
during initialisation, and then freeing (or doing anything else really)
definitely *would* be a problem.

Accordingly this *might* fix #419 and #411 but since I can't reproduce
the issue myself, there isn't really any way to know for sure.

src/hash_utils.cpp

@@ -8,51 +8,91 @@
 #include <stdint.h>
 
 #include "hash_utils.h"
-
-#define ASSERT_SUCCESS(Status) assert(((NTSTATUS)(Status)) >= 0)
+#include "mvtf/mvtf.h"
 
 Sha256Context::Sha256Context()
+    : m_algorithm_handle(nullptr)
+    , m_hash_handle(nullptr)
+    , m_internal_storage(nullptr)
+    , m_error(false)
 {
-    ASSERT_SUCCESS(BCryptOpenAlgorithmProvider((BCRYPT_ALG_HANDLE*)&m_algorithm_handle,
-                                               BCRYPT_SHA256_ALGORITHM,
-                                               nullptr,
-                                               BCRYPT_HASH_REUSABLE_FLAG));
+    m_error = m_error ||
+              FAILED(BCryptOpenAlgorithmProvider((BCRYPT_ALG_HANDLE*)&m_algorithm_handle,
+                                                 BCRYPT_SHA256_ALGORITHM,
+                                                 nullptr,
+                                                 BCRYPT_HASH_REUSABLE_FLAG));
 
     // BCryptGetProperty wants to output the number of bytes written to the output buffer.
     // This parameter is meant to be optional but we get failures without it.
     ULONG get_property_bytes_written = 0;
     DWORD hash_obj_len = 0;
-    ASSERT_SUCCESS(BCryptGetProperty((BCRYPT_ALG_HANDLE)m_algorithm_handle,
-                                     BCRYPT_OBJECT_LENGTH,
-                                     (PBYTE)&hash_obj_len,
-                                     sizeof(hash_obj_len),
-                                     &get_property_bytes_written,
-                                     0));
+    m_error = m_error ||
+              FAILED(BCryptGetProperty((BCRYPT_ALG_HANDLE)m_algorithm_handle,
+                                       BCRYPT_OBJECT_LENGTH,
+                                       (PBYTE)&hash_obj_len,
+                                       sizeof(hash_obj_len),
+                                       &get_property_bytes_written,
+                                       0));
     m_internal_storage = malloc(hash_obj_len);
     assert(m_internal_storage != nullptr);
 
-    ASSERT_SUCCESS(BCryptCreateHash((BCRYPT_ALG_HANDLE)m_algorithm_handle,
-                                    (BCRYPT_HASH_HANDLE*)&m_hash_handle,
-                                    (UCHAR*)m_internal_storage,
-                                    hash_obj_len,
-                                    nullptr,
-                                    0,
-                                    BCRYPT_HASH_REUSABLE_FLAG));
+    m_error = m_error ||
+              FAILED(BCryptCreateHash((BCRYPT_ALG_HANDLE)m_algorithm_handle,
+                                      (BCRYPT_HASH_HANDLE*)&m_hash_handle,
+                                      (UCHAR*)m_internal_storage,
+                                      hash_obj_len,
+                                      nullptr,
+                                      0,
+                                      BCRYPT_HASH_REUSABLE_FLAG));
 }
 
 Sha256Context::~Sha256Context()
 {
-    BCryptDestroyHash(m_hash_handle);
+    if(m_hash_handle != nullptr)
+    {
+        BCryptDestroyHash(m_hash_handle);
+    }
     free(m_internal_storage);
-    BCryptCloseAlgorithmProvider(m_algorithm_handle, 0);
+    if(m_algorithm_handle != nullptr)
+    {
+        BCryptCloseAlgorithmProvider(m_algorithm_handle, 0);
+    }
 }
 
 void Sha256Context::add_data(uint8_t* buffer, size_t buffer_len)
 {
-    ASSERT_SUCCESS(BCryptHashData(m_hash_handle, buffer, (ULONG)buffer_len, 0));
+    m_error = m_error || FAILED(BCryptHashData(m_hash_handle, buffer, (ULONG)buffer_len, 0));
 }
 
 void Sha256Context::finalise(uint8_t (&output)[32])
 {
-    ASSERT_SUCCESS(BCryptFinishHash(m_hash_handle, output, sizeof(output), 0));
+    if(m_error)
+    {
+        memset(output, 0x00, sizeof(output));
+    }
+    else
+    {
+        m_error = FAILED(BCryptFinishHash(m_hash_handle, output, sizeof(output), 0));
+    }
+}
+
+
+// ============
+// Tests
+// ============
+#ifdef MVTF_TESTS_ENABLED
+MVTF_TEST(sha256_correctly_hashes_input_text)
+{
+    const std::string_view input = "some random example text";
+    uint8_t output[32];
+    Sha256Context ctx;
+    ctx.add_data((uint8_t*)input.data(), input.length());
+    ctx.finalise(output);
+
+    pfc::string8 dumped = pfc::format_hexdump(output, sizeof(output), "");
+    const pfc::string8 expected_output = "AB5FE4152E4ECEC0FE1863076E21AF2A99C87D1F109B66BB72A8A751D7DFAFA0";
+    ASSERT(output[0] == 0xAB);
+    ASSERT(dumped == expected_output);
+    ASSERT(!ctx.m_error);
 }
+#endif

src/hash_utils.h

@@ -6,6 +6,7 @@ struct Sha256Context
     void* m_algorithm_handle;
     void* m_hash_handle;
     void* m_internal_storage;
+    bool m_error;
 
     Sha256Context();
     ~Sha256Context();

src/main.cpp

@@ -36,6 +36,7 @@ static void compute_about_message_string(pfc::string_base & out)
     "- Fix a crash when trying to show lyrics for a track with none saved\n"
     "- Fix letras.com returning random lyrics when a good match isn't found\n"
     "- Fix SongLyrics.com still returning lyrics saying there are no lyrics\n"
+    "- Fix a potential crash while hashing for metrics or LRCLib upload\n"
     "\n";
     out += "Version 1.11 (2024-09-05):\n"
     "- Add a source for Bandcamp.com\n"

src/sources/lrclib.cpp

@@ -338,7 +338,6 @@ static uint64_t solve_challenge(const UploadChallenge& challenge, abort_callback
     LARGE_INTEGER start_time = {};
     QueryPerformanceCounter(&start_time);
 
-    Sha256Context sha;
     uint8_t target_buffer[32] = {};
     uint8_t hash_buffer[32] = {};
     char combined_input[128] = {};
@@ -360,12 +359,18 @@ static uint64_t solve_challenge(const UploadChallenge& challenge, abort_callback
         const size_t nonce_len = (size_t)snprintf(nonce_start_ptr, nonce_capacity, "%llu", nonce);
         const size_t combined_len = prefix_len + nonce_len;
 
+        Sha256Context sha;
         sha.add_data((uint8_t*)&combined_input[0], combined_len);
         sha.finalise(hash_buffer);
 
         if(is_hash_less_or_equal(hash_buffer, target_buffer)) {
             break;
         }
+        if(sha.m_error)
+        {
+            LOG_WARN("Breaking out of proof of work calculation due to error in hash calculation");
+            break;
+        }
         if((nonce % 1024 == 0) && (abort.is_aborting()))
         {
             LOG_INFO("Breaking out of proof-of-work calculation early due to an abort signal");