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

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.

2. jonasschnelli added the label GUI on Sep 14, 2017
3. 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?
4. 
   laanwj commented at 8:13 pm on September 14, 2017: member

   Commit message needs to credit @achow101 for finding this :)
5. 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?)
6. jonasschnelli force-pushed on Sep 14, 2017
7. jonasschnelli force-pushed on Sep 14, 2017
8. 
   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.

9. 
   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)
10. jonasschnelli added the label Needs backport on Sep 14, 2017
11. 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
12. jonasschnelli force-pushed on Sep 14, 2017
13. 
    gmaxwell commented at 2:13 am on September 15, 2017: contributor

    utACK
14. 
    luke-jr commented at 2:54 am on September 15, 2017: member

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

    utACK
16. 
    achow101 commented at 3:38 am on September 15, 2017: member

    utACK
17. 
    MarcoFalke commented at 6:53 am on September 15, 2017: member

    utACK cdaf3a1f9e93be273ebf3e470dc709828c55476c
18. 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
19. laanwj merged this on Sep 15, 2017
20. laanwj closed this on Sep 15, 2017

    ---

21. laanwj referenced this in commit 09627b1dd4 on Sep 15, 2017
22. laanwj referenced this in commit 46c8d23dad on Sep 15, 2017
23. laanwj referenced this in commit 44313d8250 on Sep 20, 2017
24. MarcoFalke removed the label Needs backport on Oct 4, 2017
25. deadalnix referenced this in commit eafa2b46d8 on Feb 16, 2019
26. codablock referenced this in commit 4c81f66829 on Sep 20, 2019
27. codablock referenced this in commit 5103bacc89 on Sep 22, 2019
28. codablock referenced this in commit 499fcecaf4 on Sep 23, 2019
29. barrystyle referenced this in commit fc82b18e9d on Jan 22, 2020
30. DrahtBot locked this on Sep 8, 2021

Contributors
jonasschnelli laanwj gmaxwell luke-jr meshcollider achow101 MarcoFalke

Labels
GUI