Fix possible crash with invalid nCustomFeeRadio in QSettings (achow101, TheBlueMatt) #11332
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laanwj
reviewed
Sep 14, 2017
src/qt/sendcoinsdialog.cpp
Outdated
Member
There was a problem hiding this comment.
- 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?
Member
|
Commit message needs to credit @achow101 for finding this :) |
laanwj
reviewed
Sep 14, 2017
src/qt/sendcoinsdialog.cpp
Outdated
Member
There was a problem hiding this comment.
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
the
2017/09/qsettings_1
branch
2 times, most recently
from
a703bd4 to
e45f21c
Compare
Contributor
Author
|
Updated with the simpler solution by @TheBlueMatt (from 6ba2214). Credited @achow101. |
Contributor
Author
|
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) |
A button was removed, so now button(1) is nullptr
jonasschnelli
force-pushed
the
2017/09/qsettings_1
branch
from
e45f21c to
cdaf3a1
Compare
Contributor
|
utACK |
3 similar comments
Member
|
utACK |
Contributor
|
utACK |
Member
|
utACK |
Member
|
utACK cdaf3a1 |
maflcko
changed the title
laanwj
added a commit
that referenced
this pull request
Sep 15, 2017
…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
laanwj
added a commit
that referenced
this pull request
Sep 20, 2017
…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
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 16, 2019
Summary: introduced in D2322 backport Didn't affect most people, only those who had custom fee selected (which required using Coin Control) Workaround for affected users: change nCustomFeeRadio=1 to nCustomFeeRadio=0 in the ~/.config/BitcoinABC/<whatever>.conf file. same fix was also required in core: bitcoin/bitcoin#11332 Test Plan: it runs now! Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2559
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Sep 23, 2019
…stomFeeRadio 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 bitcoin#11332 Tree-SHA512: b47b675f900ee4e2f4823203a42bb697f707ba67a8504d730c53d4dae511d0ed03226af34efd7ea45570c6111f8b3b6c39ac28f1b5c090de225903442ad4159a
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
QButtonGroup->button()may return a nullptr.Accessing the object directly with
setCheckedseems 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
QSettingsare required.Found by @achow101.
Code by @TheBlueMatt.