Skip to content

Commit

Permalink
Revert "Fix two use-after-free issues found by address sanitizer."
Browse files Browse the repository at this point in the history
This reverts commit 4dbf803.

The use-after free errors are caused by the compiler reordering the
steps in xaccSplitFree and Transaction's do_destroy. Unfortunately the
corrections here caused trouble in other places, leading to test failures.
  • Loading branch information
jralls committed Feb 20, 2024
1 parent 4dbf803 commit 8546aa9
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 13 deletions.
8 changes: 1 addition & 7 deletions libgnucash/engine/Split.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -706,13 +706,7 @@ xaccFreeSplit (Split *split)
}
CACHE_REMOVE(split->memo);
CACHE_REMOVE(split->action);
if (GNC_IS_ACCOUNT (split->acc) && !qof_instance_get_destroying (QOF_INSTANCE (split->acc)))
gnc_account_remove_split (split->acc, split);
if (GNC_IS_LOT (split->lot) && !qof_instance_get_destroying (QOF_INSTANCE (split->lot)))
gnc_lot_remove_split (split->lot, split);
/* We should do the same for split->parent but we might be getting
* called from xaccFreeTransactiob abd tgat would cause trouble.
*/

/* Just in case someone looks up freed memory ... */
split->memo = (char *) 1;
split->action = NULL;
Expand Down
9 changes: 3 additions & 6 deletions libgnucash/engine/Transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1509,19 +1509,15 @@ do_destroy (Transaction *trans)
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. */

auto splits = trans->splits;
trans->splits = NULL;

for (node = splits; node; node = node->next)
for (node = trans->splits; node; node = node->next)
{
Split *s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
{
xaccSplitDestroy(s);
}
}
for (node = splits; node; node = node->next)
for (node = trans->splits; node; node = node->next)
{
Split *s = GNC_SPLIT(node->data);
if (s && s->parent == trans)
Expand All @@ -1530,6 +1526,7 @@ do_destroy (Transaction *trans)
}
}
g_list_free (trans->splits);
trans->splits = NULL;
xaccFreeTransaction (trans);
}

Expand Down

0 comments on commit 8546aa9

Please sign in to comment.