Bug 422561 - A half-empty PRICEPAIR causes app to crash when attempting to update prices
Summary: A half-empty PRICEPAIR causes app to crash when attempting to update prices
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: git (master)
Platform: unspecified All
: NOR normal (vote)
Target Milestone: ---
Assignee: Dawid Wróbel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-07 02:10 UTC by Dawid Wróbel
Modified: 2021-06-02 16:25 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In: 5.1.2


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dawid Wróbel 2020-06-07 02:10:11 UTC
SUMMARY
My price updating dialog crashes each time I try to update any currency by hand. It's been like this since I migrated from GnuCash, except I didn't debug it until now.

I initially pinpointed it to the index out of range exception caused by the KEquityPriceUpdateDlg::addInvestment() attempting to split the ID by ' ' without double-checking for trailing spaces.

Adding that check, however, doesn't fix the underlying issue and only prevents the crash from happening. 
Tracing the issue upstream the stack trace I was able to find that:

1) KEquityPriceUpdateDlgPrivate::addPricePair() is not checking whether any of the first or second price symbols in MyMoneySecurityPair object are empty
2) KEquityPriceUpdateDlgPrivate::init() can be initiated with an empty securityId (e.g. when but this is very hacky and not clear it is even supported until the "file->security(pair.first).isCurrency() && (securityId.isEmpty()" expression
3) That expression only checks whether the first item of the pair is a currency and so it continues to run even if the second item of the pair is empty. 
4) The actual underlying cause is my .kmm file somehow got the following added:

  <PRICEPAIR from="PLN" to="">
   <PRICE source="Transaction" date="2013-11-12" price="1/1"/>
   <PRICE source="Transaction" date="2014-02-03" price="1/1"/>
   <PRICE source="Transaction" date="2019-06-28" price="1/1"/>
   <PRICE source="Transaction" date="2019-10-04" price="1/1"/>
   <PRICE source="KMyMoney Currency" date="2020-03-20" price="1/1"/>
  </PRICEPAIR>

The first 5 prices were most likely imported as such from GnuCash. The latter, however, was clearly added by KMyMoney Currency, so this still can be further traced back to some dodgy behavior somewhere else in the app.
STEPS TO REPRODUCE
1. Add a following PRICEPAIR xml element to your statement file
2. Open the Tools->Update Stock and Currency prices
3. Update any currency, close the dialog

OBSERVED RESULT
The app crashes


EXPECTED RESULT
1) The MyMoneySecurityPair type should not allow empty values or KEquityPriceUpdateDlgPrivate::addPricePair() should assert() the pair.first and pair.second are non-empty
2) There shouldn't be any attempt made to add crippled PRICEPAIRs saved at all. 
3) The app shouldn't crash when closing the dialog
Comment 1 Dawid Wróbel 2020-06-07 02:20:56 UTC
I attempted to add !pair.second.isEmpty() check to the expression mentioned in 2) and 3), and while that stopped the crashing immediately, it... reversed the PRICEPAIR in the .kmy file: 

 <PRICEPAIR to="" from="PLN">
   <PRICE source="Transaction" date="2013-11-12" price="1/1"/>
   <PRICE source="Transaction" date="2014-02-03" price="1/1"/>
   <PRICE source="Transaction" date="2019-06-28" price="1/1"/>
   <PRICE source="Transaction" date="2019-10-04" price="1/1"/>
   <PRICE source="KMyMoney Currency" date="2020-03-20" price="1/1"/>
  </PRICEPAIR>
Comment 2 Dawid Wróbel 2021-05-30 00:06:07 UTC
I have noticed today that I also have a similar section with USD:

<PRICEPAIR to="" from="USD">
   <PRICE date="2015-10-30" source="Transaction" price="1/1"/>
(...)
   <PRICE date="2021-04-01" source="Transaction" price="1/1"/>
   <PRICE date="2021-05-05" source="Transaction" price="1/1"/>
</PRICEPAIR>

Worth noticing is that it keeps getting updated somehow.
Comment 3 Bug Janitor Service 2021-05-30 15:09:03 UTC
A possibly relevant merge request was started @ https://invent.kde.org/office/kmymoney/-/merge_requests/88
Comment 4 Dawid Wróbel 2021-06-02 16:13:05 UTC
Git commit 4e1d62d3903e4eca70e06c9dc4cc4d239be36cfa by Dawid Wróbel.
Committed on 02/06/2021 at 15:42.
Pushed by wrobelda into branch '5.1'.

Make sure prices are actual pairs when updating

When passing a pair to addPricePair() in Update Stock and Currency
Prices dialog, actually make sure that both elements are non-empty in
order to avoid an undefined behavior and SEGFAULTS.
FIXED-IN: 5.1.2

M  +8    -0    kmymoney/dialogs/kequitypriceupdatedlg.cpp

https://invent.kde.org/office/kmymoney/commit/4e1d62d3903e4eca70e06c9dc4cc4d239be36cfa
Comment 5 Dawid Wróbel 2021-06-02 16:25:39 UTC
Git commit 3b1356da845f2da7f6d66b9dea3cbca837f3e2c9 by Dawid Wrobel, on behalf of Dawid Wróbel.
Committed on 02/06/2021 at 16:25.
Pushed by wrobelda into branch 'master'.

Make sure prices are actual pairs when updating

When passing a pair to addPricePair() in Update Stock and Currency
Prices dialog, actually make sure that both elements are non-empty in
order to avoid an undefined behavior and SEGFAULTS.
FIXED-IN: 5.1.2


(cherry picked from commit 4e1d62d3903e4eca70e06c9dc4cc4d239be36cfa)

M  +8    -0    kmymoney/dialogs/kequitypriceupdatedlg.cpp

https://invent.kde.org/office/kmymoney/commit/3b1356da845f2da7f6d66b9dea3cbca837f3e2c9