Skip to content

Commit

Permalink
Fix a bunch of UB errors from ASAN about mismatched function types.
Browse files Browse the repository at this point in the history
The casts fool the compiler but not the UB sanitizer.
  • Loading branch information
jralls committed Feb 20, 2024
1 parent 7bd97f1 commit 226bfea
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 51 deletions.
9 changes: 7 additions & 2 deletions libgnucash/engine/Split.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* *
\********************************************************************/

#include "qofinstance.h"
#include <config.h>

#include <platform.h>
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
20 changes: 11 additions & 9 deletions libgnucash/engine/Transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* *
\********************************************************************/

#include "qofinstance.h"
#include <config.h>

#include <platform.h>
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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 ...
Expand All @@ -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;

/* ------------------------------------------------- */
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions libgnucash/engine/TransactionP.h
Original file line number Diff line number Diff line change
Expand Up @@ -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*);

Expand Down
2 changes: 1 addition & 1 deletion libgnucash/engine/gnc-lot.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
33 changes: 2 additions & 31 deletions libgnucash/engine/test/utest-Account.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
{
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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++)
{
Expand All @@ -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);
Expand All @@ -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 ();
*/
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion libgnucash/engine/test/utest-Split.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions libgnucash/engine/test/utest-Transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions po/POTFILES.in
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 226bfea

Please sign in to comment.