Skip to content

Commit

Permalink
Bug 799528 - Crash on account deletion (edit)
Browse files Browse the repository at this point in the history 
New function xaccAccountDeleteAllTransactions.

Delete all transactions before deleting the account; simply deleting the
splits during account destruction isn't safe. In the particular case of an
imbalance account the transaction commit after deleting a split just makes
a new one.
  • Loading branch information
@jralls
jralls committed Mar 4, 2025
1 parent f3b08dd commit bc7fafd
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 0 deletions.
11 changes: 11 additions & 0 deletions gnucash/gnome/gnc-plugin-page-account-tree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1694,11 +1694,22 @@ do_delete_account (Account* account, Account* saa, Account* sta, Account* ta)
(AccountCb)xaccAccountMoveAllSplits,
sta);
}
else
{
gnc_account_foreach_descendant (account,
[](auto acc, [[maybe_unused]] auto data)
{ xaccAccountDestroyAllTransactions(acc); },
nullptr);
}
if (ta)
{
/* Move the splits of the account to be deleted. */
xaccAccountMoveAllSplits (account, ta);
}
else
{
xaccAccountDestroyAllTransactions (account);
}
xaccAccountCommitEdit (account);

/* Drop all references from the state file for
Expand Down
13 changes: 13 additions & 0 deletions libgnucash/engine/Account.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include <numeric>
#include <map>
#include <unordered_set>
#include <algorithm>

static QofLogModule log_module = GNC_MOD_ACCOUNT;

Expand Down Expand Up @@ -1596,6 +1597,18 @@ xaccAccountDestroy (Account *acc)
xaccAccountCommitEdit (acc);
}

void
xaccAccountDestroyAllTransactions(Account *acc)
{
auto priv = GET_PRIVATE(acc);
std::vector<Transaction*> transactions;
std::transform(priv->splits.begin(), priv->splits.end(),
back_inserter(transactions),
[](auto split) { return split->parent; });
std::for_each(transactions.rbegin(), transactions.rend(),
[](auto trans) { xaccTransDestroy (trans); });
}

/********************************************************************\
\********************************************************************/

Expand Down
4 changes: 4 additions & 0 deletions libgnucash/engine/Account.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ typedef enum
* (by calling xaccAccountBeginEdit()) before calling this routine.*/
void xaccAccountDestroy (Account *account);

/** Destroy all of the transactions that parent splits in an account.
*/
void xaccAccountDestroyAllTransactions(Account *acc);

/** Compare two accounts for equality - this is a deep compare. */
gboolean xaccAccountEqual(const Account *a, const Account* b,
gboolean check_guids);
Expand Down

5 comments on commit bc7fafd

@christopherlam
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per irc... how about the following? I think priv->splits will always be trimmed from the end properly when the last trans is destroyed?

void
xaccAccountDestroyAllTransactions(Account *acc)
{
    g_return_if_fail (GNC_IS_ACCOUNT (acc));
    auto priv = GET_PRIVATE(acc);
    while (!priv->splits.empty())
        xaccTransDestroy (priv->splits.back()->parent);
}

@jralls
Copy link
Member Author

@jralls jralls commented on bc7fafd Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the same UAF problem an invalidated iterators problem (because we can't trust the compiler to optimize that in a safe way) if there are two splits from the same transaction. I think the only way short of rewriting all of the QofInstance memory management (we'll get there one of these years…) is to deduplicate the transactions. Two easy ways are to use a std::unordered_map<GUID, Transaction*> transactions; or std::unique(std:sort(transactions.begin(), transactions.end())); //pseudocode, that wouldn't compile. I was setting those up for profiling when I got distracted with bug 799565.

@christopherlam
Copy link
Contributor

@christopherlam christopherlam commented on bc7fafd Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

void
xaccAccountDestroyAllTransactions(Account *acc)
{
    auto priv = GET_PRIVATE(acc);
    std::vector<Transaction*> transactions;
    std::transform(priv->splits.begin(), priv->splits.end(),
                   back_inserter(transactions),
                   [](auto split) { return split->parent; });
    std::sort (transactions.begin(), transactions.end(),
               [](auto a, auto b){ return xaccTransOrder (a, b) < 0; });
    transactions.erase (std::unique (transactions.begin(), transactions.end()), transactions.end());
    std::for_each(transactions.rbegin(), transactions.rend(),
                  [](auto trans) { xaccTransDestroy (trans); });
}

@jralls
Copy link
Member Author

@jralls jralls commented on bc7fafd Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's the dedupe approach. I think that std::sort(transactions.begin(), transactions.end()) is sufficient: It will sort based on the pointer addresses. Since the only reason for the sort is to get the duplicates adjacent so that std::unique will work there's no need to resort to xaccTransOrder or even xaccTransGetGUID (which is a lot cheaper than xaccTransOrder).

@jralls
Copy link
Member Author

@jralls jralls commented on bc7fafd Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deduplicating the vector took no measurable time with several thousand transactions, so no point in bothering with other methods.

Please sign in to comment.