fix: some memory cleanups

rustyconover · rustyconover · commit cfc17e428e3a · 2025-12-03T01:01:48.000-05:00
diff --git a/src/crypto_extension.cpp b/src/crypto_extension.cpp
@@ -302,29 +302,16 @@ namespace duckdb
 
     struct HashAggregateState
     {
-        // No need to free this is just a reference to the algorithm.
-        blake3_hasher *blake3_hasher;
+        bool is_touched;
+        blake3_hasher blake3_hasher;
         EVP_MD_CTX *ctx;
 
         HashAggregateState()
         {
-            blake3_hasher = nullptr;
+            is_touched = false;
+            blake3_hasher_init(&blake3_hasher);
             ctx = nullptr;
         }
-
-        ~HashAggregateState()
-        {
-            if (ctx)
-            {
-                EVP_MD_CTX_free(ctx);
-                ctx = nullptr;
-            }
-            if (blake3_hasher)
-            {
-                delete blake3_hasher;
-                blake3_hasher = nullptr;
-            }
-        }
     };
 
     struct HashAggregateBindData : public FunctionData
@@ -354,8 +341,20 @@ namespace duckdb
         template <class STATE>
         static void Initialize(STATE &state)
         {
-            state.blake3_hasher = nullptr;
+            // So this may fill the state with random garbage
             state.ctx = nullptr;
+            state.is_touched = false;
+            blake3_hasher_init(&state.blake3_hasher);
+        }
+
+        template <class STATE>
+        static void Destroy(STATE &state, AggregateInputData &aggr_input_data)
+        {
+            if (state.ctx)
+            {
+                EVP_MD_CTX_free(state.ctx);
+                state.ctx = nullptr;
+            }
         }
 
         static bool IgnoreNull()
