Skip to content

Commit

Permalink
bugfix xaccTransGetTxnType: avoid returning TXN_TYPE_LINK incorrectly
Browse files Browse the repository at this point in the history 
A TXN_TYPE_PAYMENT will have non-APAR splits; a TXN_TYPE_LINK will not
have non-APAR splits. This bug manifests as a regular TXN_TYPE_PAYMENT
transaction being later voided being incorrectly changed to
TXN_TYPE_LINK.
  • Loading branch information
@christopherlam
christopherlam committed Jun 14, 2023
1 parent 853791c commit 4a60e01
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
9 changes: 4 additions & 5 deletions libgnucash/engine/Transaction.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2484,7 +2484,7 @@ xaccTransRetDateDue(const Transaction *trans)
char
xaccTransGetTxnType (Transaction *trans)
{
gboolean has_nonAPAR_amount = FALSE;
gboolean has_nonAPAR_split = FALSE;

if (!trans) return TXN_TYPE_NONE;

Expand All @@ -2499,9 +2499,8 @@ xaccTransGetTxnType (Transaction *trans)
if (!acc)
continue;

if (!xaccAccountIsAPARType (xaccAccountGetType (acc)) &&
!gnc_numeric_zero_p (xaccSplitGetValue (n->data)))
has_nonAPAR_amount = TRUE;
if (!xaccAccountIsAPARType (xaccAccountGetType (acc)))
has_nonAPAR_split = TRUE;
else if (trans->txn_type == TXN_TYPE_NONE)
{
GNCLot *lot = xaccSplitGetLot (n->data);
Expand All @@ -2515,7 +2514,7 @@ xaccTransGetTxnType (Transaction *trans)
}
}

if (!has_nonAPAR_amount && (trans->txn_type == TXN_TYPE_PAYMENT))
if (!has_nonAPAR_split && (trans->txn_type == TXN_TYPE_PAYMENT))
trans->txn_type = TXN_TYPE_LINK;

return trans->txn_type;
Expand Down
6 changes: 6 additions & 0 deletions libgnucash/engine/test/utest-Invoice.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ test_xaccTransGetTxnTypeInvoice (Fixture *fixture, gconstpointer pData)
g_assert_cmpint (TXN_TYPE_INVOICE, ==, xaccTransGetTxnType (fixture->trans));

g_assert_cmpint (TXN_TYPE_PAYMENT, ==, xaccTransGetTxnType (fixture->trans2));

xaccTransVoid (fixture->trans2, "Cancel payment");

g_assert_cmpint (TXN_TYPE_PAYMENT, ==, xaccTransGetTxnType (fixture->trans2));

xaccTransUnvoid (fixture->trans2);
}


Expand Down

6 comments on commit 4a60e01

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW the xaccTransUnvoid does not seem important in this unit test. However without this line, the teardown function segfaults.

@jralls
Copy link
Member

@jralls jralls commented on 4a60e01 Jun 14, 2023

Choose a reason for hiding this comment

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

Segfaults on what?

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok 57 /engine/gncInvoice/post trans - vendor bill
ok 58 /engine/gncInvoice/post trans - vendor credit note
ok 59 /engine/gncInvoice/post trans - customer creditnote
ok 60 /engine/gncInvoice/post trans - customer invoice
not ok /engine/gncInvoice/tests txntype I & P - GLib-GObject-FATAL-CRITICAL: invalid unclassed pointer in cast to 'QofInstance'
Bail out!

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff76324f1 in g_logv () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
(gdb) bt
#0  0x00007ffff76324f1 in g_logv () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#1  0x00007ffff76327a3 in g_log () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007ffff75ab7ea in g_type_check_instance_cast () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
#3  0x00007ffff7d1f1a3 in gnc_lot_begin_edit (lot=0x5555556c1630) at /home/chris/sources/gnucash/stable/libgnucash/engine/gnc-lot.c:322
#4  0x00007ffff7d20110 in gnc_lot_remove_split (lot=0x5555556c1630, split=0x5555556c3af0)
    at /home/chris/sources/gnucash/stable/libgnucash/engine/gnc-lot.c:653
