gui: Fix proxy setting options dialog crash

laanwj commented at 11:54 am on December 1, 2017: member

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.

laanwj added the label GUI on Dec 1, 2017

gmaxwell approved

gmaxwell commented at 8:41 pm on December 1, 2017: contributor

utACK

in src/qt/optionsmodel.cpp:31 in 76755055e0 outdated

27@@ -28,6 +28,8 @@
28 #include <QSettings>
29 #include <QStringList>
30 
31+const char *DEFAULT_GUI_PROXY_HOST = "127.0.0.1";

achow101 commented at 10:49 pm on December 1, 2017:

Why not put this in the same place as DEFAULT_GUI_PROXY_PORT?

laanwj commented at 7:56 am on December 2, 2017:

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.

achow101 approved

achow101 commented at 10:52 pm on December 1, 2017: member

utACK 76755055e038d06ce1f3c69a6e9a2bab4625507b

in src/qt/optionsmodel.cpp:219 in 76755055e0 outdated

214+    // Handle the case that the setting is not set at all
215+    if (!settings.contains(name)) {
216+        return default_val;
217+    }
218+    // contains IP at index 0 and port at index 1
219+    QStringList ip_port = settings.value(name).toString().split(":", QString::SkipEmptyParts);

promag commented at 2:56 pm on December 4, 2017:

Remove SkipEmptyParts? Should foobar::1234 be valid?

promag commented at 3:05 pm on December 4, 2017:

Also validate port is numeric?

laanwj commented at 4:44 pm on December 4, 2017:

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.

in src/qt/optionsmodel.cpp:223 in 76755055e0 outdated

218+    // contains IP at index 0 and port at index 1
219+    QStringList ip_port = settings.value(name).toString().split(":", QString::SkipEmptyParts);
220+    if (ip_port.size() == 2) {
221+        return {true, ip_port.at(0), ip_port.at(1)};
222+    } else { // Invalid: return default
223+        return default_val;

promag commented at 3:05 pm on December 4, 2017:

Setting is invalid, maybe log/warn?

laanwj commented at 4:45 pm on December 4, 2017:

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)

promag commented at 5:28 pm on December 4, 2017: member

utACK 7675505.

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

f05d349887

laanwj commented at 4:34 pm on December 7, 2017: member

squashed efcd4cd 7f434d0 415cd45 7675505 to f05d34988719b22ef5c64888f90b4c8f3a2c7931

laanwj force-pushed on Dec 7, 2017

laanwj merged this on Dec 7, 2017

laanwj closed this on Dec 7, 2017

laanwj referenced this in commit 80f9dad0b7 on Dec 7, 2017

promag commented at 4:50 pm on December 7, 2017: member

:+1:

Willtech commented at 10:24 am on February 27, 2018: contributor

In v0.16.0rc4 on Windows-64 executable bitcoin-qt.exe, the default proxy port is %2 ?

I might suggest, if it is empty or not-a-valid-number, then just set the port to 9050.

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.

MarcoFalke added this to the milestone 0.15.2 on Mar 17, 2018

MarcoFalke added the label Needs backport on Mar 17, 2018

MarcoFalke commented at 3:22 pm on March 17, 2018: member

Tagged for backport to 15.2 if we want that, but please also backport #12650, which fixes another bug in this code.

laanwj removed the label Needs backport on May 24, 2018

laanwj removed this from the milestone 0.15.2 on May 24, 2018

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 0cc2f4216b on Mar 28, 2021

MarcoFalke locked this on Sep 8, 2021

gui: Fix proxy setting options dialog crash #11809