From 48b29f5e91b6bce411a10505ad08b657967ec222 Mon Sep 17 00:00:00 2001 From: Geert Janssens Date: Mon, 10 Sep 2018 19:49:43 +0200 Subject: [PATCH] Fix memory leak in char* type KvpValue and fix improper uses The core issue was that the delete visitor was never called because its parameter type (char *) didn't match the boost::variant type (const char *). Fixing the visitor's parameter type also require a const_cast back to char * because that's what g_free takes as argument. The rest of this commit is merely fixing KvpValue instantiations that tried to create a char* KvpValue from a stack based const string instead of a heap allocated one. That would bomb out on calling the delete visitor. --- libgnucash/app-utils/test/test-option-util.cpp | 2 +- .../backend/dbi/test/test-backend-dbi-basic.cpp | 2 +- libgnucash/backend/xml/sixtp-dom-parsers.cpp | 2 +- libgnucash/engine/gnc-aqbanking-templates.cpp | 12 ++++++------ libgnucash/engine/kvp-value.cpp | 6 +++--- libgnucash/engine/kvp-value.hpp | 2 +- libgnucash/engine/qofbook.cpp | 2 +- libgnucash/engine/test/test-kvp-frame.cpp | 2 +- libgnucash/engine/test/utest-Transaction.cpp | 4 ++-- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/libgnucash/app-utils/test/test-option-util.cpp b/libgnucash/app-utils/test/test-option-util.cpp index 4c7d358b44..d0fff4e875 100644 --- a/libgnucash/app-utils/test/test-option-util.cpp +++ b/libgnucash/app-utils/test/test-option-util.cpp @@ -70,7 +70,7 @@ setup_kvp (Fixture *fixture, gconstpointer pData) NULL); slots->set_path({"options", "Business", "Company Name"}, - new KvpValue("Bogus Company")); + new KvpValue(g_strdup ("Bogus Company"))); qof_commit_edit (QOF_INSTANCE (book)); } diff --git a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp index 534c185d81..c1f635acd6 100644 --- a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp +++ b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp @@ -139,7 +139,7 @@ setup_memory (Fixture* fixture, gconstpointer pData) frame->set ({"double-val"}, new KvpValue (3.14159)); frame->set ({"numeric-val"}, new KvpValue (gnc_numeric_zero ())); frame->set ({"time-val"}, new KvpValue (gnc_time(nullptr))); - frame->set ({"string-val"}, new KvpValue ("abcdefghijklmnop")); + frame->set ({"string-val"}, new KvpValue (g_strdup ("abcdefghijklmnop"))); auto guid = qof_instance_get_guid (QOF_INSTANCE (acct1)); frame->set ({"guid-val"}, new KvpValue (const_cast (guid_copy ( guid)))); diff --git a/libgnucash/backend/xml/sixtp-dom-parsers.cpp b/libgnucash/backend/xml/sixtp-dom-parsers.cpp index 0bde9909b3..7762903963 100644 --- a/libgnucash/backend/xml/sixtp-dom-parsers.cpp +++ b/libgnucash/backend/xml/sixtp-dom-parsers.cpp @@ -205,7 +205,7 @@ dom_tree_to_numeric_kvp_value (xmlNodePtr node) static KvpValue* dom_tree_to_string_kvp_value (xmlNodePtr node) { - gchar* datext; + const gchar* datext; KvpValue* ret = NULL; datext = dom_tree_to_text (node); diff --git a/libgnucash/engine/gnc-aqbanking-templates.cpp b/libgnucash/engine/gnc-aqbanking-templates.cpp index ad6b8fda0a..6749d0eb87 100644 --- a/libgnucash/engine/gnc-aqbanking-templates.cpp +++ b/libgnucash/engine/gnc-aqbanking-templates.cpp @@ -106,13 +106,13 @@ KvpFrame* _GncABTransTempl::make_kvp_frame() { auto frame = new KvpFrame; - frame->set({TT_NAME}, new KvpValue(m_name.c_str())); - frame->set({TT_RNAME}, new KvpValue(m_recipient_name.c_str())); - frame->set({TT_RACC}, new KvpValue(m_recipient_account.c_str())); - frame->set({TT_RBCODE}, new KvpValue(m_recipient_bankcode.c_str())); + frame->set({TT_NAME}, new KvpValue(g_strdup (m_name.c_str()))); + frame->set({TT_RNAME}, new KvpValue(g_strdup (m_recipient_name.c_str()))); + frame->set({TT_RACC}, new KvpValue(g_strdup (m_recipient_account.c_str()))); + frame->set({TT_RBCODE}, new KvpValue(g_strdup (m_recipient_bankcode.c_str()))); frame->set({TT_AMOUNT}, new KvpValue(m_amount)); - frame->set({TT_PURPOS}, new KvpValue(m_purpose.c_str())); - frame->set({TT_PURPOSCT}, new KvpValue(m_purpose_continuation.c_str())); + frame->set({TT_PURPOS}, new KvpValue(g_strdup (m_purpose.c_str()))); + frame->set({TT_PURPOSCT}, new KvpValue(g_strdup (m_purpose_continuation.c_str()))); return frame; } diff --git a/libgnucash/engine/kvp-value.cpp b/libgnucash/engine/kvp-value.cpp index ab2f3bee89..5c4addd3d6 100644 --- a/libgnucash/engine/kvp-value.cpp +++ b/libgnucash/engine/kvp-value.cpp @@ -365,9 +365,9 @@ delete_visitor::operator()(GList * & value) g_list_free_full(value, destroy_value); } template <> void -delete_visitor::operator()(gchar * & value) +delete_visitor::operator()(const gchar * & value) { - g_free(value); + g_free(const_cast (value)); } template <> void delete_visitor::operator()(GncGUID * & value) @@ -390,7 +390,7 @@ void KvpValueImpl::duplicate(const KvpValueImpl& other) noexcept { if (other.datastore.type() == typeid(const gchar *)) - this->datastore = g_strdup(other.get()); + this->datastore = const_cast(g_strdup(other.get())); else if (other.datastore.type() == typeid(GncGUID*)) this->datastore = guid_copy(other.get()); else if (other.datastore.type() == typeid(GList*)) diff --git a/libgnucash/engine/kvp-value.hpp b/libgnucash/engine/kvp-value.hpp index acc072b824..61b7dfd99f 100644 --- a/libgnucash/engine/kvp-value.hpp +++ b/libgnucash/engine/kvp-value.hpp @@ -86,7 +86,7 @@ struct KvpValueImpl /** Create a KvpValue containing the passed in item. Note that for pointer * types const char*, KvpFrame*, GncGUID*, and GList* the KvpValue takes - * ownership of the objcet and will delete/free it when the KvpValue is + * ownership of the object and will delete/free it when the KvpValue is * destroyed. That means these objects must be allocated in the free store * or heap as follows: * * const char*: GLib string allocation, e.g. g_strdup()/ diff --git a/libgnucash/engine/qofbook.cpp b/libgnucash/engine/qofbook.cpp index c1fb7b0942..db7bfd20e3 100644 --- a/libgnucash/engine/qofbook.cpp +++ b/libgnucash/engine/qofbook.cpp @@ -1211,7 +1211,7 @@ qof_book_set_feature (QofBook *book, const gchar *key, const gchar *descr) if (feature == nullptr || g_strcmp0 (feature->get(), descr)) { qof_book_begin_edit (book); - delete frame->set_path({GNC_FEATURES, key}, new KvpValue(descr)); + delete frame->set_path({GNC_FEATURES, key}, new KvpValue(g_strdup (descr))); qof_instance_set_dirty (QOF_INSTANCE (book)); qof_book_commit_edit (book); } diff --git a/libgnucash/engine/test/test-kvp-frame.cpp b/libgnucash/engine/test/test-kvp-frame.cpp index 29a162c146..7ad855da65 100644 --- a/libgnucash/engine/test/test-kvp-frame.cpp +++ b/libgnucash/engine/test/test-kvp-frame.cpp @@ -33,7 +33,7 @@ class KvpFrameTest : public ::testing::Test public: KvpFrameTest() : t_int_val{new KvpValue {INT64_C(15)}}, - t_str_val{new KvpValue{"a value"}} { + t_str_val{new KvpValue{g_strdup ("a value")}} { auto f1 = new KvpFrame; t_root.set({"top"}, new KvpValue{f1}); f1->set({"first"}, t_int_val); diff --git a/libgnucash/engine/test/utest-Transaction.cpp b/libgnucash/engine/test/utest-Transaction.cpp index 4a3e93c811..d8cc90bfc9 100644 --- a/libgnucash/engine/test/utest-Transaction.cpp +++ b/libgnucash/engine/test/utest-Transaction.cpp @@ -151,7 +151,7 @@ setup (Fixture *fixture, gconstpointer pData) xaccTransSetCurrency (txn, fixture->curr); xaccSplitSetParent (split1, txn); xaccSplitSetParent (split2, txn); - frame->set({trans_notes_str}, new KvpValue("Salt pork sausage")); + frame->set({trans_notes_str}, new KvpValue(g_strdup ("Salt pork sausage"))); frame->set_path({"qux", "quux", "corge"}, new KvpValue(123.456)); qof_instance_set_slots (QOF_INSTANCE (txn), frame); } @@ -557,7 +557,7 @@ test_dupe_trans (Fixture *fixture, gconstpointer pData) oldtxn->date_posted = posted; oldtxn->date_entered = entered; oldtxn->inst.kvp_data->set({"foo", "bar", "baz"}, - new KvpValue("The Great Waldo Pepper")); + new KvpValue(g_strdup ("The Great Waldo Pepper"))); newtxn = fixture->func->dupe_trans (oldtxn);