Skip to content

Commit f05d349

Browse files
laanwjlaanwj
committed
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.
1 parent 16fff80 commit f05d349

File tree

3 files changed

+61
-53
lines changed

3 files changed

+61
-53
lines changed

‎src/qt/optionsdialog.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ QValidator::State ProxyAddressValidator::validate(QString &input, int &pos) cons
338338
{
339339
Q_UNUSED(pos);
340340
// Validate the proxy
341-
CService serv(LookupNumeric(input.toStdString().c_str(), 9050));
341+
CService serv(LookupNumeric(input.toStdString().c_str(), DEFAULT_GUI_PROXY_PORT));
342342
proxyType addrProxy = proxyType(serv, true);
343343
if (addrProxy.IsValid())
344344
return QValidator::Acceptable;

‎src/qt/optionsmodel.cpp

Lines changed: 57 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
#include <QSettings>
2929
#include <QStringList>
3030

31+
const char *DEFAULT_GUI_PROXY_HOST = "127.0.0.1";
32+
3133
OptionsModel::OptionsModel(QObject *parent, bool resetSettings) :
3234
QAbstractListModel(parent)
3335
{
@@ -124,8 +126,8 @@ void OptionsModel::Init(bool resetSettings)
124126

125127
if (!settings.contains("fUseProxy"))
126128
settings.setValue("fUseProxy", false);
127-
if (!settings.contains("addrProxy") || !settings.value("addrProxy").toString().contains(':'))
128-
settings.setValue("addrProxy", "127.0.0.1:9050");
129+
if (!settings.contains("addrProxy"))
130+
settings.setValue("addrProxy", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));
129131
// Only try to set -proxy, if user has enabled fUseProxy
130132
if (settings.value("fUseProxy").toBool() && !gArgs.SoftSetArg("-proxy", settings.value("addrProxy").toString().toStdString()))
131133
addOverriddenOption("-proxy");
@@ -134,8 +136,8 @@ void OptionsModel::Init(bool resetSettings)
134136

135137
if (!settings.contains("fUseSeparateProxyTor"))
136138
settings.setValue("fUseSeparateProxyTor", false);
137-
if (!settings.contains("addrSeparateProxyTor") || !settings.value("addrSeparateProxyTor").toString().contains(':'))
138-
settings.setValue("addrSeparateProxyTor", "127.0.0.1:9050");
139+
if (!settings.contains("addrSeparateProxyTor"))
140+
settings.setValue("addrSeparateProxyTor", QString("%1:%2").arg(DEFAULT_GUI_PROXY_HOST, DEFAULT_GUI_PROXY_PORT));
139141
// Only try to set -onion, if user has enabled fUseSeparateProxyTor
140142
if (settings.value("fUseSeparateProxyTor").toBool() && !gArgs.SoftSetArg("-onion", settings.value("addrSeparateProxyTor").toString().toStdString()))
141143
addOverriddenOption("-onion");
@@ -200,6 +202,33 @@ int OptionsModel::rowCount(const QModelIndex & parent) const
200202
return OptionIDRowCount;
201203
}
202204

