gnucash stable: Multiple changes pushed
John Ralls
jralls at code.gnucash.org
Tue Feb 20 19:12:00 EST 2024
Updated via https://github.com/Gnucash/gnucash/commit/226bfea1 (commit)
via https://github.com/Gnucash/gnucash/commit/7bd97f15 (commit)
from https://github.com/Gnucash/gnucash/commit/8546aa97 (commit)
commit 226bfea1084646328dd3022cceee7271eb6e4ee5
Author: John Ralls <jralls at ceridwen.us>
Date: Tue Feb 20 15:33:11 2024 -0800
Fix a bunch of UB errors from ASAN about mismatched function types.
The casts fool the compiler but not the UB sanitizer.
diff --git a/libgnucash/engine/Split.cpp b/libgnucash/engine/Split.cpp
index 19797df438..656adb43a3 100644
--- a/libgnucash/engine/Split.cpp
+++ b/libgnucash/engine/Split.cpp
@@ -24,6 +24,7 @@
* *
\********************************************************************/
+#include "qofinstance.h"
#include <config.h>
#include <platform.h>
@@ -692,6 +693,11 @@ xaccSplitDump (const Split *split, const char *tag)
/********************************************************************\
\********************************************************************/
+static void
+do_destroy (QofInstance *inst)
+{
+ xaccFreeSplit (GNC_SPLIT (inst));
+}
void
xaccFreeSplit (Split *split)
@@ -1044,8 +1050,7 @@ xaccSplitCommitEdit(Split *s)
original and new transactions, for the _next_ begin/commit cycle. */
s->orig_acc = s->acc;
s->orig_parent = s->parent;
- if (!qof_commit_edit_part2(QOF_INSTANCE(s), commit_err, NULL,
- (void (*) (QofInstance *)) xaccFreeSplit))
+ if (!qof_commit_edit_part2(QOF_INSTANCE(s), commit_err, NULL, do_destroy))
return;
if (acc)
diff --git a/libgnucash/engine/Transaction.cpp b/libgnucash/engine/Transaction.cpp
index 899adaa9d0..4dc9afd30f 100644
--- a/libgnucash/engine/Transaction.cpp
+++ b/libgnucash/engine/Transaction.cpp
@@ -24,6 +24,7 @@
* *
\********************************************************************/
+#include "qofinstance.h"
#include <config.h>
#include <platform.h>
@@ -1485,8 +1486,9 @@ destroy_gains (Transaction *trans)
}
static void
-do_destroy (Transaction *trans)
+do_destroy (QofInstance* inst)
{
+ Transaction *trans{GNC_TRANSACTION (inst)};
SplitList *node;
gboolean shutting_down = qof_book_shutting_down(qof_instance_get_book(trans));
@@ -1551,8 +1553,10 @@ static gboolean was_trans_emptied(Transaction *trans)
return TRUE;
}
-static void trans_on_error(Transaction *trans, QofBackendError errcode)
+static void trans_on_error(QofInstance *inst, QofBackendError errcode)
{
+ Transaction *trans{GNC_TRANSACTION(inst)};
+
/* If the backend puked, then we must roll-back
* at this point, and let the user know that we failed.
* The GUI should check for error conditions ...
@@ -1568,8 +1572,9 @@ static void trans_on_error(Transaction *trans, QofBackendError errcode)
gnc_engine_signal_commit_error( errcode );
}
-static void trans_cleanup_commit(Transaction *trans)
+static void trans_cleanup_commit(QofInstance *inst)
{
+ Transaction *trans{GNC_TRANSACTION(inst)};
GList *slist, *node;
/* ------------------------------------------------- */
@@ -1684,11 +1689,8 @@ xaccTransCommitEdit (Transaction *trans)
}
trans->txn_type = TXN_TYPE_UNCACHED;
- qof_commit_edit_part2(QOF_INSTANCE(trans),
- (void (*) (QofInstance *, QofBackendError))
- trans_on_error,
- (void (*) (QofInstance *)) trans_cleanup_commit,
- (void (*) (QofInstance *)) do_destroy);
+ qof_commit_edit_part2(QOF_INSTANCE(trans), trans_on_error,
+ trans_cleanup_commit, do_destroy);
LEAVE ("(trans=%p)", trans);
}
@@ -1829,7 +1831,7 @@ xaccTransRollbackEdit (Transaction *trans)
* out about it until this user tried to edit it.
*/
xaccTransDestroy (trans);
- do_destroy (trans);
+ do_destroy (QOF_INSTANCE(trans));
/* push error back onto the stack */
qof_backend_set_error (be, errcode);
diff --git a/libgnucash/engine/TransactionP.h b/libgnucash/engine/TransactionP.h
index 015914a19f..6493fe96a4 100644
--- a/libgnucash/engine/TransactionP.h
+++ b/libgnucash/engine/TransactionP.h
@@ -184,10 +184,10 @@ typedef struct
void (*gen_event_trans)(Transaction*);
void (*xaccFreeTransaction)(Transaction*);
void (*destroy_gains)(Transaction*);
- void (*do_destroy)(Transaction*);
+ void (*do_destroy)(QofInstance*);
gboolean (*was_trans_emptied)(Transaction*);
- void (*trans_on_error)(Transaction*, QofBackendError);
- void (*trans_cleanup_commit)(Transaction*);
+ void (*trans_on_error)(QofInstance*, QofBackendError);
+ void (*trans_cleanup_commit)(QofInstance*);
void (*xaccTransScrubGainsDate)(Transaction*);
Transaction *(*dupe_trans)(const Transaction*);
diff --git a/libgnucash/engine/gnc-lot.cpp b/libgnucash/engine/gnc-lot.cpp
index 15457f20df..22f1012535 100644
--- a/libgnucash/engine/gnc-lot.cpp
+++ b/libgnucash/engine/gnc-lot.cpp
@@ -656,7 +656,7 @@ gnc_lot_remove_split (GNCLot *lot, Split *split)
xaccSplitSetLot(split, NULL);
priv->is_closed = LOT_CLOSED_UNKNOWN; /* force an is-closed computation */
- if (NULL == priv->splits)
+ if (!priv->splits && priv->account)
{
xaccAccountRemoveLot (priv->account, lot);
priv->account = NULL;
diff --git a/libgnucash/engine/test/utest-Account.cpp b/libgnucash/engine/test/utest-Account.cpp
index d9e2d70901..8a29680510 100644
--- a/libgnucash/engine/test/utest-Account.cpp
+++ b/libgnucash/engine/test/utest-Account.cpp
@@ -858,29 +858,18 @@ test_xaccFreeAccount (Fixture *fixture, gconstpointer pData)
{
auto msg1 = "[xaccFreeAccount()] instead of calling xaccFreeAccount(), please call\n"
" xaccAccountBeginEdit(); xaccAccountDestroy();\n";
-#ifdef USE_CLANG_FUNC_SIG
-#define _func "int xaccTransGetSplitIndex(const Transaction *, const Split *)"
-#else
-#define _func "int xaccTransGetSplitIndex(const Transaction*, const Split*)"
-#endif
- auto msg2 = _func ": assertion 'trans && split' failed";
-#undef _func
auto loglevel = static_cast<GLogLevelFlags>(G_LOG_LEVEL_CRITICAL | G_LOG_FLAG_FATAL);
auto check1 = test_error_struct_new("gnc.account", loglevel, msg1);
- auto check2 = test_error_struct_new("gnc.engine", loglevel, msg2);
QofBook *book = gnc_account_get_book (fixture->acct);
Account *parent = gnc_account_get_parent (fixture->acct);
AccountPrivate *p_priv = fixture->func->get_private (parent);
const guint numItems = 3;
guint i = 0;
- guint hdlr1, hdlr2;
+ guint hdlr1;
gnc_commodity *commodity = gnc_commodity_new (book, "US Dollar", "CURRENCY", "USD", "0", 100);
test_add_error (check1);
- test_add_error (check2);
hdlr1 = g_log_set_handler ("gnc.account", loglevel,
(GLogFunc)test_checked_handler, check1);
- hdlr2 = g_log_set_handler ("gnc.engine", loglevel,
- (GLogFunc)test_checked_handler, check2);
g_test_log_set_fatal_handler ((GTestLogFatalFunc)test_list_handler, NULL);
for (i = 0; i < numItems; i++)
{
@@ -897,7 +886,6 @@ test_xaccFreeAccount (Fixture *fixture, gconstpointer pData)
g_assert_true (p_priv->parent != NULL);
g_assert_true (p_priv->commodity != NULL);
g_assert_cmpint (check1->hits, ==, 0);
- g_assert_cmpint (check2->hits, ==, 0);
/* Now set the other private parts to something so that they can be set back */
p_priv->cleared_balance = gnc_numeric_create ( 5, 12);
p_priv->reconciled_balance = gnc_numeric_create ( 5, 12);
@@ -906,10 +894,8 @@ test_xaccFreeAccount (Fixture *fixture, gconstpointer pData)
p_priv->sort_dirty = TRUE;
fixture->func->xaccFreeAccount (parent);
g_assert_cmpint (check1->hits, ==, 6);
- g_assert_cmpint (check2->hits, ==, 6);
/* cleanup what's left */
g_log_remove_handler ("gnc.account", hdlr1);
- g_log_remove_handler ("gnc.engine", hdlr2);
test_clear_error_list ();
qof_book_destroy (book);
g_free (fixture->func);
@@ -972,17 +958,9 @@ test_xaccAccountCommitEdit (Fixture *fixture, gconstpointer pData)
{
auto msg1 = "[xaccFreeAccount()] instead of calling xaccFreeAccount(), please call\n"
" xaccAccountBeginEdit(); xaccAccountDestroy();\n";
-#ifdef USE_CLANG_FUNC_SIG
-#define _func "int xaccTransGetSplitIndex(const Transaction *, const Split *)"
-#else
-#define _func "int xaccTransGetSplitIndex(const Transaction*, const Split*)"
-#endif
- auto msg2 = _func ": assertion 'trans && split' failed";
-#undef _func
auto loglevel = static_cast<GLogLevelFlags>(G_LOG_LEVEL_CRITICAL | G_LOG_FLAG_FATAL);
auto check1 = test_error_struct_new("gnc.account", loglevel, msg1);
- auto check2 = test_error_struct_new("gnc.engine", loglevel, msg2);
- guint hdlr1, hdlr2;
+ guint hdlr1;
TestSignal sig1, sig2;
QofBook *book = gnc_account_get_book (fixture->acct);
Account *parent = gnc_account_get_parent (fixture->acct);
@@ -991,11 +969,8 @@ test_xaccAccountCommitEdit (Fixture *fixture, gconstpointer pData)
guint i = 0;
gnc_commodity *commodity = gnc_commodity_new (book, "US Dollar", "CURRENCY", "USD", "0", 100);
test_add_error (check1);
- test_add_error (check2);
hdlr1 = g_log_set_handler ("gnc.account", loglevel,
(GLogFunc)test_checked_handler, check1);
- hdlr2 = g_log_set_handler ("gnc.engine", loglevel,
- (GLogFunc)test_checked_handler, check2);
g_test_log_set_fatal_handler ((GTestLogFatalFunc)test_list_handler, NULL);
for (i = 0; i < numItems; i++)
{
@@ -1012,7 +987,6 @@ test_xaccAccountCommitEdit (Fixture *fixture, gconstpointer pData)
g_assert_true (p_priv->parent != NULL);
g_assert_true (p_priv->commodity != NULL);
g_assert_cmpint (check1->hits, ==, 0);
- g_assert_cmpint (check2->hits, ==, 0);
sig1 = test_signal_new (&parent->inst, QOF_EVENT_MODIFY, NULL);
sig2 = test_signal_new (&parent->inst, QOF_EVENT_DESTROY, NULL);
@@ -1028,7 +1002,6 @@ test_xaccAccountCommitEdit (Fixture *fixture, gconstpointer pData)
g_assert_true (p_priv->parent != NULL);
g_assert_true (p_priv->commodity != NULL);
g_assert_cmpint (check1->hits, ==, 0);
- g_assert_cmpint (check2->hits, ==, 0);
/* xaccAccountDestroy destroys the account by calling
* qof_instance_set_destroying (), then xaccAccountCommitEdit ();
*/
@@ -1038,12 +1011,10 @@ test_xaccAccountCommitEdit (Fixture *fixture, gconstpointer pData)
test_signal_assert_hits (sig1, 2);
test_signal_assert_hits (sig2, 1);
g_assert_cmpint (check1->hits, ==, 2);
- g_assert_cmpint (check2->hits, ==, 12);
/* And clean up */
test_signal_free (sig1);
test_signal_free (sig2);
g_log_remove_handler ("gnc.account", hdlr1);
- g_log_remove_handler ("gnc.engine", hdlr2);
test_clear_error_list ();
qof_book_destroy (book);
g_free (fixture->func);
diff --git a/libgnucash/engine/test/utest-Split.cpp b/libgnucash/engine/test/utest-Split.cpp
index a1535ba79d..d0293184fe 100644
--- a/libgnucash/engine/test/utest-Split.cpp
+++ b/libgnucash/engine/test/utest-Split.cpp
@@ -75,7 +75,7 @@ setup (Fixture *fixture, gconstpointer pData)
xaccTransSetCurrency (txn, fixture->curr);
xaccSplitSetParent (fixture->split, txn);
xaccTransCommitEdit (txn);
- gnc_lot_set_account (lot, acc);
+ xaccAccountInsertLot (acc, lot);
fixture->split->action = CACHE_INSERT ("foo");
fixture->split->memo = CACHE_INSERT ("bar");
fixture->split->acc = acc;
diff --git a/libgnucash/engine/test/utest-Transaction.cpp b/libgnucash/engine/test/utest-Transaction.cpp
index e3f31fdac8..ff3ebc344c 100644
--- a/libgnucash/engine/test/utest-Transaction.cpp
+++ b/libgnucash/engine/test/utest-Transaction.cpp
@@ -1419,7 +1419,7 @@ test_do_destroy (GainsFixture *fixture, gconstpointer pData)
/* Protect against recursive calls to do_destroy from xaccTransCommitEdit */
xaccTransBeginEdit(base->txn);
- base->func->do_destroy (base->txn);
+ base->func->do_destroy (QOF_INSTANCE(base->txn));
g_assert_cmpint (test_signal_return_hits (sig), ==, 1);
g_assert_true (base->txn->description == NULL);
g_assert_cmpint (GPOINTER_TO_INT(base->txn->num), ==, 1);
@@ -1472,7 +1472,7 @@ test_trans_on_error (Fixture *fixture, gconstpointer pData)
gnc_engine_add_commit_error_callback ((EngineCommitErrorCallback)commit_error_cb, NULL);
xaccTransBeginEdit (fixture->txn);
g_assert_cmpint (qof_instance_get_editlevel (fixture->txn), ==, 1);
- fixture->func->trans_on_error (fixture->txn, errcode);
+ fixture->func->trans_on_error (QOF_INSTANCE(fixture->txn), errcode);
g_assert_cmpint (check->hits, ==, 1);
g_assert_cmpint ((guint)errorvalue, ==, (guint)errcode);
g_assert_cmpint (qof_instance_get_editlevel (fixture->txn), ==, 0);
@@ -1515,7 +1515,7 @@ test_trans_cleanup_commit (Fixture *fixture, gconstpointer pData)
/*Reverse the splits list so we can check later that it got sorted */
fixture->txn->splits = g_list_reverse (fixture->txn->splits);
g_assert_true (fixture->txn->splits->data != split0);
- fixture->func->trans_cleanup_commit (fixture->txn);
+ fixture->func->trans_cleanup_commit (QOF_INSTANCE(fixture->txn));
g_assert_cmpint (test_signal_return_hits (sig_d_remove), ==, 1);
g_assert_cmpint (test_signal_return_hits (sig_b_remove), ==, 1);
@@ -1540,7 +1540,7 @@ test_trans_cleanup_commit (Fixture *fixture, gconstpointer pData)
fixture->txn->orig = orig;
orig->num = fixture->txn->num;
g_object_ref (orig);
- fixture->func->trans_cleanup_commit (fixture->txn);
+ fixture->func->trans_cleanup_commit (QOF_INSTANCE(fixture->txn));
g_assert_cmpint (test_signal_return_hits (sig_d_remove), ==, 2);
g_assert_cmpint (test_signal_return_hits (sig_b_remove), ==, 1);
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 7a45a22348..3ecbc9aee3 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -656,6 +656,7 @@ libgnucash/engine/gnc-option-impl.cpp
libgnucash/engine/gncOrder.c
libgnucash/engine/gncOwner.c
libgnucash/engine/gnc-pricedb.cpp
+libgnucash/engine/gnc-quote-source.cpp
libgnucash/engine/gnc-rational.cpp
libgnucash/engine/gnc-session.c
libgnucash/engine/gncTaxTable.c
commit 7bd97f15d0fdbd53e752c173d616bf032810e57b
Author: John Ralls <jralls at ceridwen.us>
Date: Tue Feb 20 15:10:05 2024 -0800
Fix transaction delete use-after-free, take 2.
The problem with take 1 was that the duplicate split uses the same lot
and account pointers without adding itself to those lists, causing
checks in unit tests to fail.
diff --git a/libgnucash/engine/Split.cpp b/libgnucash/engine/Split.cpp
index 2da0c51b9f..19797df438 100644
--- a/libgnucash/engine/Split.cpp
+++ b/libgnucash/engine/Split.cpp
@@ -707,6 +707,19 @@ xaccFreeSplit (Split *split)
CACHE_REMOVE(split->memo);
CACHE_REMOVE(split->action);
+ if (split->inst.e_type) /* Don't do this for dupe splits. */
+ {
+ /* gnc_lot_remove_split needs the account, so do it first. */
+ if (GNC_IS_LOT (split->lot) && !qof_instance_get_destroying (QOF_INSTANCE (split->lot)))
+ gnc_lot_remove_split (split->lot, split);
+ if (GNC_IS_ACCOUNT (split->acc)
+ && !qof_instance_get_destroying (QOF_INSTANCE (split->acc)))
+ gnc_account_remove_split (split->acc, split);
+ /* We should do the same for split->parent but we might be getting
+ * called from xaccFreeTransaction and that would cause trouble.
+ */
+ }
+
/* Just in case someone looks up freed memory ... */
split->memo = (char *) 1;
split->action = NULL;
Summary of changes:
libgnucash/engine/Split.cpp | 22 +++++++++++++++++--
libgnucash/engine/Transaction.cpp | 20 +++++++++--------
libgnucash/engine/TransactionP.h | 6 ++---
libgnucash/engine/gnc-lot.cpp | 2 +-
libgnucash/engine/test/utest-Account.cpp | 33 ++--------------------------
libgnucash/engine/test/utest-Split.cpp | 2 +-
libgnucash/engine/test/utest-Transaction.cpp | 8 +++----
po/POTFILES.in | 1 +
8 files changed, 43 insertions(+), 51 deletions(-)
More information about the gnucash-changes
mailing list