From f51474d9bb1131fa0c4cc5b4d648f1d7944cefb5 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Wed, 7 Jul 2021 20:56:58 +0100 Subject: [PATCH] Bug 798234 - Cut Transaction discards the reference to the description/memo strings so that Paste Transaction will paste uninitialised data (or other strings) When a transaction is cut, any globally unique description and memo strings that are referenced by FloatingTxn/FloatingSplit will refer to memory that has been freed. This results in uninitialised data (or other strings) being inserted when the transaction is pasted. These strings are in the string cache so take a new reference to them when copying them to the FloatingTxn/FloatingSplit. Fix memory leaks of FloatingTxn and FloatingSplit when they're used temporarily. Fix memory leak in FloatingTxn by freeing the m_splits list too. --- .../ledger-core/split-register-copy-ops.c | 45 ++++++++++++++----- .../ledger-core/split-register-copy-ops.h | 4 ++ gnucash/register/ledger-core/split-register.c | 6 ++- .../test/utest-split-register-copy-ops.c | 10 ++--- 4 files changed, 44 insertions(+), 21 deletions(-) diff --git a/gnucash/register/ledger-core/split-register-copy-ops.c b/gnucash/register/ledger-core/split-register-copy-ops.c index 1461a9fc64..5b4b6a7869 100644 --- a/gnucash/register/ledger-core/split-register-copy-ops.c +++ b/gnucash/register/ledger-core/split-register-copy-ops.c @@ -103,13 +103,13 @@ void gnc_float_split_set_transaction (FloatingSplit *fs, Transaction *transactio void gnc_float_split_set_memo (FloatingSplit *fs, const char *memo) { g_return_if_fail (fs); - fs->m_memo = memo; + CACHE_REPLACE (fs->m_memo, memo); }; void gnc_float_split_set_action (FloatingSplit *fs, const char *action) { g_return_if_fail (fs); - fs->m_action = action; + CACHE_REPLACE (fs->m_action, action); }; void gnc_float_split_set_reconcile_state (FloatingSplit *fs, char reconcile_state) @@ -152,8 +152,8 @@ FloatingSplit *gnc_split_to_float_split (Split *split) fs->m_split = split; fs->m_account = xaccSplitGetAccount (split); fs->m_transaction = xaccSplitGetParent (split); - fs->m_memo = xaccSplitGetMemo (split); - fs->m_action = xaccSplitGetAction (split); + fs->m_memo = CACHE_INSERT (xaccSplitGetMemo (split)); + fs->m_action = CACHE_INSERT (xaccSplitGetAction (split)); fs->m_reconcile_state = xaccSplitGetReconcile (split); fs->m_reconcile_date = xaccSplitGetDateReconciled (split); fs->m_amount = xaccSplitGetAmount (split); @@ -186,6 +186,15 @@ void gnc_float_split_to_split (const FloatingSplit *fs, Split *split) } } +void gnc_float_split_free (FloatingSplit *fs) +{ + g_return_if_fail (fs); + + CACHE_REMOVE (fs->m_memo); + CACHE_REMOVE (fs->m_action); + g_free (fs); +} + /* accessors */ Transaction *gnc_float_txn_get_txn (const FloatingTxn *ft) { @@ -294,25 +303,25 @@ void gnc_float_txn_set_date_posted (FloatingTxn *ft, time64 date_posted) void gnc_float_txn_set_num (FloatingTxn *ft, const char *num) { g_return_if_fail (ft); - ft->m_num = num; + CACHE_REPLACE (ft->m_num, num); }; void gnc_float_txn_set_description (FloatingTxn *ft, const char *description) { g_return_if_fail (ft); - ft->m_description = description; + CACHE_REPLACE (ft->m_description, description); }; void gnc_float_txn_set_notes (FloatingTxn *ft, const char *notes) { g_return_if_fail (ft); - ft->m_notes = notes; + CACHE_REPLACE (ft->m_notes, notes); }; void gnc_float_txn_set_doclink (FloatingTxn *ft, const char *doclink) { g_return_if_fail (ft); - ft->m_doclink = doclink; + CACHE_REPLACE (ft->m_doclink, doclink); }; void gnc_float_txn_set_splits (FloatingTxn *ft, SplitList *splits) @@ -342,11 +351,11 @@ FloatingTxn *gnc_txn_to_float_txn (Transaction *txn, gboolean use_cut_semantics) if (use_cut_semantics) { ft->m_date_posted = xaccTransGetDate (txn); - ft->m_num = xaccTransGetNum (txn); + ft->m_num = CACHE_INSERT (xaccTransGetNum (txn)); } - ft->m_description = xaccTransGetDescription (txn); - ft->m_notes = xaccTransGetNotes (txn); - ft->m_doclink = xaccTransGetDocLink (txn); + ft->m_description = CACHE_INSERT (xaccTransGetDescription (txn)); + ft->m_notes = CACHE_INSERT (xaccTransGetNotes (txn)); + ft->m_doclink = CACHE_INSERT (xaccTransGetDocLink (txn)); for (iter = xaccTransGetSplitList (txn); iter ; iter = iter->next) { @@ -427,3 +436,15 @@ void gnc_float_txn_to_txn_swap_accounts (const FloatingTxn *ft, Transaction *txn if (do_commit) xaccTransCommitEdit (txn); } + +void gnc_float_txn_free (FloatingTxn *ft) +{ + g_return_if_fail (ft); + + CACHE_REMOVE (ft->m_num); + CACHE_REMOVE (ft->m_description); + CACHE_REMOVE (ft->m_notes); + CACHE_REMOVE (ft->m_doclink); + g_list_free_full (ft->m_splits, (GDestroyNotify)gnc_float_split_free); + g_free (ft); +} diff --git a/gnucash/register/ledger-core/split-register-copy-ops.h b/gnucash/register/ledger-core/split-register-copy-ops.h index bbfa694f0d..707353136a 100644 --- a/gnucash/register/ledger-core/split-register-copy-ops.h +++ b/gnucash/register/ledger-core/split-register-copy-ops.h @@ -84,6 +84,8 @@ void gnc_float_split_set_value (FloatingSplit *fs, gnc_numeric value); FloatingSplit *gnc_split_to_float_split (Split *split); void gnc_float_split_to_split (const FloatingSplit *fs, Split *split); +void gnc_float_split_free (FloatingSplit *fs); + /* accessors */ Transaction *gnc_float_txn_get_txn (const FloatingTxn *ft); gnc_commodity *gnc_float_txn_get_currency (const FloatingTxn *ft); @@ -116,4 +118,6 @@ FloatingTxn *gnc_txn_to_float_txn (Transaction *txn, gboolean use_cut_semantics) void gnc_float_txn_to_txn (const FloatingTxn *ft, Transaction *txn, gboolean do_commit); void gnc_float_txn_to_txn_swap_accounts (const FloatingTxn *ft, Transaction *txn, Account *acct1, Account *acct2, gboolean do_commit); +void gnc_float_txn_free (FloatingTxn *ft); + #endif diff --git a/gnucash/register/ledger-core/split-register.c b/gnucash/register/ledger-core/split-register.c index 68e01c549e..0c31a81a86 100644 --- a/gnucash/register/ledger-core/split-register.c +++ b/gnucash/register/ledger-core/split-register.c @@ -100,6 +100,7 @@ gnc_copy_split_onto_split (Split* from, Split* to, gboolean use_cut_semantics) return; gnc_float_split_to_split (fs, to); + gnc_float_split_free (fs); } void @@ -117,6 +118,7 @@ gnc_copy_trans_onto_trans (Transaction* from, Transaction* to, return; gnc_float_txn_to_txn (ft, to, do_commit); + gnc_float_txn_free (ft); } static int @@ -819,9 +821,9 @@ gnc_split_register_copy_current_internal (SplitRegister* reg, /* unprotect the old object, if any */ if (copied_item.ftype == GNC_TYPE_SPLIT) - g_free (copied_item.fs); + gnc_float_split_free (copied_item.fs); if (copied_item.ftype == GNC_TYPE_TRANSACTION) - g_free (copied_item.ft); + gnc_float_txn_free (copied_item.ft); copied_item.ftype = 0; if (new_fs) diff --git a/gnucash/register/ledger-core/test/utest-split-register-copy-ops.c b/gnucash/register/ledger-core/test/utest-split-register-copy-ops.c index 9723f973de..46c8ce9e29 100644 --- a/gnucash/register/ledger-core/test/utest-split-register-copy-ops.c +++ b/gnucash/register/ledger-core/test/utest-split-register-copy-ops.c @@ -276,7 +276,7 @@ test_gnc_split_to_float_split (Fixture *fixture, gconstpointer pData) g_assert_true (gnc_numeric_equal(fs->m_value, xaccSplitGetValue (s))); g_assert_true (gnc_numeric_equal(fs->m_amount, xaccSplitGetAmount (s))); - g_free (fs); + gnc_float_split_free (fs); } /* gnc_float_split_to_split void gnc_float_split_to_split (const FloatingSplit *fs, Split *split)// C: 2 in 1 Local: 1:0:0 @@ -412,9 +412,7 @@ test_gnc_txn_to_float_txn (Fixture *fixture, gconstpointer pData) g_assert_null (fsiter->next); - g_list_free_full(ft->m_splits, g_free); - ft->m_splits = NULL; - g_free (ft); + gnc_float_txn_free (ft); } static void test_gnc_txn_to_float_txn_cut_semantics (Fixture *fixture, gconstpointer pData) @@ -471,9 +469,7 @@ test_gnc_txn_to_float_txn_cut_semantics (Fixture *fixture, gconstpointer pData) g_assert_null (fsiter->next); - g_list_free_full(ft->m_splits, g_free); - ft->m_splits = NULL; - g_free (ft); + gnc_float_txn_free (ft); }