205+
struct ProxySetting {
206+
bool is_set;
207+
QString ip;
208+
QString port;
209+
};
210+
211+
static ProxySetting GetProxySetting(QSettings &settings, const QString &name)
212+
{
213+
static const ProxySetting default_val = {false, DEFAULT_GUI_PROXY_HOST, QString("%1").arg(DEFAULT_GUI_PROXY_PORT)};
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);
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;
224+
}
225+
}
226+
227+
static void SetProxySetting(QSettings &settings, const QString &name, const ProxySetting &ip_port)
228+
{
229+
settings.setValue(name, ip_port.ip + ":" + ip_port.port);
230+
}
231+
203232
// read QSettings values and return them
204233
QVariant OptionsModel::data(const QModelIndex & index, int role) const
205234
{
@@ -226,30 +255,18 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
226255
// default proxy
227256
case ProxyUse:
228257
return settings.value("fUseProxy", false);
229-
case ProxyIP: {
230-
// contains IP at index 0 and port at index 1
231-
QStringList strlIpPort = settings.value("addrProxy").toString().split(":", QString::SkipEmptyParts);
232-
return strlIpPort.at(0);
233-
}
234-
case ProxyPort: {
235-
// contains IP at index 0 and port at index 1
236-
QStringList strlIpPort = settings.value("addrProxy").toString().split(":", QString::SkipEmptyParts);
237-
return strlIpPort.at(1);
238-
}
258+
case ProxyIP:
259+
return GetProxySetting(settings, "addrProxy").ip;
260+
case ProxyPort:
261+
return GetProxySetting(settings, "addrProxy").port;
239262

240263
// separate Tor proxy
241264
case ProxyUseTor:
242265
return settings.value("fUseSeparateProxyTor", false);
243-
case ProxyIPTor: {
244-
// contains IP at index 0 and port at index 1
245-
QStringList strlIpPort = settings.value("addrSeparateProxyTor").toString().split(":", QString::SkipEmptyParts);
246-
return strlIpPort.at(0);
247-
}
248-
case ProxyPortTor: {
249-
// contains IP at index 0 and port at index 1
250-
QStringList strlIpPort = settings.value("addrSeparateProxyTor").toString().split(":", QString::SkipEmptyParts);
251-
return strlIpPort.at(1);
252-
}
266+
case ProxyIPTor:
267+
return GetProxySetting(settings, "addrSeparateProxyTor").ip;
268+
case ProxyPortTor:
269+
return GetProxySetting(settings, "addrSeparateProxyTor").port;
253270

254271
#ifdef ENABLE_WALLET
255272
case SpendZeroConfChange:
@@ -314,25 +331,19 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
314331
}
315332
break;
316333
case ProxyIP: {
317-
// contains current IP at index 0 and current port at index 1
318-
QStringList strlIpPort = settings.value("addrProxy").toString().split(":", QString::SkipEmptyParts);
319-
// if that key doesn't exist or has a changed IP
320-
if (!settings.contains("addrProxy") || strlIpPort.at(0) != value.toString()) {
321-
// construct new value from new IP and current port
322-
QString strNewValue = value.toString() + ":" + strlIpPort.at(1);
323-
settings.setValue("addrProxy", strNewValue);
334+
auto ip_port = GetProxySetting(settings, "addrProxy");
335+
if (!ip_port.is_set || ip_port.ip != value.toString()) {
336+
ip_port.ip = value.toString();
337+
SetProxySetting(settings, "addrProxy", ip_port);
324338
setRestartRequired(true);
325339
}
326340
}
327341
break;
328342
case ProxyPort: {
329-
// contains current IP at index 0 and current port at index 1
330-
QStringList strlIpPort = settings.value("addrProxy").toString().split(":", QString::SkipEmptyParts);
331-
// if that key doesn't exist or has a changed port
332-
if (!settings.contains("addrProxy") || strlIpPort.at(1) != value.toString()) {
333-
// construct new value from current IP and new port
334-
QString strNewValue = strlIpPort.at(0) + ":" + value.toString();
335-
settings.setValue("addrProxy", strNewValue);
343+
auto ip_port = GetProxySetting(settings, "addrProxy");
344+
if (!ip_port.is_set || ip_port.port != value.toString()) {
345+
ip_port.port = value.toString();
346+
SetProxySetting(settings, "addrProxy", ip_port);
336347
setRestartRequired(true);
337348
}
338349
}
@@ -346,25 +357,19 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
346357
}
347358
break;
348359
case ProxyIPTor: {
349-
// contains current IP at index 0 and current port at index 1
350-
QStringList strlIpPort = settings.value("addrSeparateProxyTor").toString().split(":", QString::SkipEmptyParts);
351-
// if that key doesn't exist or has a changed IP
352-
if (!settings.contains("addrSeparateProxyTor") || strlIpPort.at(0) != value.toString()) {
353-
// construct new value from new IP and current port
354-
QString strNewValue = value.toString() + ":" + strlIpPort.at(1);
355-
settings.setValue("addrSeparateProxyTor", strNewValue);
360+
auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
361+
if (!ip_port.is_set || ip_port.ip != value.toString()) {
362+
ip_port.ip = value.toString();
363+
SetProxySetting(settings, "addrSeparateProxyTor", ip_port);
356364
setRestartRequired(true);
357365
}
358366
}
359367
break;
360368
case ProxyPortTor: {
361-
// contains current IP at index 0 and current port at index 1
362-
QStringList strlIpPort = settings.value("addrSeparateProxyTor").toString().split(":", QString::SkipEmptyParts);
363-
// if that key doesn't exist or has a changed port
364-
if (!settings.contains("addrSeparateProxyTor") || strlIpPort.at(1) != value.toString()) {
365-
// construct new value from current IP and new port
366-
QString strNewValue = strlIpPort.at(0) + ":" + value.toString();
367-
settings.setValue("addrSeparateProxyTor", strNewValue);
369+
auto ip_port = GetProxySetting(settings, "addrSeparateProxyTor");
370+
if (!ip_port.is_set || ip_port.port != value.toString()) {
371+
ip_port.port = value.toString();
372+
SetProxySetting(settings, "addrSeparateProxyTor", ip_port);
368373
setRestartRequired(true);
369374
}
370375
}

‎src/qt/optionsmodel.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ QT_BEGIN_NAMESPACE
1313
class QNetworkProxy;
1414
QT_END_NAMESPACE
1515

16+
extern const char *DEFAULT_GUI_PROXY_HOST;
17+
static constexpr unsigned short DEFAULT_GUI_PROXY_PORT = 9050;
18+
1619
/** Interface from Qt to configuration data structure for Bitcoin client.
1720
To Qt, the options are presented as a list with the different options
1821
laid out vertically.

0 commit comments

Comments
 (0)