/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 15, 2017
+1 −1

Conversation

Reviewers

@laanwj laanwj

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
8 participants
@jonasschnelli @laanwj @gmaxwell @luke-jr @MeshCollider @achow101 @MarcoFalke @TheBlueMatt
@jonasschnelli
Member

jonasschnelli commented Sep 14, 2017 •
Edited 1 time

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 jonasschnelli added the GUI label Sep 14, 2017

@jonasschnelli jonasschnelli referenced this pull request Sep 14, 2017

Closed

bitcoin-qt segmentation fault #11262

src/qt/sendcoinsdialog.cpp
- ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
+ QAbstractButton *groupCustomFee = ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())));
+ if (groupCustomFee) {
+ groupCustomFee->setChecked(true);
@laanwj

laanwj Sep 14, 2017

Owner
  • 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
Owner

laanwj commented Sep 14, 2017

Commit message needs to credit @achow101 for finding this :)
src/qt/sendcoinsdialog.cpp
@@ -128,7 +128,10 @@ SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *p
ui->groupFee->setId(ui->radioCustomFee, 1);
ui->groupFee->button((int)std::max(0, std::min(1, settings.value("nFeeRadio").toInt())))->setChecked(true);
ui->groupCustomFee->setId(ui->radioCustomPerKilobyte, 0);
- ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())))->setChecked(true);
+ QAbstractButton *groupCustomFee = ui->groupCustomFee->button((int)std::max(0, std::min(1, settings.value("nCustomFeeRadio").toInt())));
@laanwj

laanwj Sep 14, 2017

Owner

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

@jonasschnelli
Member

jonasschnelli commented Sep 14, 2017

Updated with the simpler solution by @TheBlueMatt (from 6ba2214).

Credited @achow101.
@jonasschnelli
Member

jonasschnelli commented Sep 14, 2017

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 jonasschnelli added the Needs backport label Sep 14, 2017

@TheBlueMatt @jonasschnelli
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
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
cdaf3a1
@gmaxwell
Member

gmaxwell commented Sep 15, 2017

utACK
@luke-jr
Member

luke-jr commented Sep 15, 2017

utACK
@MeshCollider
Member

MeshCollider commented Sep 15, 2017

utACK
@achow101
Member

achow101 commented Sep 15, 2017

utACK

@achow101 achow101 referenced this pull request Sep 15, 2017

Merged

qt: Remove custom fee radio group and remove nCustomFeeRadio setting #11334

@MarcoFalke
Member

MarcoFalke commented Sep 15, 2017

utACK cdaf3a1

@MarcoFalke MarcoFalke changed the title from Fix possible crash with invalid nCustomFeeRadio in QSettings to Fix possible crash with invalid nCustomFeeRadio in QSettings (achow101, TheBlueMatt) Sep 15, 2017

@laanwj laanwj merged commit cdaf3a1 into bitcoin:master Sep 15, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 15, 2017

@laanwj
Merge #11332: Fix possible crash with invalid nCustomFeeRadio in QSet… 
…tings (achow101, TheBlueMatt)


cdaf3a1 Fix Qt 0.14.2->0.15.0 segfault if "total at least" is selected (Matt Corallo)

Pull request description:

  `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.

Tree-SHA512: a1b5d6636382a4e20c4e66ef82de19e6daa8b1b5f21b0f2bc5f51cfb6b37797045c7e29ebead8088ee2b990ed12c549c217cae6aad7566319599d086d526f6dc
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
09627b1

laanwj added a commit that referenced this pull request Sep 15, 2017

@TheBlueMatt @laanwj
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

Github-Pull: #11332
Rebased-From: cdaf3a1
Tree-SHA512: 0a49bf4e9ab08e5869170c8a212da60c9a6b90c36427d788de384aa4be6d87bb5e00a21edf78eed34f81bbc554b6f15565bb9b493dafcbfe9d6f4664d7424d9d
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
46c8d23

laanwj added a commit that referenced this pull request Sep 20, 2017

@laanwj
Merge #11334: qt: Remove custom fee radio group and remove nCustomFee… 
…Radio setting


e53fa4a Remove custom fee radio group (Andrew Chow)

Pull request description:

  Removes the extraneous custom fee radio group and its single radio button. The radio button is replaced with a label that has the radio button's text.

  Continuation of #11332

Tree-SHA512: b47b675f900ee4e2f4823203a42bb697f707ba67a8504d730c53d4dae511d0ed03226af34efd7ea45570c6111f8b3b6c39ac28f1b5c090de225903442ad4159a
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
44313d8

@MarcoFalke MarcoFalke removed the Needs backport label Oct 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment