IntenseFinance/gnucash
3
0
Fork 0
mirror of https://github.com/Gnucash/gnucash.git synced 2025-02-16 18:25:11 -06:00
Code Issues Packages Projects Releases Wiki Activity

Bug 799213 - SIGSEGV caused by revising an auto completed transaction

Browse Source 
Calling xaccSplitDestroy without also calling xaccSplitCommitEdit then
deleting the split list before calling xaccTransCommitEdit prevents
xaccSplitCommitEdit from being called on the supposedly deleted
splits. Not only does this leak them it leaves them in the book
potentially with a dangling parent pointer.
This commit is contained in:
John Ralls 2024-02-29 14:29:40 -08:00
parent d011f06481
commit 8ebac5b596
1 changed files with 31 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

64
libgnucash/engine/Transaction.cpp
View File

@ -25,6 +25,7 @@
\********************************************************************/
#include "qofinstance.h"
#include <__nullptr>
#include <config.h>
#include <platform.h>
@ -754,10 +755,7 @@ xaccTransCopyFromClipBoard(const Transaction *from_trans, Transaction *to_trans,
change_accounts = from_acc && GNC_IS_ACCOUNT(to_acc) && from_acc != to_acc;
xaccTransBeginEdit(to_trans);
FOR_EACH_SPLIT(to_trans, xaccSplitDestroy(s));
g_list_free(to_trans->splits);
to_trans->splits = NULL;
xaccTransClearSplits(to_trans);
xaccTransSetCurrency(to_trans, xaccTransGetCurrency(from_trans));
xaccTransSetDescription(to_trans, xaccTransGetDescription(from_trans));
@ -1489,7 +1487,6 @@ static void
do_destroy (QofInstance* inst)
{
Transaction *trans{GNC_TRANSACTION (inst)};
SplitList *node;
gboolean shutting_down = qof_book_shutting_down(qof_instance_get_book(trans));
/* If there are capital-gains transactions associated with this,
@ -1503,32 +1500,10 @@ do_destroy (QofInstance* inst)
xaccTransWriteLog (trans, 'D');
qof_event_gen (&trans->inst, QOF_EVENT_DESTROY, NULL);
/* We only own the splits that still think they belong to us. This is done
in 2 steps. In the first, the splits are marked as being destroyed, but they
are not destroyed yet. In the second, the destruction is committed which will
do the actual destruction. If both steps are done for a split before they are
done for the next split, then a split will still be on the split list after it
has been freed. This can cause other parts of the code (e.g. in xaccSplitDestroy())
to reference the split after it has been freed. */
for (node = trans->splits; node; node = node->next)
{
Split *s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
{
xaccSplitDestroy(s);
}
}
for (node = trans->splits; node; node = node->next)
{
Split *s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
{
xaccSplitCommitEdit(s);
}
}
g_list_free (trans->splits);
trans->splits = NULL;
/* xaccFreeTransaction will also clean up the splits but without
* emitting GNC_EVENT_ITEM_REMOVED.
*/
xaccTransClearSplits(trans);
xaccFreeTransaction (trans);
}
@ -2232,9 +2207,32 @@ void
xaccTransClearSplits(Transaction* trans)
{
xaccTransBeginEdit(trans);
FOR_EACH_SPLIT(trans, xaccSplitDestroy(s));
/* We only own the splits that still think they belong to us. This is done
in 2 steps. In the first, the splits are marked as being destroyed, but they
are not destroyed yet. In the second, the destruction is committed which will
do the actual destruction. If both steps are done for a split before they are
done for the next split, then a split will still be on the split list after it
has been freed. This can cause other parts of the code (e.g. in xaccSplitDestroy())
to reference the split after it has been freed. */
for (auto node = trans->splits; node; node = node->next)
{
auto s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
{
xaccSplitDestroy(s);
}
}
for (auto node = trans->splits; node; node = node->next)
{
auto s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
{
xaccSplitCommitEdit(s);
}
}
g_list_free (trans->splits);
trans->splits = NULL;
trans->splits = nullptr;
xaccTransCommitEdit(trans);
}