From 226bfea1084646328dd3022cceee7271eb6e4ee5 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Tue, 20 Feb 2024 15:33:11 -0800 Subject: [PATCH] Fix a bunch of UB errors from ASAN about mismatched function types. The casts fool the compiler but not the UB sanitizer. --- libgnucash/engine/Split.cpp | 9 ++++-- 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, 30 insertions(+), 51 deletions(-) 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 #include @@ -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 #include @@ -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(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(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