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

Diapolo commented at 10:58 AM on September 23, 2012: none

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

in src/netbase.cpp:None in 917f9e5ced outdated

  18 | @@ -18,7 +19,9 @@
  19 |  // Settings
  20 |  typedef std::pair<CService, int> proxyType;
  21 |  static proxyType proxyInfo[NET_MAX];
  22 | +static CCriticalSection cs_proxyInfo;
  23 |  static proxyType nameproxyInfo;
  24 | +static CCriticalSection cs_nameproxyInfo;

laanwj commented at 12:38 PM on September 23, 2012:

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

Diapolo commented at 4:24 PM on September 23, 2012:

Indeed, fixed!

BitcoinPullTester commented at 1:22 PM on September 24, 2012: none

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

laanwj commented at 4:28 PM on September 24, 2012: member

ACK

in src/netbase.cpp:None in bd5f5de6b8 outdated

 473 | @@ -467,6 +474,7 @@ bool IsProxy(const CNetAddr &addr) {
 474 |  
 475 |  bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
 476 |  {
 477 | +    LOCK(cs_proxyInfos);

gavinandresen commented at 8:31 PM on September 25, 2012:

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

laanwj commented at 10:26 AM on September 26, 2012:

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

Diapolo commented at 2:04 PM on September 26, 2012:

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.

laanwj commented at 6:54 AM on September 27, 2012:

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.

in src/netbase.cpp:None in bd5f5de6b8 outdated

 511 | @@ -504,6 +512,7 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
 512 |      SplitHostPort(string(pszDest), port, strDest);
 513 |  
 514 |      SOCKET hSocket = INVALID_SOCKET;
 515 | +    LOCK(cs_proxyInfos);

gavinandresen commented at 8:32 PM on September 25, 2012:

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

Diapolo commented at 6:40 AM on September 28, 2012: none

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

Diapolo commented at 11:56 AM on September 28, 2012: none

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.

in src/netbase.h:None in 511b58723b outdated

 138 | @@ -139,7 +139,7 @@ class CService : public CNetAddr
 139 |  bool GetProxy(enum Network net, CService &addrProxy);
 140 |  bool IsProxy(const CNetAddr &addr);
 141 |  bool SetNameProxy(CService addrProxy, int nSocksVersion = 5);
 142 | -bool GetNameProxy();
 143 | +bool IsNameProxy();

sipa commented at 2:23 PM on September 28, 2012:

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

Diapolo commented at 2:35 PM on September 28, 2012:

Seems better suited, yes.

in src/netbase.cpp:None in 5163a25776 outdated

 479 | +bool HaveNameProxy() {
 480 | +    LOCK(cs_proxyInfos);
 481 |      return nameproxyInfo.second != 0;
 482 |  }
 483 |  
 484 |  bool IsProxy(const CNetAddr &addr) {

laanwj commented at 3:56 PM on September 28, 2012:

<i>Edit: never mind, it does exactly what it says, I misunderstood the function.</i>

in src/netbase.cpp:None in 5163a25776 outdated

 446 |          return false;
 447 |      addrProxy = proxyInfo[net].first;
 448 |      return true;
 449 |  }
 450 |  
 451 | +bool GetProxyPair(enum Network net, proxyType &proxyInfoOut) {

laanwj commented at 3:59 PM on September 28, 2012:

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.

Diapolo commented at 4:32 PM on September 28, 2012:

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.

laanwj commented at 4:52 PM on September 28, 2012:

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.

sipa commented at 4:52 PM on September 28, 2012:

Agree with @laanwj

Diapolo commented at 4:57 PM on September 28, 2012:

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

Diapolo commented at 9:43 AM on September 29, 2012: none

@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).

in src/qt/optionsmodel.cpp:None in f7536f881c outdated

 144 | @@ -145,21 +145,26 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
 145 |          case ProxyUse:
 146 |              return settings.value("fUseProxy", false);

Diapolo commented at 9:46 AM on September 29, 2012:

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

<pre> case ProxyUse: { proxyType proxy; return QVariant(GetProxy(NET_IPV4, proxy)); } </pre>

laanwj commented at 9:51 AM on September 29, 2012:

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.

Diapolo commented at 10:10 AM on September 29, 2012:

Understood :), thanks for your valuable input master!

in src/netbase.cpp:None in f7536f881c outdated

 480 | @@ -467,7 +481,9 @@ bool IsProxy(const CNetAddr &addr) {
 481 |  
 482 |  bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
 483 |  {
 484 | -    const proxyType &proxy = proxyInfo[addrDest.GetNetwork()];
 485 | +    proxyType proxy;
 486 | +    if (!GetProxy(addrDest.GetNetwork(), proxy))
 487 | +        return false;

laanwj commented at 9:54 AM on September 29, 2012:

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

Diapolo commented at 10:00 AM on September 29, 2012:

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 :)?

laanwj commented at 10:04 AM on September 29, 2012:

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

in src/netbase.cpp:None in f7536f881c outdated

 519 | @@ -504,19 +520,23 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
 520 |      SplitHostPort(string(pszDest), port, strDest);
 521 |  
 522 |      SOCKET hSocket = INVALID_SOCKET;
 523 | -    CService addrResolved(CNetAddr(strDest, fNameLookup && !nameproxyInfo.second), port);
 524 | +
 525 | +    proxyType nameproxy;
 526 | +    if (!GetNameProxy(nameproxy))

laanwj commented at 9:54 AM on September 29, 2012:

Same here?

sipa commented at 1:08 PM on September 30, 2012: member

ACK on changes to core.

BitcoinPullTester commented at 9:13 PM on September 30, 2012: none

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

Diapolo commented at 9:17 PM on September 30, 2012: none

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

laanwj commented at 5:50 AM on October 2, 2012: member

ACK

Diapolo commented at 10:14 PM on October 2, 2012: none

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

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

81bbef2609

in src/netbase.cpp:None in 32613fc936 outdated

 480 | @@ -467,7 +481,8 @@ bool IsProxy(const CNetAddr &addr) {
 481 |  
 482 |  bool ConnectSocket(const CService &addrDest, SOCKET& hSocketRet, int nTimeout)
 483 |  {
 484 | -    const proxyType &proxy = proxyInfo[addrDest.GetNetwork()];
 485 | +    proxyType proxy;
 486 | +    GetProxy(addrDest.GetNetwork(), proxy);

laanwj commented at 2:21 AM on October 4, 2012:

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

Diapolo commented at 7:32 AM on October 4, 2012:

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

Diapolo commented at 7:43 AM on October 5, 2012: none

Can we get this fix into 0.7.1?

laanwj commented at 8:26 AM on October 5, 2012: member

Yes as I said on the mailing list, that's the goal.

Avoiding corner-case random crashes is always good.

sipa referenced this in commit 43de64949c on Oct 7, 2012

sipa merged this on Oct 7, 2012

sipa closed this on Oct 7, 2012

DrahtBot locked this on Sep 8, 2021