@@ -371,56 +370,53 @@ namespace duckdb
 
             if (bind_data.is_blake3)
             {
-                if (!state.blake3_hasher)
+                if (!state.is_touched)
                 {
-                    state.blake3_hasher = new blake3_hasher;
-                    blake3_hasher_init(state.blake3_hasher);
+                    blake3_hasher_init(&state.blake3_hasher);
+                    state.is_touched = true;
                 }
 
                 // hash the record length as well to prevent length extension attacks
                 if constexpr (std::is_same_v<A_TYPE, string_t>)
                 {
                     const uint64_t size = a_data.GetSize();
-                    blake3_hasher_update(state.blake3_hasher, &size, sizeof(uint64_t));
-                    blake3_hasher_update(state.blake3_hasher, a_data.GetDataUnsafe(), size);
+                    blake3_hasher_update(&state.blake3_hasher, &size, sizeof(uint64_t));
+                    blake3_hasher_update(&state.blake3_hasher, a_data.GetDataUnsafe(), size);
                 }
                 else
                 {
-                    blake3_hasher_update(state.blake3_hasher, &a_data, sizeof(a_data));
+                    blake3_hasher_update(&state.blake3_hasher, &a_data, sizeof(a_data));
                 }
             }
             else
             {
-                if (!state.ctx)
+                if (!state.is_touched)
                 {
                     state.ctx = EVP_MD_CTX_new();
                     if (EVP_DigestInit_ex(state.ctx, bind_data.md, nullptr) != 1)
                     {
-                        EVP_MD_CTX_free(state.ctx);
                         throw InternalException("Failed to initialize hash context");
                     }
+                    state.is_touched = true;
                 }
 
                 if constexpr (std::is_same_v<A_TYPE, string_t>)
                 {
                     const uint64_t size = a_data.GetSize();
                     if (EVP_DigestUpdate(state.ctx, &size, sizeof(uint64_t)) != 1)
                     {
-                        EVP_MD_CTX_free(state.ctx);
                         throw InternalException("Failed to update hash");
                     }
 
                     if (EVP_DigestUpdate(state.ctx, a_data.GetDataUnsafe(), size) != 1)
                     {
-                        EVP_MD_CTX_free(state.ctx);
                         throw InternalException("Failed to update hash");
                     }
                 }
                 else
                 {
                     if (EVP_DigestUpdate(state.ctx, &a_data, sizeof(a_data)) != 1)
                     {
-                        EVP_MD_CTX_free(state.ctx);
                         throw InternalException("Failed to update hash");
                     }
                 }
@@ -441,13 +437,21 @@ namespace duckdb
         static void Combine(const STATE &source, STATE &target, AggregateInputData &aggr_input_data)
         {
             auto &bind_data = aggr_input_data.bind_data->Cast<HashAggregateBindData>();
-            const bool source_used = bind_data.is_blake3 ? (source.blake3_hasher != nullptr) : (source.ctx != nullptr);
-            const bool target_used = bind_data.is_blake3 ? (target.blake3_hasher != nullptr) : (target.ctx != nullptr);
+            const bool source_used = source.is_touched;
+            const bool target_used = target.is_touched;
 
             if (source_used && !target_used)
             {
-                target.blake3_hasher = source.blake3_hasher;
-                target.ctx = source.ctx;
+                if (bind_data.is_blake3)
+                {
+                    target.blake3_hasher = source.blake3_hasher;
+                }
+                else
+                {
+                    target.ctx = EVP_MD_CTX_new();
+                    EVP_MD_CTX_copy_ex(target.ctx, source.ctx);
+                }
+                target.is_touched = true;
             }
             else if (!source_used)
             {
@@ -464,19 +468,15 @@ namespace duckdb
             auto &bind_data = finalize_data.input.bind_data->Cast<HashAggregateBindData>();
             if (bind_data.is_blake3)
             {
-                if (!state.blake3_hasher)
+                if (!state.is_touched)
                 {
-                    delete state.blake3_hasher;
-                    state.blake3_hasher = nullptr;
                     finalize_data.ReturnNull();
                     return;
                 }
                 char output[BLAKE3_OUT_LEN];
-                blake3_hasher_finalize(state.blake3_hasher, reinterpret_cast<uint8_t *>(&output), BLAKE3_OUT_LEN);
+                blake3_hasher_finalize(&state.blake3_hasher, reinterpret_cast<uint8_t *>(&output), BLAKE3_OUT_LEN);
                 target = StringVector::AddStringOrBlob(finalize_data.result, reinterpret_cast<const char *>(&output),
                                                        BLAKE3_OUT_LEN);
-                delete state.blake3_hasher;
-                state.blake3_hasher = nullptr;
             }
             else
             {
@@ -491,11 +491,8 @@ namespace duckdb
                 // Finalize the hash
                 if (EVP_DigestFinal_ex(state.ctx, hash_result, &hash_len) != 1)
                 {
-                    EVP_MD_CTX_free(state.ctx);
                     throw InternalException("Failed to finalize hash");
                 }
-                EVP_MD_CTX_free(state.ctx);
-                state.ctx = nullptr;
                 target = StringVector::AddStringOrBlob(finalize_data.result, reinterpret_cast<const char *>(&hash_result),
                                                        hash_len);
             }
@@ -549,7 +546,7 @@ namespace duckdb
     static void RegisterHashAggType(AggregateFunctionSet &agg_set, const LogicalType &logical_type)
     {
         auto agg_func =
-            AggregateFunction::UnaryAggregate<HashAggregateState, CPP_TYPE, string_t, HashAggregateOperation<HashAggregateState>>(
+            AggregateFunction::UnaryAggregateDestructor<HashAggregateState, CPP_TYPE, string_t, HashAggregateOperation<HashAggregateState>>(
                 logical_type, LogicalType::BLOB);
         agg_func.order_dependent = AggregateOrderDependent::ORDER_DEPENDENT;
         agg_func.distinct_dependent = AggregateDistinctDependent::DISTINCT_DEPENDENT;

Original file line number	Diff line number	Diff line change
`@@ -302,29 +302,16 @@ namespace duckdb`
`302`	`302`
`303`	`303`	`struct HashAggregateState`
`304`	`304`	`{`
`305`		`- // No need to free this is just a reference to the algorithm.`
`306`		`- blake3_hasher *blake3_hasher;`
	`305`	`+ bool is_touched;`
	`306`	`+ blake3_hasher blake3_hasher;`
`307`	`307`	`EVP_MD_CTX *ctx;`
`308`	`308`
`309`	`309`	`HashAggregateState()`
`310`	`310`	`{`
`311`		`- blake3_hasher = nullptr;`
	`311`	`+ is_touched = false;`
	`312`	`+ blake3_hasher_init(&blake3_hasher);`
`312`	`313`	`ctx = nullptr;`
`313`	`314`	`}`
`314`		`-`
`315`		`- ~HashAggregateState()`
`316`		`- {`
`317`		`- if (ctx)`
`318`		`- {`
`319`		`- EVP_MD_CTX_free(ctx);`
`320`		`- ctx = nullptr;`
`321`		`- }`
`322`		`- if (blake3_hasher)`
`323`		`- {`
`324`		`- delete blake3_hasher;`
`325`		`- blake3_hasher = nullptr;`
`326`		`- }`
`327`		`- }`
`328`	`315`	`};`
`329`	`316`
`330`	`317`	`struct HashAggregateBindData : public FunctionData`
`@@ -354,8 +341,20 @@ namespace duckdb`
`354`	`341`	`template <class STATE>`
`355`	`342`	`static void Initialize(STATE &state)`
`356`	`343`	`{`
`357`		`- state.blake3_hasher = nullptr;`
	`344`	`+ // So this may fill the state with random garbage`
`358`	`345`	`state.ctx = nullptr;`
	`346`	`+ state.is_touched = false;`
	`347`	`+ blake3_hasher_init(&state.blake3_hasher);`
	`348`	`+ }`
	`349`	`+`
	`350`	`+ template <class STATE>`
	`351`	`+ static void Destroy(STATE &state, AggregateInputData &aggr_input_data)`
	`352`	`+ {`
	`353`	`+ if (state.ctx)`
	`354`	`+ {`
	`355`	`+ EVP_MD_CTX_free(state.ctx);`
	`356`	`+ state.ctx = nullptr;`
	`357`	`+ }`
`359`	`358`	`}`
`360`	`359`
`361`	`360`	`static bool IgnoreNull()`
`@@ -371,56 +370,53 @@ namespace duckdb`
`371`	`370`
`372`	`371`	`if (bind_data.is_blake3)`
`373`	`372`	`{`
`374`		`- if (!state.blake3_hasher)`
	`373`	`+ if (!state.is_touched)`
`375`	`374`	`{`
`376`		`- state.blake3_hasher = new blake3_hasher;`
`377`		`- blake3_hasher_init(state.blake3_hasher);`
	`375`	`+ blake3_hasher_init(&state.blake3_hasher);`
	`376`	`+ state.is_touched = true;`
`378`	`377`	`}`
`379`	`378`
`380`	`379`	`// hash the record length as well to prevent length extension attacks`
`381`	`380`	`if constexpr (std::is_same_v<A_TYPE, string_t>)`
`382`	`381`	`{`
`383`	`382`	`const uint64_t size = a_data.GetSize();`
`384`		`- blake3_hasher_update(state.blake3_hasher, &size, sizeof(uint64_t));`
`385`		`- blake3_hasher_update(state.blake3_hasher, a_data.GetDataUnsafe(), size);`
	`383`	`+ blake3_hasher_update(&state.blake3_hasher, &size, sizeof(uint64_t));`
	`384`	`+ blake3_hasher_update(&state.blake3_hasher, a_data.GetDataUnsafe(), size);`
`386`	`385`	`}`
`387`	`386`	`else`
`388`	`387`	`{`
`389`		`- blake3_hasher_update(state.blake3_hasher, &a_data, sizeof(a_data));`
	`388`	`+ blake3_hasher_update(&state.blake3_hasher, &a_data, sizeof(a_data));`
`390`	`389`	`}`
`391`	`390`	`}`
`392`	`391`	`else`
`393`	`392`	`{`
`394`		`- if (!state.ctx)`
	`393`	`+ if (!state.is_touched)`
`395`	`394`	`{`
`396`	`395`	`state.ctx = EVP_MD_CTX_new();`
`397`	`396`	`if (EVP_DigestInit_ex(state.ctx, bind_data.md, nullptr) != 1)`
`398`	`397`	`{`
`399`		`- EVP_MD_CTX_free(state.ctx);`
`400`	`398`	`throw InternalException("Failed to initialize hash context");`
`401`	`399`	`}`
	`400`	`+ state.is_touched = true;`
`402`	`401`	`}`
`403`	`402`
`404`	`403`	`if constexpr (std::is_same_v<A_TYPE, string_t>)`
`405`	`404`	`{`
`406`	`405`	`const uint64_t size = a_data.GetSize();`
`407`	`406`	`if (EVP_DigestUpdate(state.ctx, &size, sizeof(uint64_t)) != 1)`
`408`	`407`	`{`
`409`		`- EVP_MD_CTX_free(state.ctx);`
`410`	`408`	`throw InternalException("Failed to update hash");`
`411`	`409`	`}`
`412`	`410`
`413`	`411`	`if (EVP_DigestUpdate(state.ctx, a_data.GetDataUnsafe(), size) != 1)`
`414`	`412`	`{`
`415`		`- EVP_MD_CTX_free(state.ctx);`
`416`	`413`	`throw InternalException("Failed to update hash");`
`417`	`414`	`}`
`418`	`415`	`}`
`419`	`416`	`else`
`420`	`417`	`{`
`421`	`418`	`if (EVP_DigestUpdate(state.ctx, &a_data, sizeof(a_data)) != 1)`
`422`	`419`	`{`
`423`		`- EVP_MD_CTX_free(state.ctx);`
`424`	`420`	`throw InternalException("Failed to update hash");`
`425`	`421`	`}`
`426`	`422`	`}`
`@@ -441,13 +437,21 @@ namespace duckdb`
`441`	`437`	`static void Combine(const STATE &source, STATE &target, AggregateInputData &aggr_input_data)`
`442`	`438`	`{`
`443`	`439`	`auto &bind_data = aggr_input_data.bind_data->Cast<HashAggregateBindData>();`
`444`		`- const bool source_used = bind_data.is_blake3 ? (source.blake3_hasher != nullptr) : (source.ctx != nullptr);`
`445`		`- const bool target_used = bind_data.is_blake3 ? (target.blake3_hasher != nullptr) : (target.ctx != nullptr);`
	`440`	`+ const bool source_used = source.is_touched;`
	`441`	`+ const bool target_used = target.is_touched;`
`446`	`442`
`447`	`443`	`if (source_used && !target_used)`
`448`	`444`	`{`
`449`		`- target.blake3_hasher = source.blake3_hasher;`
`450`		`- target.ctx = source.ctx;`
	`445`	`+ if (bind_data.is_blake3)`
	`446`	`+ {`
	`447`	`+ target.blake3_hasher = source.blake3_hasher;`
	`448`	`+ }`
	`449`	`+ else`
	`450`	`+ {`
	`451`	`+ target.ctx = EVP_MD_CTX_new();`
	`452`	`+ EVP_MD_CTX_copy_ex(target.ctx, source.ctx);`
	`453`	`+ }`
	`454`	`+ target.is_touched = true;`
`451`	`455`	`}`
`452`	`456`	`else if (!source_used)`
`453`	`457`	`{`
`@@ -464,19 +468,15 @@ namespace duckdb`
`464`	`468`	`auto &bind_data = finalize_data.input.bind_data->Cast<HashAggregateBindData>();`
`465`	`469`	`if (bind_data.is_blake3)`
`466`	`470`	`{`
`467`		`- if (!state.blake3_hasher)`
	`471`	`+ if (!state.is_touched)`
`468`	`472`	`{`
`469`		`- delete state.blake3_hasher;`
`470`		`- state.blake3_hasher = nullptr;`
`471`	`473`	`finalize_data.ReturnNull();`
`472`	`474`	`return;`
`473`	`475`	`}`
`474`	`476`	`char output[BLAKE3_OUT_LEN];`
`475`		`- blake3_hasher_finalize(state.blake3_hasher, reinterpret_cast<uint8_t *>(&output), BLAKE3_OUT_LEN);`
	`477`	`+ blake3_hasher_finalize(&state.blake3_hasher, reinterpret_cast<uint8_t *>(&output), BLAKE3_OUT_LEN);`
`476`	`478`	`target = StringVector::AddStringOrBlob(finalize_data.result, reinterpret_cast<const char *>(&output),`
`477`	`479`	`BLAKE3_OUT_LEN);`
`478`		`- delete state.blake3_hasher;`
`479`		`- state.blake3_hasher = nullptr;`
`480`	`480`	`}`
`481`	`481`	`else`
`482`	`482`	`{`
`@@ -491,11 +491,8 @@ namespace duckdb`
`491`	`491`	`// Finalize the hash`
`492`	`492`	`if (EVP_DigestFinal_ex(state.ctx, hash_result, &hash_len) != 1)`
`493`	`493`	`{`
`494`		`- EVP_MD_CTX_free(state.ctx);`
`495`	`494`	`throw InternalException("Failed to finalize hash");`
`496`	`495`	`}`
`497`		`- EVP_MD_CTX_free(state.ctx);`
`498`		`- state.ctx = nullptr;`
`499`	`496`	`target = StringVector::AddStringOrBlob(finalize_data.result, reinterpret_cast<const char *>(&hash_result),`
`500`	`497`	`hash_len);`
`501`	`498`	`}`
`@@ -549,7 +546,7 @@ namespace duckdb`
`549`	`546`	`static void RegisterHashAggType(AggregateFunctionSet &agg_set, const LogicalType &logical_type)`
`550`	`547`	`{`
`551`	`548`	`auto agg_func =`
`552`		`- AggregateFunction::UnaryAggregate<HashAggregateState, CPP_TYPE, string_t, HashAggregateOperation<HashAggregateState>>(`
	`549`	`+ AggregateFunction::UnaryAggregateDestructor<HashAggregateState, CPP_TYPE, string_t, HashAggregateOperation<HashAggregateState>>(`
`553`	`550`	`logical_type, LogicalType::BLOB);`
`554`	`551`	`agg_func.order_dependent = AggregateOrderDependent::ORDER_DEPENDENT;`
`555`	`552`	`agg_func.distinct_dependent = AggregateDistinctDependent::DISTINCT_DEPENDENT;`