[qt] Avoid potential null pointer dereference in TransactionView::exportClicked() #10673

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:null-pointer-dereference changing 1 files +4 −0

practicalswift commented at 8:07 PM on June 26, 2017: contributor

Avoid potential null pointer dereference in TransactionView::exportClicked().

Issue introduced in commit 8969828d069e4e55108618a493749535edc12ec7.
fanquake added the label GUI on Jun 27, 2017
[qt] Avoid potential null pointer dereference in TransactionView::exportClicked() fd9599b135

in src/qt/transactionview.cpp:358 in 46756b16ef outdated

 354 | @@ -355,7 +355,11 @@ void TransactionView::exportClicked()
 355 |      writer.addColumn(tr("Type"), TransactionTableModel::Type, Qt::EditRole);
 356 |      writer.addColumn(tr("Label"), 0, TransactionTableModel::LabelRole);
 357 |      writer.addColumn(tr("Address"), 0, TransactionTableModel::AddressRole);
 358 | -    writer.addColumn(BitcoinUnits::getAmountColumnTitle(model->getOptionsModel()->getDisplayUnit()), 0, TransactionTableModel::FormattedAmountRole);
 359 | +    if (model && model->getOptionsModel()) {

laanwj commented at 12:21 PM on June 27, 2017:

In case model or model->getOptionsModel() is not set, there's no reason to continue here - there's no data to export. Might as well return from the function early if so.

practicalswift force-pushed on Jun 27, 2017
practicalswift commented at 1:07 PM on June 27, 2017: contributor

@laanwj Fixed! Looks good? Is an error message dialog necessary?
laanwj commented at 2:31 PM on June 27, 2017: member

Is an error message dialog necessary?

That's not necessary. It's fine for the export to do nothing in that case.
practicalswift commented at 11:05 PM on June 27, 2017: contributor
A bit of context might be needed for this change – I assumed model can be NULL when entering exportClicked() due to this check:
```
    if (model && model->haveWatchOnly())
        writer.addColumn(tr("Watch-only"), TransactionTableModel::Watchonly);
```
Let me know if that assumption is incorrect :-)
laanwj commented at 11:16 AM on June 28, 2017: member

In normal use that shouldn't happen. However the view class should be able to work (at least: not crash) with a NULL model, for UI-only testing.
practicalswift commented at 9:27 PM on June 28, 2017: contributor

@laanwj OK, thanks for the clarification!
jonasschnelli commented at 6:31 AM on June 29, 2017: contributor

utACK fd9599b1358a314b073a9ca0a68ca8037915d91d (runtime protection is always good).

Though I having a hard time finding a possibility how the user could press the "Export" button while not having a walletmodel attached to the TransactionView.
laanwj merged this on Jun 29, 2017
laanwj closed this on Jun 29, 2017
laanwj referenced this in commit 4c72cc33eb on Jun 29, 2017
PastaPastaPasta referenced this in commit 70c89c5150 on Jul 6, 2019
PastaPastaPasta referenced this in commit c222458559 on Jul 8, 2019
PastaPastaPasta referenced this in commit 8681acfac3 on Jul 9, 2019
PastaPastaPasta referenced this in commit 535d2c89e7 on Jul 11, 2019
PastaPastaPasta referenced this in commit 7b04780f3c on Jul 13, 2019
PastaPastaPasta referenced this in commit 184847c2ed on Jul 17, 2019
barrystyle referenced this in commit 314a7075e0 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels