Fix possible crash with invalid nCustomFeeRadio in QSettings (achow101, TheBlueMatt) #11332

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2017/09/qsettings_1 changing 1 files +1 −1

jonasschnelli commented at 8:09 PM on September 14, 2017: contributor

QButtonGroup->button() may return a nullptr. Accessing the object directly with setChecked seems fragile.

This is a simple fix to ensure to never call a button out of bounds (nullptr).

There are probably other places where a sanity check for QSettings are required.

Found by @achow101. Code by @TheBlueMatt.
jonasschnelli added the label GUI on Sep 14, 2017
jonasschnelli cross-referenced this on Sep 14, 2017 from issue bitcoin-qt segmentation fault by mputter

in src/qt/sendcoinsdialog.cpp:133 in 8052406a9c outdated

 127 | @@ -128,7 +128,10 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
 128 |      ui->groupFee->setId(ui->radioCustomFee, 1);
 129 |      ui->groupFee->button((int)std::max(0, std::min(1, settings.value("nFeeRadio").toInt())))->setChecked(true);
 130 |      ui->groupCustomFee->setId(ui->radioCustomPerKilobyte, 0);
 131 | -    ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
 132 | +    QAbstractButton *groupCustomFee = ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())));
 133 | +    if (groupCustomFee) {
 134 | +        groupCustomFee->setChecked(true);

laanwj commented at 8:12 PM on September 14, 2017:

If you add the null check, do you still need the awkward std::max(0, std::min(1, construction? after all, button is defined to return 0 if no such button exists http://doc.qt.io/qt-4.8/qbuttongroup.html#button
Wouldn't it be better to remove the group completely if there is only one choice?

laanwj commented at 8:13 PM on September 14, 2017: member

Commit message needs to credit @achow101 for finding this :)

in src/qt/sendcoinsdialog.cpp:131 in 8052406a9c outdated

 127 | @@ -128,7 +128,10 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
 128 |      ui->groupFee->setId(ui->radioCustomFee, 1);
 129 |      ui->groupFee->button((int)std::max(0, std::min(1, settings.value("nFeeRadio").toInt())))->setChecked(true);
 130 |      ui->groupCustomFee->setId(ui->radioCustomPerKilobyte, 0);
 131 | -    ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
 132 | +    QAbstractButton *groupCustomFee = ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())));

laanwj commented at 8:14 PM on September 14, 2017:

groupCustomFee might not be the best name for this variable, after all it's a button not a group (also snake_case?)

jonasschnelli force-pushed on Sep 14, 2017
jonasschnelli force-pushed on Sep 14, 2017
jonasschnelli commented at 8:30 PM on September 14, 2017: contributor

Updated with the simpler solution by @TheBlueMatt (from https://github.com/bitcoin/bitcoin/commit/6ba2214e0fe57ecb9271819d2422ec44a9020a8d).

Credited @achow101.
jonasschnelli commented at 8:31 PM on September 14, 2017: contributor

This is a back-portable short fix and for 0.16 we should consider removing the group entirely (over further overhaul the custom fee settings)
jonasschnelli added the label Needs backport on Sep 14, 2017
Fix Qt 0.14.2->0.15.0 segfault if "total at least" is selected
A button was removed, so now button(1) is nullptr
cdaf3a1f9e
jonasschnelli force-pushed on Sep 14, 2017
gmaxwell commented at 2:13 AM on September 15, 2017: contributor

utACK
luke-jr commented at 2:54 AM on September 15, 2017: member

utACK
meshcollider commented at 2:55 AM on September 15, 2017: contributor

utACK
achow101 commented at 3:38 AM on September 15, 2017: member

utACK
achow101 cross-referenced this on Sep 15, 2017 from issue qt: Remove custom fee radio group and remove nCustomFeeRadio setting by achow101
MarcoFalke commented at 6:53 AM on September 15, 2017: member

utACK cdaf3a1f9e93be273ebf3e470dc709828c55476c
MarcoFalke renamed this:
~~Fix possible crash with invalid nCustomFeeRadio in QSettings~~
Fix possible crash with invalid nCustomFeeRadio in QSettings (achow101, TheBlueMatt)
on Sep 15, 2017
laanwj merged this on Sep 15, 2017
laanwj closed this on Sep 15, 2017
laanwj referenced this in commit 09627b1dd4 on Sep 15, 2017
laanwj referenced this in commit 46c8d23dad on Sep 15, 2017
laanwj referenced this in commit 44313d8250 on Sep 20, 2017
MarcoFalke removed the label Needs backport on Oct 4, 2017
markblundeberg cross-referenced this on Feb 16, 2019 from issue [wallet] remove minimum total fee option by instagibbs
deadalnix referenced this in commit eafa2b46d8 on Feb 16, 2019
codablock referenced this in commit 4c81f66829 on Sep 20, 2019
codablock referenced this in commit 5103bacc89 on Sep 22, 2019
codablock referenced this in commit 499fcecaf4 on Sep 23, 2019
barrystyle referenced this in commit fc82b18e9d on Jan 22, 2020
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10390 [wallet] remove minimum total fee option #11262 bitcoin-qt segmentation fault #11334 qt: Remove custom fee radio group and remove nCustomFeeRadio setting