gui: Fix proxy setting options dialog crash#11809
Conversation
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
utACK 76755055e038d06ce1f3c69a6e9a2bab4625507b
src/qt/optionsmodel.cpp
Outdated
There was a problem hiding this comment.
Why not put this in the same place as DEFAULT_GUI_PROXY_PORT?
There was a problem hiding this comment.
It is there, just not the value.
Strings are pointers which point into a .rodata segment, so need to be present in a compilation unit.
Defining string constants in header files is a bad idea because it causes duplication in every place it's used, or even can lead to linker conflicts.
src/qt/optionsmodel.cpp
Outdated
There was a problem hiding this comment.
Remove SkipEmptyParts? Should foobar::1234 be valid?
There was a problem hiding this comment.
Also validate port is numeric?
There was a problem hiding this comment.
I intentionally (see the OP) do not add any extra validation in this pull. All that this does is prevent the crash.
A non-numeric port will already cause an init error with a message that the proxy is invalid.
src/qt/optionsmodel.cpp
Outdated
There was a problem hiding this comment.
Setting is invalid, maybe log/warn?
There was a problem hiding this comment.
Same as above. Invalid proxy settings already cause an error at a different level (e.g. -proxy=). I don't want to duplicate validation and parsing logic.
(also it's out of scope of this pull, which is just a bugfix/cleanup)
|
utACK 7675505. |
This fixes a crash bug when opening the options dialog. - Check the return value of split() to avoid segmentation faults due to out of bounds when the user manages to enter invalid proxy settings. This is reported resonably often. - Move the default proxy/port to a constant instead of hardcoding magic values. - Factor out some common code. - Revert bitcoin#11448 because this proves a more robust replacement, it is no longer necessary and didn't generally solve the issue. No attempt is made to do full sanity checking on the proxy, so it can still be rejected by the core with an InitError message.
|
squashed efcd4cd 7f434d0 415cd45 7675505 to f05d349 |
laanwj
force-pushed
the
2017_12_gui_proxy_robustness
branch
from
7675505 to
f05d349
Compare
f05d349 gui: Fix proxy setting options dialog crash (Wladimir J. van der Laan) Pull request description: This fixes a crash bug when opening the options dialog. - Check the return value of split() to avoid segmentation faults due to out of bounds when the user manages to enter invalid proxy settings. This is reported resonably often. - Move the default proxy/port to a constant instead of hardcoding magic values. - Factor out some common code. - Revert #11448 because this proves a more robust replacement, it is no longer necessary and didn't generally solve the issue. No attempt is made to do full sanity checking on the proxy, so it can still be rejected by the core with an InitError message. Tree-SHA512: 72b700b7d6c4d3e3410f0c60e9e4facf93d7c6c1a1b6b23957c48b074a045970f518166952859d1ebca8620062cb70d222670a7310bbd6fe50550ec6d04417b5
|
👍 |
|
In v0.16.0rc4 on Windows-64 executable I might suggest, if it is empty or not-a-valid-number, then just set the port to Also, the warning to restart client for changes to be effective does not seem to be displayed consistently, if the client needed to be restarted for any change. |
|
Tagged for backport to 15.2 if we want that, but please also backport #12650, which fixes another bug in this code. |
f05d349 gui: Fix proxy setting options dialog crash (Wladimir J. van der Laan) Pull request description: This fixes a crash bug when opening the options dialog. - Check the return value of split() to avoid segmentation faults due to out of bounds when the user manages to enter invalid proxy settings. This is reported resonably often. - Move the default proxy/port to a constant instead of hardcoding magic values. - Factor out some common code. - Revert bitcoin#11448 because this proves a more robust replacement, it is no longer necessary and didn't generally solve the issue. No attempt is made to do full sanity checking on the proxy, so it can still be rejected by the core with an InitError message. Tree-SHA512: 72b700b7d6c4d3e3410f0c60e9e4facf93d7c6c1a1b6b23957c48b074a045970f518166952859d1ebca8620062cb70d222670a7310bbd6fe50550ec6d04417b5
6 participants
This fixes a crash bug when opening the options dialog.
Check the return value of split() to avoid segmentation faults due to out of bounds when the user manages to enter invalid proxy settings. This is reported resonably often.
Move the default proxy/port to a constant instead of hardcoding magic values.
Factor out some common code.
Revert [gui] reset addrProxy/addrSeparateProxyTor if colon char missing #11448 because this proves a more robust replacement, it is no longer necessary and didn't generally solve the issue.
No attempt is made to do full sanity checking on the proxy, so it can still be rejected by the core with an InitError message.