#5  0x00007ffff7ca8065 in xaccSplitCommitEdit (s=0x5555556c3af0) at /home/chris/sources/gnucash/stable/libgnucash/engine/Split.c:982
#6  0x00007ffff7cb0390 in trans_cleanup_commit (trans=0x5555556c27e0) at /home/chris/sources/gnucash/stable/libgnucash/engine/Transaction.c:1608
#7  0x00007ffff7e30d68 in qof_commit_edit_part2(QofInstance*, void (*)(QofInstance*, QofBackendError), void (*)(QofInstance*), void (*)(QofInstance*))
    (inst=0x5555556c27e0, on_error=0x7ffff7cb01ea <trans_on_error>, on_done=0x7ffff7cb0251 <trans_cleanup_commit>, on_free=0x7ffff7cb000f <do_destroy>)
    at /home/chris/sources/gnucash/stable/libgnucash/engine/qofinstance.cpp:1039
#8  0x00007ffff7cb072b in xaccTransCommitEdit (trans=0x5555556c27e0) at /home/chris/sources/gnucash/stable/libgnucash/engine/Transaction.c:1691
#9  0x00007ffff7ca9ad5 in xaccSplitDestroy (split=0x5555556c3c70) at /home/chris/sources/gnucash/stable/libgnucash/engine/Split.c:1484
#10 0x00007ffff7c6e66a in xaccAccountCommitEdit(Account*) (acc=0x5555556bb910) at /home/chris/sources/gnucash/stable/libgnucash/engine/Account.cpp:1511
#11 0x00007ffff7c6e860 in xaccAccountDestroy(Account*) (acc=0x5555556bb910) at /home/chris/sources/gnucash/stable/libgnucash/engine/Account.cpp:1562
#12 0x000055555558e2a7 in teardown (fixture=0x5555556c2220, pData=0x5555555dae60 <pData.0>)
    at /home/chris/sources/gnucash/stable/libgnucash/engine/test/utest-Invoice.c:97
#13 0x000055555558ebf7 in teardown_with_invoice (fixture=0x5555556c2220, pData=0x5555555dae60 <pData.0>)
    at /home/chris/sources/gnucash/stable/libgnucash/engine/test/utest-Invoice.c:282
#14 0x00007ffff765c6d3 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#15 0x00007ffff765c38b in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#16 0x00007ffff765c38b in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00007ffff765cbca in g_test_run_suite () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007ffff7656ffd in g_test_run () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#19 0x0000555555575256 in main (argc=1, argv=0x7fffffffdb58) at /home/chris/sources/gnucash/stable/libgnucash/engine/test/test-engine.c:61
(gdb) 
(gdb) f 5
#5  0x00007ffff7ca8065 in xaccSplitCommitEdit (s=0x5555556c3af0) at /home/chris/sources/gnucash/stable/libgnucash/engine/Split.c:982
982	        gnc_lot_remove_split (s->lot, s);
(gdb) p s->lot
$1 = 0x5555556c1630
(gdb)

@jralls
Copy link
Member

@jralls jralls commented on 4a60e01 Jun 15, 2023

Choose a reason for hiding this comment

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

I think what's happening is that teardown destroys the invoice at line 94, which frees the lot but doesn't remove the split's copy of the pointer. Line 97 destroys the account containing the split, which eventually calls xaccSplitCommitEdit. That wants its lot* to remove the split, but the lot has been freed and you get the crash. I guess that xaccSplitUnvoid is getting the split removed and the split's lot* nulled before cleanup, avoiding the crash.

Simplest fix would be to destroy the invoice at the end instead of the beginning of teardown.

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplest fix would be to destroy the invoice at the end instead of the beginning of teardown.

And the best fix would be to let RAII do it lol

@jralls
Copy link
Member

@jralls jralls commented on 4a60e01 Jun 15, 2023

Choose a reason for hiding this comment

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

And the best fix would be to let RAII do it lol

Maybe, but using RAII requires a complete re-implementation of the engine including creating an actual memory model. We need to first get rid of QofQuery and all of the crap that goes along with it.

Please sign in to comment.