Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

add LOCK() for proxy related data-structures #1859

Merged
merged 1 commit into from

5 participants

P. Kaufmann BitcoinPullTester Wladimir J. van der Laan Pieter Wuille Gavin Andresen
P. Kaufmann
  • fix #1560 by properly locking proxy related data-structures
  • introduce GetProxyPair() and GetNameProxyPair() to be able to use a thread-safe local copy from proxyInfo and nameproxyInfo
  • rename GetNameProxy() into HaveNameProxy() to be more clear
src/netbase.cpp
((5 lines not shown))
static proxyType nameproxyInfo;
+static CCriticalSection cs_nameproxyInfo;
Wladimir J. van der Laan Owner
laanwj added a note

Using one "proxy info" lock would be enough, no need for a separate one for the nameproxy.

P. Kaufmann
Diapolo added a note

Indeed, fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bd5f5de6b8ab13ec34c96fa7376da329a07c1105 for binaries and test log.

Wladimir J. van der Laan
Owner

ACK

src/netbase.cpp
@@ -467,6 +474,7 @@ bool IsProxy(const CNetAddr &addr) {
bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
{
+ LOCK(cs_proxyInfos);
Gavin Andresen Owner

Holding the cs_proxyInfos lock all through ConnectSocket and it's subroutines is bad practice.

How about something like:
proxyType proxy;
{
LOCK(cs_proxyInfos);
proxy = proxyInfo[addrDest.GetNetwork()];
}

... proceed, with local copy of proxy...

Wladimir J. van der Laan Owner
laanwj added a note

Or... We already have a GetProxy function to encapsulate getting values from proxyInfo, why not use it here?

P. Kaufmann
Diapolo added a note

Just to understand, a lock is persistent in a function scope, right? So when I have a lock in a sub-function used in ConnectSocket it is released, when returning back to ConnectSocket.

Wladimir J. van der Laan Owner
laanwj added a note

A scoped lock is acquired where you define it with LOCK(), and it is released immediately at the end of the block (}) in which you define it. So, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/netbase.cpp
@@ -504,6 +512,7 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
SplitHostPort(string(pszDest), port, strDest);
SOCKET hSocket = INVALID_SOCKET;
+ LOCK(cs_proxyInfos);
Gavin Andresen Owner

Same comment here, maybe:
proxyType npInfo;
{
LOCK(); npInfo = nameproxyInfo;
}
... then proceed to use npInfo instead of the static nameproxyInfo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
P. Kaufmann

I'm going to update this until early next week :).

P. Kaufmann

Updated to include 2 new functions to use local copies of the proxyInfo and nameproxyInfo objects in the time-critical functions, which removes the bad practise of holding the lock all through ConnectSocket and it's subroutines, as Gavin suggested.

I chose to add these 2 function (currently not exposed to netbase.h), because I think I need those when updating the GUI with extended proxy settings.

src/netbase.h
@@ -139,7 +139,7 @@ class CService : public CNetAddr
bool GetProxy(enum Network net, CService &addrProxy);
bool IsProxy(const CNetAddr &addr);
bool SetNameProxy(CService addrProxy, int nSocksVersion = 5);
-bool GetNameProxy();
+bool IsNameProxy();
Pieter Wuille Owner
sipa added a note

What "is" a name proxy? I agree GetNameProxy() currently doesn't make sense either, but I'd go for HaveNameProxy().

P. Kaufmann
Diapolo added a note

Seems better suited, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Wladimir J. van der Laan laanwj commented on the diff
src/netbase.cpp
((33 lines not shown))
nameproxyInfo = std::make_pair(addrProxy, nSocksVersion);
return true;
}
-bool GetNameProxy() {
+bool GetNameProxyPair(proxyType &nameproxyInfoOut) {
+ LOCK(cs_proxyInfos);
+ if (!nameproxyInfo.second)
+ return false;
+ nameproxyInfoOut = nameproxyInfo;
+ return true;
+}
+
+bool HaveNameProxy() {
+ LOCK(cs_proxyInfos);
return nameproxyInfo.second != 0;
}
bool IsProxy(const CNetAddr &addr) {
Wladimir J. van der Laan Owner
laanwj added a note

Edit: never mind, it does exactly what it says, I misunderstood the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/netbase.cpp
((12 lines not shown))
if (!proxyInfo[net].second)
return false;
addrProxy = proxyInfo[net].first;
return true;
}
+bool GetProxyPair(enum Network net, proxyType &proxyInfoOut) {
Wladimir J. van der Laan Owner
laanwj added a note

GetProxy already returns the same information (addrProxy, used), although in a slightly different form. Either keep this function or GetProxy, but we don't need both.

P. Kaufmann
Diapolo added a note

GetProxy() returns a CService, but I need the CService AND SOCKS version for the requested net, see ConnectSocket().
Perhaps you need to explain a little more :).

Edit: ConnectSocket() uses both values of the pair .first and .second.

Wladimir J. van der Laan Owner
laanwj added a note

Yeah, so replace GetProxy to return a proxyType and use that. Returning all the information is better IMO. I just don't see the need for two functions, one which returns a subset of the information that the other does.

Pieter Wuille Owner
sipa added a note

Agree with @laanwj

P. Kaufmann
Diapolo added a note

I simply didn't understand what you wanted me to do ^^, now it's clear @laanwj.
I'll update tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
P. Kaufmann

@sipa I updated GetProxy() to fill a proxyType object and make that usable outsite of netbase.cpp (moved the typedef to netbase.h).
@laanwj Can you have a in depth look at the changes to optionsmodel.cpp, I included the use of GetProxy() for the SOCKS version, too, which was not in before (was read from the settings only).

P. Kaufmann Diapolo commented on the diff
src/qt/optionsmodel.cpp
@@ -145,21 +145,26 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
case ProxyUse:
return settings.value("fUseProxy", false);
P. Kaufmann
Diapolo added a note

@laanwj I was asking myself, if this should be changed into, as data() returns active state for IP, port and SOCKS version, too.

        case ProxyUse: {
            proxyType proxy;
            return QVariant(GetProxy(NET_IPV4, proxy));
        }
Wladimir J. van der Laan Owner
laanwj added a note

Could be better because that better represents the current configuration, but leave that for a later pull. Aim to keep functionality precisely the same in these commits.

P. Kaufmann
Diapolo added a note

Understood :), thanks for your valuable input master!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/netbase.cpp
@@ -467,7 +481,9 @@ bool IsProxy(const CNetAddr &addr) {
bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
{
- const proxyType &proxy = proxyInfo[addrDest.GetNetwork()];
+ proxyType proxy;
+ if (!GetProxy(addrDest.GetNetwork(), proxy))
+ return false;
Wladimir J. van der Laan Owner
laanwj added a note

This is new behavior -- I don't think you want to exit here with an error if there is no proxy

P. Kaufmann
Diapolo added a note

Indeed, that's a fault, same for the below occurance!
Should I memset the proxyType var to 0 or is this done by the compiler here?

Edit: Should be safe to leave it, because of default constructors used, right :)?

Wladimir J. van der Laan Owner
laanwj added a note

No need to zero it explicitly. It's an object with constructor (std::pair) and not a POD ("Plain Old Data") type. See http://stackoverflow.com/questions/9025792/does-the-default-constructor-of-stdpair-set-basic-types-int-etc-to-zero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/netbase.cpp
@@ -504,19 +520,23 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
SplitHostPort(string(pszDest), port, strDest);
SOCKET hSocket = INVALID_SOCKET;
- CService addrResolved(CNetAddr(strDest, fNameLookup && !nameproxyInfo.second), port);
+
+ proxyType nameproxy;
+ if (!GetNameProxy(nameproxy))
Wladimir J. van der Laan Owner
laanwj added a note

Same here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Pieter Wuille
Owner

ACK on changes to core.

BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/fd31870d2820d7e1d0f8f3253f081aa71c0b2dca for binaries and test log.

P. Kaufmann

@laanwj Anything left for the Qt changes? I trie to not include feature-changes and will do that after this got merged.

Wladimir J. van der Laan
Owner

ACK

P. Kaufmann

No code changes, only updated some code-layout in optionsmodel.cpp.

src/netbase.cpp
@@ -467,7 +481,8 @@ bool IsProxy(const CNetAddr &addr) {
bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
{
- const proxyType &proxy = proxyInfo[addrDest.GetNetwork()];
+ proxyType proxy;
+ GetProxy(addrDest.GetNetwork(), proxy);
Wladimir J. van der Laan Owner
laanwj added a note

I'd suggest using the return value of GetProxy here, instead of querying proxy.second later on. It's a little bit more readable.

P. Kaufmann
Diapolo added a note

Yes, good idea ... and updated :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Philip Kaufmann add LOCK() for proxy related data-structures
- fix #1560 by properly locking proxy related data-structures
- update GetProxy() and introduce GetNameProxy() to be able to use a
  thread-safe local copy from proxyInfo and nameproxyInfo
- update usage of GetProxy() all over the source to match the new
  behaviour, as it now fills a full proxyType object
- rename GetNameProxy() into HaveNameProxy() to be more clear
81bbef2
P. Kaufmann

Can we get this fix into 0.7.1?

Wladimir J. van der Laan
Owner
Pieter Wuille sipa merged commit 43de649 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 4, 2012
  1. add LOCK() for proxy related data-structures

    Philip Kaufmann authored
    - fix #1560 by properly locking proxy related data-structures
    - update GetProxy() and introduce GetNameProxy() to be able to use a
      thread-safe local copy from proxyInfo and nameproxyInfo
    - update usage of GetProxy() all over the source to match the new
      behaviour, as it now fills a full proxyType object
    - rename GetNameProxy() into HaveNameProxy() to be more clear
This page is out of date. Refresh to see the latest.
4 src/net.cpp
View
@@ -1185,7 +1185,7 @@ void ThreadDNSAddressSeed2(void* parg)
printf("Loading addresses from DNS seeds (could take a while)\n");
for (unsigned int seed_idx = 0; seed_idx < ARRAYLEN(strDNSSeed); seed_idx++) {
- if (GetNameProxy()) {
+ if (HaveNameProxy()) {
AddOneShot(strDNSSeed[seed_idx][1]);
} else {
vector<CNetAddr> vaddr;
@@ -1529,7 +1529,7 @@ void ThreadOpenAddedConnections2(void* parg)
if (mapArgs.count("-addnode") == 0)
return;
- if (GetNameProxy()) {
+ if (HaveNameProxy()) {
while(!fShutdown) {
BOOST_FOREACH(string& strAddNode, mapMultiArgs["-addnode"]) {
CAddress addr;
41 src/netbase.cpp
View
@@ -5,6 +5,7 @@
#include "netbase.h"
#include "util.h"
+#include "sync.h"
#ifndef WIN32
#include <sys/fcntl.h>
@@ -16,9 +17,9 @@
using namespace std;
// Settings
-typedef std::pair<CService, int> proxyType;
static proxyType proxyInfo[NET_MAX];
static proxyType nameproxyInfo;
+static CCriticalSection cs_proxyInfos;
int nConnectTimeout = 5000;
bool fNameLookup = false;
@@ -432,15 +433,17 @@ bool SetProxy(enum Network net, CService addrProxy, int nSocksVersion) {
return false;
if (nSocksVersion != 0 && !addrProxy.IsValid())
return false;
+ LOCK(cs_proxyInfos);
proxyInfo[net] = std::make_pair(addrProxy, nSocksVersion);
return true;
}
-bool GetProxy(enum Network net, CService &addrProxy) {
+bool GetProxy(enum Network net, proxyType &proxyInfoOut) {
assert(net >= 0 && net < NET_MAX);
+ LOCK(cs_proxyInfos);
if (!proxyInfo[net].second)
return false;
- addrProxy = proxyInfo[net].first;
+ proxyInfoOut = proxyInfo[net];
return true;
}
@@ -449,16 +452,27 @@ bool SetNameProxy(CService addrProxy, int nSocksVersion) {
return false;
if (nSocksVersion != 0 && !addrProxy.IsValid())
return false;
+ LOCK(cs_proxyInfos);
nameproxyInfo = std::make_pair(addrProxy, nSocksVersion);
return true;
}
-bool GetNameProxy() {
+bool GetNameProxy(proxyType &nameproxyInfoOut) {
+ LOCK(cs_proxyInfos);
+ if (!nameproxyInfo.second)
+ return false;
+ nameproxyInfoOut = nameproxyInfo;
+ return true;
+}
+
+bool HaveNameProxy() {
+ LOCK(cs_proxyInfos);
return nameproxyInfo.second != 0;
}
bool IsProxy(const CNetAddr &addr) {
Wladimir J. van der Laan Owner
laanwj added a note

Edit: never mind, it does exactly what it says, I misunderstood the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- for (int i=0; i<NET_MAX; i++) {
+ LOCK(cs_proxyInfos);
+ for (int i = 0; i < NET_MAX; i++) {
if (proxyInfo[i].second && (addr == (CNetAddr)proxyInfo[i].first))
return true;
}
@@ -467,10 +481,10 @@ bool IsProxy(const CNetAddr &addr) {
bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
{
- const proxyType &proxy = proxyInfo[addrDest.GetNetwork()];
+ proxyType proxy;
// no proxy needed
- if (!proxy.second)
+ if (!GetProxy(addrDest.GetNetwork(), proxy))
return ConnectSocketDirectly(addrDest, hSocketRet, nTimeout);
SOCKET hSocket = INVALID_SOCKET;
@@ -504,19 +518,22 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
SplitHostPort(string(pszDest), port, strDest);
SOCKET hSocket = INVALID_SOCKET;
- CService addrResolved(CNetAddr(strDest, fNameLookup && !nameproxyInfo.second), port);
+
+ proxyType nameproxy;
+ GetNameProxy(nameproxy);
+
+ CService addrResolved(CNetAddr(strDest, fNameLookup && !nameproxy.second), port);
if (addrResolved.IsValid()) {
addr = addrResolved;
return ConnectSocket(addr, hSocketRet, nTimeout);
}
addr = CService("0.0.0.0:0");
- if (!nameproxyInfo.second)
+ if (!nameproxy.second)
return false;
- if (!ConnectSocketDirectly(nameproxyInfo.first, hSocket, nTimeout))
+ if (!ConnectSocketDirectly(nameproxy.first, hSocket, nTimeout))
return false;
- switch(nameproxyInfo.second)
- {
+ switch(nameproxy.second) {
default:
case 4: return false;
case 5:
6 src/netbase.h
View
@@ -133,13 +133,15 @@ class CService : public CNetAddr
)
};
+typedef std::pair<CService, int> proxyType;
+
enum Network ParseNetwork(std::string net);
void SplitHostPort(std::string in, int &portOut, std::string &hostOut);
bool SetProxy(enum Network net, CService addrProxy, int nSocksVersion = 5);
-bool GetProxy(enum Network net, CService &addrProxy);
+bool GetProxy(enum Network net, proxyType &proxyInfoOut);
bool IsProxy(const CNetAddr &addr);
bool SetNameProxy(CService addrProxy, int nSocksVersion = 5);
-bool GetNameProxy();
+bool HaveNameProxy();
bool LookupHost(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions = 0, bool fAllowLookup = true);
bool LookupHostNumeric(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions = 0);
bool Lookup(const char *pszName, CService& addr, int portDefault = 0, bool fAllowLookup = true);
69 src/qt/optionsmodel.cpp
View
@@ -145,18 +145,18 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
case ProxyUse:
return settings.value("fUseProxy", false);
P. Kaufmann
Diapolo added a note

@laanwj I was asking myself, if this should be changed into, as data() returns active state for IP, port and SOCKS version, too.

        case ProxyUse: {
            proxyType proxy;
            return QVariant(GetProxy(NET_IPV4, proxy));
        }
Wladimir J. van der Laan Owner
laanwj added a note

Could be better because that better represents the current configuration, but leave that for a later pull. Aim to keep functionality precisely the same in these commits.

P. Kaufmann
Diapolo added a note

Understood :), thanks for your valuable input master!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
case ProxyIP: {
- CService addrProxy;
- if (GetProxy(NET_IPV4, addrProxy))
- return QVariant(QString::fromStdString(addrProxy.ToStringIP()));
+ proxyType proxy;
+ if (GetProxy(NET_IPV4, proxy))
+ return QVariant(QString::fromStdString(proxy.first.ToStringIP()));
else
return QVariant(QString::fromStdString("127.0.0.1"));
}
case ProxyPort: {
- CService addrProxy;
- if (GetProxy(NET_IPV4, addrProxy))
- return QVariant(addrProxy.GetPort());
+ proxyType proxy;
+ if (GetProxy(NET_IPV4, proxy))
+ return QVariant(proxy.first.GetPort());
else
- return 9050;
+ return QVariant(9050);
}
case ProxySocksVersion:
return settings.value("nSocksVersion", 5);
@@ -176,6 +176,7 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
}
return QVariant();
}
+
bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, int role)
{
bool successful = true; /* set to false on parse error */
@@ -204,29 +205,37 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
settings.setValue("fUseProxy", value.toBool());
ApplyProxySettings();
break;
- case ProxyIP:
- {
- CService addrProxy("127.0.0.1", 9050);
- GetProxy(NET_IPV4, addrProxy);
- CNetAddr addr(value.toString().toStdString());
- addrProxy.SetIP(addr);
- settings.setValue("addrProxy", addrProxy.ToStringIPPort().c_str());
- successful = ApplyProxySettings();
- }
- break;
- case ProxyPort:
- {
- CService addrProxy("127.0.0.1", 9050);
- GetProxy(NET_IPV4, addrProxy);
- addrProxy.SetPort(value.toInt());
- settings.setValue("addrProxy", addrProxy.ToStringIPPort().c_str());
- successful = ApplyProxySettings();
- }
- break;
- case ProxySocksVersion:
- settings.setValue("nSocksVersion", value.toInt());
- ApplyProxySettings();
- break;
+ case ProxyIP: {
+ proxyType proxy;
+ proxy.first = CService("127.0.0.1", 9050);
+ GetProxy(NET_IPV4, proxy);
+
+ CNetAddr addr(value.toString().toStdString());
+ proxy.first.SetIP(addr);
+ settings.setValue("addrProxy", proxy.first.ToStringIPPort().c_str());
+ successful = ApplyProxySettings();
+ }
+ break;
+ case ProxyPort: {
+ proxyType proxy;
+ proxy.first = CService("127.0.0.1", 9050);
+ GetProxy(NET_IPV4, proxy);
+
+ proxy.first.SetPort(value.toInt());
+ settings.setValue("addrProxy", proxy.first.ToStringIPPort().c_str());
+ successful = ApplyProxySettings();
+ }
+ break;
+ case ProxySocksVersion: {
+ proxyType proxy;
+ proxy.second = 5;
+ GetProxy(NET_IPV4, proxy);
+
+ proxy.second = value.toInt();
+ settings.setValue("nSocksVersion", proxy.second);
+ successful = ApplyProxySettings();
+ }
+ break;
case Fee:
nTransactionFee = value.toLongLong();
settings.setValue("nTransactionFee", nTransactionFee);
6 src/rpcwallet.cpp
View
@@ -62,8 +62,8 @@ Value getinfo(const Array& params, bool fHelp)
"getinfo\n"
"Returns an object containing various state info.");
- CService addrProxy;
- GetProxy(NET_IPV4, addrProxy);
+ proxyType proxy;
+ GetProxy(NET_IPV4, proxy);
Object obj;
obj.push_back(Pair("version", (int)CLIENT_VERSION));
@@ -72,7 +72,7 @@ Value getinfo(const Array& params, bool fHelp)
obj.push_back(Pair("balance", ValueFromAmount(pwalletMain->GetBalance())));
obj.push_back(Pair("blocks", (int)nBestHeight));
obj.push_back(Pair("connections", (int)vNodes.size()));
- obj.push_back(Pair("proxy", (addrProxy.IsValid() ? addrProxy.ToStringIPPort() : string())));
+ obj.push_back(Pair("proxy", (proxy.first.IsValid() ? proxy.first.ToStringIPPort() : string())));
obj.push_back(Pair("difficulty", (double)GetDifficulty()));
obj.push_back(Pair("testnet", fTestNet));
obj.push_back(Pair("keypoololdest", (boost::int64_t)pwalletMain->GetOldestKeyPoolTime()));
Something went wrong with that request. Please try again.