gnucash maint: Multiple changes pushed

Geert Janssens gjanssens at code.gnucash.org
Mon Sep 10 13:50:11 EDT 2018


Updated	 via  https://github.com/Gnucash/gnucash/commit/48b29f5e (commit)
	 via  https://github.com/Gnucash/gnucash/commit/b866d7d9 (commit)
	from  https://github.com/Gnucash/gnucash/commit/196decf6 (commit)



commit 48b29f5e91b6bce411a10505ad08b657967ec222
Author: Geert Janssens <geert at kobaltwit.be>
Date:   Mon Sep 10 19:49:43 2018 +0200

    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.

diff --git a/libgnucash/app-utils/test/test-option-util.cpp b/libgnucash/app-utils/test/test-option-util.cpp
index 4c7d358..d0fff4e 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 534c185..c1f635a 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<GncGUID*> (guid_copy (
             guid))));
diff --git a/libgnucash/backend/xml/sixtp-dom-parsers.cpp b/libgnucash/backend/xml/sixtp-dom-parsers.cpp
index 0bde990..7762903 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 ad6b8fd..6749d0e 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 ab2f3be..5c4addd 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<gchar *> (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<const gchar *>());
+        this->datastore = const_cast<const gchar *>(g_strdup(other.get<const gchar *>()));
     else if (other.datastore.type() == typeid(GncGUID*))
         this->datastore = guid_copy(other.get<GncGUID *>());
     else if (other.datastore.type() == typeid(GList*))
diff --git a/libgnucash/engine/kvp-value.hpp b/libgnucash/engine/kvp-value.hpp
index acc072b..61b7dfd 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 c1fb7b0..db7bfd2 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<const char*>(), 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 29a162c..7ad855d 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 4a3e93c..d8cc90b 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);
 

commit b866d7d955f89d74ac31ea49167c95a9cdbc8899
Author: Geert Janssens <geert at kobaltwit.be>
Date:   Mon Sep 10 18:57:39 2018 +0200

    Plug memory leak in xaccSplitDestroy
    
    Splits were not marked for deletion if the transaction is read-only
    and the account is not marked for deletion yet. The net result is
    that split will not be freed later on.
    However xaccSplitDestroy is also called from a Transaction's do_destroy.
    At that point accounts are not necessarily marked for deletion yet (like
    is the case when a datafile is closed). This turned out to be a problem
    for invoice post transactions (which are also read only) and hence
    would cause memory to leak.

diff --git a/libgnucash/engine/Split.c b/libgnucash/engine/Split.c
index 65c3e2e..288c424 100644
--- a/libgnucash/engine/Split.c
+++ b/libgnucash/engine/Split.c
@@ -1447,6 +1447,7 @@ xaccSplitDestroy (Split *split)
     acc = split->acc;
     trans = split->parent;
     if (acc && !qof_instance_get_destroying(acc)
+        && !qof_instance_get_destroying(trans)
         && xaccTransGetReadOnly(trans))
         return FALSE;
 



Summary of changes:
 libgnucash/app-utils/test/test-option-util.cpp         |  2 +-
 libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp |  2 +-
 libgnucash/backend/xml/sixtp-dom-parsers.cpp           |  2 +-
 libgnucash/engine/Split.c                              |  1 +
 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 ++--
 10 files changed, 18 insertions(+), 17 deletions(-)



More information about the gnucash-changes mailing list