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

pull mess110 wants to merge 1 commits into bitcoin:master from mess110:ensure_colon_in_proxies_qsettings_storage changing 1 files +2 −2

mess110 commented at 9:57 PM on October 3, 2017: contributor

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 added the label GUI on Oct 3, 2017
fanquake added the label P2P on Oct 3, 2017

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

ce2418fa4c

in src/qt/optionsmodel.cpp:129 in 71ce95dad6 outdated

 125 | @@ -126,6 +126,8 @@ void OptionsModel::Init(bool resetSettings)
 126 |          settings.setValue("fUseProxy", false);
 127 |      if (!settings.contains("addrProxy"))
 128 |          settings.setValue("addrProxy", "127.0.0.1:9050");
 129 | +    if (!settings.value("addrProxy").toString().contains(':'))

achow101 commented at 10:06 PM on October 3, 2017:

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

mess110 commented at 10:09 PM on October 3, 2017:

Done

mess110 force-pushed on Oct 3, 2017
meshcollider commented at 1:33 AM on October 4, 2017: contributor

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 commented at 6:04 AM on October 4, 2017: contributor

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 commented at 6:04 AM on October 4, 2017: contributor

utACK ce2418fa4cef5514305cca82e74891b9d643c4c7
mess110 commented at 7:51 AM on October 4, 2017: contributor

@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 commented at 7:59 AM on October 4, 2017: contributor

Fair enough I guess. utACK
laanwj commented at 12:28 PM on October 4, 2017: member

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 commented at 2:00 PM on October 4, 2017: member

utACK ce2418fa4cef5514305cca82e74891b9d643c4c7
promag commented at 10:38 PM on October 6, 2017: member

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 commented at 2:07 PM on October 9, 2017: member

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 merged this on Oct 9, 2017
laanwj closed this on Oct 9, 2017
laanwj referenced this in commit d473e6ddc5 on Oct 9, 2017
mess110 deleted the branch on Oct 9, 2017
laanwj referenced this in commit e8f10248cf on Dec 1, 2017
laanwj referenced this in commit efcd4cde7d on Dec 1, 2017
laanwj referenced this in commit f05d349887 on Dec 7, 2017
laanwj referenced this in commit 80f9dad0b7 on Dec 7, 2017
HashUnlimited referenced this in commit 86dec39120 on Mar 15, 2018
PastaPastaPasta referenced this in commit 88197c7414 on Jan 26, 2020
PastaPastaPasta referenced this in commit abb27edc72 on Feb 13, 2020
PastaPastaPasta referenced this in commit ea94b5dd35 on Feb 27, 2020
PastaPastaPasta referenced this in commit 315504c8c3 on Feb 27, 2020
ckti referenced this in commit 3216ba5b54 on Mar 28, 2021
ckti referenced this in commit 0cc2f4216b on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11209 [GUI] Proxies in QSettings storage without a colon cause crashes when options dialog is opened #11809 gui: Fix proxy setting options dialog crash