Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gui] reset addrProxy/addrSeparateProxyTor if colon char missing #11448

Merged
merged 1 commit into from Oct 9, 2017

Conversation

Projects
None yet
7 participants
@mess110
Copy link
Contributor

mess110 commented Oct 3, 2017

If addrProxy or addrSeparateProxyTor do not have a colon in the string
somewhere in the QSettings storage, then attempting to open the options
dialog will cause the entire program to crash.

Fixes #11209

@fanquake fanquake added GUI P2P labels Oct 3, 2017

src/qt/optionsmodel.cpp Outdated
@@ -126,6 +126,8 @@ void OptionsModel::Init(bool resetSettings)
settings.setValue("fUseProxy", false);
if (!settings.contains("addrProxy"))
settings.setValue("addrProxy", "127.0.0.1:9050");
if (!settings.value("addrProxy").toString().contains(':'))

This comment has been minimized.

@achow101

achow101 Oct 3, 2017

Member

Can you just make this an OR with the if statement above?

This comment has been minimized.

@mess110

mess110 Oct 3, 2017

Author Contributor

Done

[gui] reset addrProxy/addrSeparateProxyTor if colon char missing
If addrProxy or addrSeparateProxyTor do not have a colon in the string
somewhere in the QSettings storage, then attempting to open the options
dialog will cause the entire program to crash.

@mess110 mess110 force-pushed the mess110:ensure_colon_in_proxies_qsettings_storage branch to ce2418f Oct 3, 2017

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 4, 2017

Please change PR description to say 'Fixes #11209' so that GitHub auto-closes the issue if this is merged.

If for some reason there was no colon, why doesn't the default port just get appended to the existing IP rather than defaulting too?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 4, 2017

Can't reproduce the #11209 crash on OSX.
Also, there are probably other situations where a corrupted or manually edited QSettings files may lead to a crash.

But this is better then noting.

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 4, 2017

utACK ce2418f

@mess110

This comment has been minimized.

Copy link
Contributor Author

mess110 commented Oct 4, 2017

@MeshCollider To check this I just deleted the : from addrProxy, so the value was 127.0.0.19050. My though process was: corrupt? ok, rewrite. My goal was to avoid the crash.

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 4, 2017

Fair enough I guess. utACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 4, 2017

utACK,

though I'm not sure I agree the original issue is really an issue - this can only be reached by manually tampering with the settings, which can mess up the client in a lot of ways.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Oct 4, 2017

utACK ce2418f

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 6, 2017

AFAIK it doesn't fix the issue when the setting is changed in runtime externally.

I think the settings value should be validated when used. In this case the .split(":")[1] wouldn't crash. This is also relevant when a setting can be upgraded/extended to a new format and/or have default values for the missing bits (port in this case).

In this particular case I would say the fix could be the introduction/usage of a decent address parser.

utACK ce2418f considering it's an remote use case.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 9, 2017

AFAIK it doesn't fix the issue when the setting is changed in runtime externally.

The setting is only ever loaded from the qsettings at startup. Changing it at runtime externally does nothing.

@laanwj laanwj merged commit ce2418f into bitcoin:master Oct 9, 2017

1 check passed

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

laanwj added a commit that referenced this pull request Oct 9, 2017

Merge #11448: [gui] reset addrProxy/addrSeparateProxyTor if colon cha…
…r missing

ce2418f [gui] reset addrProxy/addrSeparateProxyTor if colon char missing (Cristian Mircea Messel)

Pull request description:

  If addrProxy or addrSeparateProxyTor do not have a colon in the string
  somewhere in the QSettings storage, then attempting to open the options
  dialog will cause the entire program to crash.

  Fixes #11209

Tree-SHA512: 2d9e6987cf05af3f41033290b61d00920f7fe4a65bea7efd96ed417a8ca7866d248f091e09947cc8aad3a6a4aa8b7777211cfff7f379a62188be50df2c46d4b2

@mess110 mess110 deleted the mess110:ensure_colon_in_proxies_qsettings_storage branch Oct 9, 2017

laanwj added a commit to laanwj/bitcoin that referenced this pull request Dec 7, 2017

gui: Fix proxy setting options dialog crash
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.

laanwj added a commit that referenced this pull request Dec 7, 2017

Merge #11809: gui: Fix proxy setting options dialog crash
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

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 15, 2018

gui: Fix proxy setting options dialog crash
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.