| Summary: | Crash when importin investments CSV | ||
|---|---|---|---|
| Product: | [Applications] kmymoney | Reporter: | gzatko |
| Component: | general | Assignee: | KMyMoney Devel Mailing List <kmymoney-devel> |
| Status: | RESOLVED FIXED | ||
| Severity: | crash | Keywords: | drkonqi |
| Priority: | NOR | ||
| Version First Reported In: | 5.1.3 | ||
| Target Milestone: | --- | ||
| Platform: | openSUSE | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/office/kmymoney/commit/a8a631dc3f0a85a5594674c05a461a534a6a900a | Version Fixed In: | 5.2 |
| Sentry Crash Report: | |||
| Attachments: |
csvimporterrc
CSV to import |
||
|
Description
gzatko
2023-01-26 20:26:01 UTC
Could it be, that the selection for the symbol column is out of range? Can you attach the file $HOME/.config/kmymoney/csvimporterrc (the most personal info in there are your profile names) and a sample CSV file that shows the crash here (does not need to be real data - it should only reproduce the crash)? Created attachment 155756 [details]
csvimporterrc
Created attachment 155757 [details]
CSV to import
I have reproduced the crash with a new user using the csvimporterrc from comment2. Running in a debugger, in CSVImporterCore::sortSecurities, the row number starts out negative, even when I set the start and end rows to import, so I suspect something is getting invalid data, and the crash seems to be in the next call into some qt internal routine (which I don't currently have symbols for.) Can you try finding a csv file which imports with the same profile successfully, and then see if this csv file still causes the crash? I did two things to get a successful import. First, I edited the csvimporterrc and changed StartLine to 1 and TrailerLines to 0, and I created a new col identical to the first col so I could specify both a symbol and name col, as I don't have any of these securities in the test data file. I haven't fully followed the code, but I wonder if m_profile->m_startLine is somehow getting mis-set to a negative number. Looking at the following two parameters in the rc file StartLine=2 TrailerLines=12 and the code in CSVFile::getStartEndRow(CSVProfile *profile) explains how m_startLine can become negative if the number of rows is less than 12 (-7 using the files attached). Note: this is what I found out by code reading not executing. I wonder whether it is StartLine (or m_profile->m_startline) being negative that leads to the crash. The importer has plenty of loops with "for (int row = m_profile->m_startLine; row <= m_profile->m_endLine; ++row)" which use row to find a value (such as symbol and/or name in CSVImporterCore::sortSecurities) essentially leading to an out-of-bounds issue. Might it be necessary to confirm that m_startLine and m_endLine are reasonable before any of those loops? I would not want to check that before every loop but in a central spot. If start is -7 and end is -1 then the for loop still gets executed because -7 is less than -1. The crash happens on this line
symbol = m_file->m_model->item(row, symbolCol)->text().trimmed();
and with row being negative, m_file->m_model->item() will definitely return a null pointer. OTOH, it could be that the item does not even exist. In this case, item() also returns a null pointer according to the Qt docs.
I agree a central check is good - perhaps whenever those values get changed. I don't think this crash is simply because item() returns a null pointer, but because something crashes in the depths of Qt during the call to item(). I'm recompiling Qt with debug symbols, although even if we see what actually triggers the crash, I don't think it will change the conclusion. I can't find any documentation that says whether QStandardItemModel allows negative row numbers. Even though the csv import wizard clearly shows Start Line as 1 and End Line as 6, within sortLines m_startLine is -2 (from csvimporterrc) and m_endLine is 5 (which I assume is correct after changing 1 based counting for humans to 0 based counting internally.) I'm still trying to figure out why Qt crashes instead of just returning a nullpointer for the invalid row, and why the row seems to be wrong in the first place. From my reading of Qt code, QStandardItemModel should not allow negative rows (or columns) but this is apparently only checked on creation, not retrieval. However, I'm still unable find why it crashes instead of just returning a nullpointer. I'll try to get an answer on a qt list or forum, but will now try to create an MR, following from Comment #6. QStandardItem has no problem. Looking at the codeline in question
symbol = m_file->m_model->item(row, symbolCol)->text().trimmed();
the first part "m_file->m_model->item(row, symbolCol)" returns a nullpointer which is then used to call the "->text()" member with (this=0x0). That is clearly identified in the stacktrace as
#4 QStandardItem::text() const (this=0x0) at /usr/include/qt5/QtGui/qstandarditemmodel.h:75
#5 0x00007f130fe90429 in CSVImporterCore::sortSecurities(QSet<QString>&, QSet<QString>&, QMap<QString, QString>&) (this=0x7f12ec005b40, onlySymbols=..., onlyNames=..., mapSymbolName=...) at /usr/src/debug/kmymoney-5.1.3/kmymoney/plugins/csv/import/core/csvimportercore.cpp:1306
Calling text() via an invalid (null-) pointer is the problem and that is in our code. No need to get answers from other places when we already have them.
OK, my c++ reading ability is far worse than I thought. Even debugging through the Qt code, I somehow missed the switch from the call to item() to the call to text(). Adding a check before calling text() would certainly work - but I'll have to look at how many of them there are. If more than just a few, might it still be better to assure the starting row is not negative? Git commit a8a631dc3f0a85a5594674c05a461a534a6a900a by Jack Ostroff. Committed on 21/02/2023 at 22:17. Pushed by ostroffjh into branch '5.1'. Prevent crash in csv importer FIXED-IN: 5.1.4 M +18 -8 kmymoney/plugins/csv/import/core/csvimportercore.cpp https://invent.kde.org/office/kmymoney/commit/a8a631dc3f0a85a5594674c05a461a534a6a900a |