From 9eaa3756f8dc711b6b828c8336ed34185b4b5834 Mon Sep 17 00:00:00 2001
From: Adam Ierymenko <adam.ierymenko@gmail.com>
Date: Fri, 30 Sep 2016 12:22:54 -0700
Subject: Fix deadlock-causing regression in Network.
---
node/Network.cpp | 230 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 124 insertions(+), 106 deletions(-)
(limited to 'node')
diff --git a/node/Network.cpp b/node/Network.cpp
index ac679e46..d38a3fdd 100644
--- a/node/Network.cpp
+++ b/node/Network.cpp
@@ -886,118 +886,130 @@ uint64_t Network::handleConfigChunk(const Packet &chunk,unsigned int ptr)
const unsigned int chunkLen = chunk.at<uint16_t>(ptr); ptr += 2;
const void *chunkData = chunk.field(ptr,chunkLen); ptr += chunkLen;
- Mutex::Lock _l(_lock);
-
- _IncomingConfigChunk *c = (_IncomingConfigChunk *)0;
- uint64_t chunkId = 0;
+ NetworkConfig *nc = (NetworkConfig *)0;
uint64_t configUpdateId;
- unsigned long totalLength,chunkIndex;
- if (ptr < chunk.size()) {
- const bool fastPropagate = ((chunk[ptr++] & 0x01) != 0);
- configUpdateId = chunk.at<uint64_t>(ptr); ptr += 8;
- totalLength = chunk.at<uint32_t>(ptr); ptr += 4;
- chunkIndex = chunk.at<uint32_t>(ptr); ptr += 4;
-
- if (((chunkIndex + chunkLen) > totalLength)||(totalLength >= ZT_NETWORKCONFIG_DICT_CAPACITY)) { // >= since we need room for a null at the end
- TRACE("discarded chunk from %s: invalid length or length overflow",chunk.source().toString().c_str());
- return 0;
- }
+ {
+ Mutex::Lock _l(_lock);
+
+ _IncomingConfigChunk *c = (_IncomingConfigChunk *)0;
+ uint64_t chunkId = 0;
+ unsigned long totalLength,chunkIndex;
+ if (ptr < chunk.size()) {
+ const bool fastPropagate = ((chunk[ptr++] & 0x01) != 0);
+ configUpdateId = chunk.at<uint64_t>(ptr); ptr += 8;
+ totalLength = chunk.at<uint32_t>(ptr); ptr += 4;
+ chunkIndex = chunk.at<uint32_t>(ptr); ptr += 4;
+
+ if (((chunkIndex + chunkLen) > totalLength)||(totalLength >= ZT_NETWORKCONFIG_DICT_CAPACITY)) { // >= since we need room for a null at the end
+ TRACE("discarded chunk from %s: invalid length or length overflow",chunk.source().toString().c_str());
+ return 0;
+ }
- if ((chunk[ptr] != 1)||(chunk.at<uint16_t>(ptr + 1) != ZT_C25519_SIGNATURE_LEN)) {
- TRACE("discarded chunk from %s: unrecognized signature type",chunk.source().toString().c_str());
- return 0;
- }
- const uint8_t *sig = reinterpret_cast<const uint8_t *>(chunk.field(ptr + 3,ZT_C25519_SIGNATURE_LEN));
+ if ((chunk[ptr] != 1)||(chunk.at<uint16_t>(ptr + 1) != ZT_C25519_SIGNATURE_LEN)) {
+ TRACE("discarded chunk from %s: unrecognized signature type",chunk.source().toString().c_str());
+ return 0;
+ }
+ const uint8_t *sig = reinterpret_cast<const uint8_t *>(chunk.field(ptr + 3,ZT_C25519_SIGNATURE_LEN));
- // We can use the signature, which is unique per chunk, to get a per-chunk ID for local deduplication use
- for(unsigned int i=0;i<16;++i)
- reinterpret_cast<uint8_t *>(&chunkId)[i & 7] ^= sig[i];
+ // We can use the signature, which is unique per chunk, to get a per-chunk ID for local deduplication use
+ for(unsigned int i=0;i<16;++i)
+ reinterpret_cast<uint8_t *>(&chunkId)[i & 7] ^= sig[i];
- // Find existing or new slot for this update and check if this is a duplicate chunk
- for(int i=0;i<ZT_NETWORK_MAX_INCOMING_UPDATES;++i) {
- if (_incomingConfigChunks[i].updateId == configUpdateId) {
- c = &(_incomingConfigChunks[i]);
+ // Find existing or new slot for this update and check if this is a duplicate chunk
+ for(int i=0;i<ZT_NETWORK_MAX_INCOMING_UPDATES;++i) {
+ if (_incomingConfigChunks[i].updateId == configUpdateId) {
+ c = &(_incomingConfigChunks[i]);
- for(unsigned long j=0;j<c->haveChunks;++j) {
- if (c->haveChunkIds[j] == chunkId)
- return 0;
- }
+ for(unsigned long j=0;j<c->haveChunks;++j) {
+ if (c->haveChunkIds[j] == chunkId)
+ return 0;
+ }
- break;
- } else if ((!c)||(_incomingConfigChunks[i].ts < c->ts)) {
- c = &(_incomingConfigChunks[i]);
+ break;
+ } else if ((!c)||(_incomingConfigChunks[i].ts < c->ts)) {
+ c = &(_incomingConfigChunks[i]);
+ }
}
- }
- // If it's not a duplicate, check chunk signature
- const Identity controllerId(RR->topology->getIdentity(controller()));
- if (!controllerId) { // we should always have the controller identity by now, otherwise how would we have queried it the first time?
- TRACE("unable to verify chunk from %s: don't have controller identity",chunk.source().toString().c_str());
- return 0;
- }
- if (!controllerId.verify(chunk.field(start,ptr - start),ptr - start,sig,ZT_C25519_SIGNATURE_LEN)) {
- TRACE("discarded chunk from %s: signature check failed",chunk.source().toString().c_str());
- return 0;
- }
+ // If it's not a duplicate, check chunk signature
+ const Identity controllerId(RR->topology->getIdentity(controller()));
+ if (!controllerId) { // we should always have the controller identity by now, otherwise how would we have queried it the first time?
+ TRACE("unable to verify chunk from %s: don't have controller identity",chunk.source().toString().c_str());
+ return 0;
+ }
+ if (!controllerId.verify(chunk.field(start,ptr - start),ptr - start,sig,ZT_C25519_SIGNATURE_LEN)) {
+ TRACE("discarded chunk from %s: signature check failed",chunk.source().toString().c_str());
+ return 0;
+ }
- // New properly verified chunks can be flooded "virally" through the network
- if (fastPropagate) {
- Address *a = (Address *)0;
- Membership *m = (Membership *)0;
- Hashtable<Address,Membership>::Iterator i(_memberships);
- while (i.next(a,m)) {
- if ((*a != chunk.source())&&(*a != controller())) {
- Packet outp(*a,RR->identity.address(),Packet::VERB_NETWORK_CONFIG);
- outp.append(reinterpret_cast<const uint8_t *>(chunk.data()) + start,chunk.size() - start);
- RR->sw->send(outp,true);
+ // New properly verified chunks can be flooded "virally" through the network
+ if (fastPropagate) {
+ Address *a = (Address *)0;
+ Membership *m = (Membership *)0;
+ Hashtable<Address,Membership>::Iterator i(_memberships);
+ while (i.next(a,m)) {
+ if ((*a != chunk.source())&&(*a != controller())) {
+ Packet outp(*a,RR->identity.address(),Packet::VERB_NETWORK_CONFIG);
+ outp.append(reinterpret_cast<const uint8_t *>(chunk.data()) + start,chunk.size() - start);
+ RR->sw->send(outp,true);
+ }
}
}
- }
- } else if (chunk.source() == controller()) {
- // Legacy support for OK(NETWORK_CONFIG_REQUEST) from older controllers
- chunkId = chunk.packetId();
- configUpdateId = chunkId;
- totalLength = chunkLen;
- chunkIndex = 0;
-
- if (totalLength >= ZT_NETWORKCONFIG_DICT_CAPACITY)
+ } else if (chunk.source() == controller()) {
+ // Legacy support for OK(NETWORK_CONFIG_REQUEST) from older controllers
+ chunkId = chunk.packetId();
+ configUpdateId = chunkId;
+ totalLength = chunkLen;
+ chunkIndex = 0;
+
+ if (totalLength >= ZT_NETWORKCONFIG_DICT_CAPACITY)
+ return 0;
+
+ for(int i=0;i<ZT_NETWORK_MAX_INCOMING_UPDATES;++i) {
+ if ((!c)||(_incomingConfigChunks[i].ts < c->ts))
+ c = &(_incomingConfigChunks[i]);
+ }
+ } else {
+ TRACE("discarded single-chunk unsigned legacy config: this is only allowed if the sender is the controller itself");
return 0;
-
- for(int i=0;i<ZT_NETWORK_MAX_INCOMING_UPDATES;++i) {
- if ((!c)||(_incomingConfigChunks[i].ts < c->ts))
- c = &(_incomingConfigChunks[i]);
}
- } else {
- TRACE("discarded single-chunk unsigned legacy config: this is only allowed if the sender is the controller itself");
- return 0;
- }
- ++c->ts; // newer is higher, that's all we need
+ ++c->ts; // newer is higher, that's all we need
- if (c->updateId != configUpdateId) {
- c->updateId = configUpdateId;
- c->haveChunks = 0;
- c->haveBytes = 0;
- }
- if (c->haveChunks >= ZT_NETWORK_MAX_UPDATE_CHUNKS)
- return false;
- c->haveChunkIds[c->haveChunks++] = chunkId;
+ if (c->updateId != configUpdateId) {
+ c->updateId = configUpdateId;
+ c->haveChunks = 0;
+ c->haveBytes = 0;
+ }
+ if (c->haveChunks >= ZT_NETWORK_MAX_UPDATE_CHUNKS)
+ return false;
+ c->haveChunkIds[c->haveChunks++] = chunkId;
- memcpy(c->data.unsafeData() + chunkIndex,chunkData,chunkLen);
- c->haveBytes += chunkLen;
+ memcpy(c->data.unsafeData() + chunkIndex,chunkData,chunkLen);
+ c->haveBytes += chunkLen;
- if (c->haveBytes == totalLength) {
- c->data.unsafeData()[c->haveBytes] = (char)0; // ensure null terminated
+ if (c->haveBytes == totalLength) {
+ c->data.unsafeData()[c->haveBytes] = (char)0; // ensure null terminated
- NetworkConfig *const nc = new NetworkConfig();
- try {
- if (nc->fromDictionary(c->data)) {
- this->_setConfiguration(*nc,true);
+ nc = new NetworkConfig();
+ try {
+ if (!nc->fromDictionary(c->data)) {
+ delete nc;
+ nc = (NetworkConfig *)0;
+ }
+ } catch ( ... ) {
delete nc;
- return configUpdateId;
+ nc = (NetworkConfig *)0;
}
- } catch ( ... ) {}
+ }
+ }
+
+ if (nc) {
+ this->_setConfiguration(*nc,true);
delete nc;
+ return configUpdateId;
+ } else {
+ return 0;
}
return 0;
@@ -1027,10 +1039,9 @@ void Network::requestConfiguration()
NetworkConfig *nconf = new NetworkConfig();
try {
switch(RR->localNetworkController->doNetworkConfigRequest(InetAddress(),RR->identity,RR->identity,_id,rmd,*nconf)) {
- case NetworkController::NETCONF_QUERY_OK: {
- Mutex::Lock _l(_lock);
+ case NetworkController::NETCONF_QUERY_OK:
this->_setConfiguration(*nconf,true);
- } break;
+ break;
case NetworkController::NETCONF_QUERY_OBJECT_NOT_FOUND:
this->setNotFound();
break;
@@ -1238,7 +1249,7 @@ ZT_VirtualNetworkStatus Network::_status() const
int Network::_setConfiguration(const NetworkConfig &nconf,bool saveToDisk)
{
- // assumes _lock is locked
+ // _lock is NOT locked when this is called
try {
if ((nconf.issuedTo != RR->identity.address())||(nconf.networkId != _id))
return 0;
@@ -1246,20 +1257,27 @@ int Network::_setConfiguration(const NetworkConfig &nconf,bool saveToDisk)
return 1; // OK config, but duplicate of what we already have
ZT_VirtualNetworkConfig ctmp;
- _config = nconf;
- _lastConfigUpdate = RR->node->now();
- _netconfFailure = NETCONF_FAILURE_NONE;
- _externalConfig(&ctmp);
- const bool oldPortInitialized = _portInitialized;
- _portInitialized = true;
+ bool oldPortInitialized;
+ {
+ Mutex::Lock _l(_lock);
+ _config = nconf;
+ _lastConfigUpdate = RR->node->now();
+ _netconfFailure = NETCONF_FAILURE_NONE;
+ oldPortInitialized = _portInitialized;
+ _portInitialized = true;
+ _externalConfig(&ctmp);
+ }
_portError = RR->node->configureVirtualNetworkPort(_id,&_uPtr,(oldPortInitialized) ? ZT_VIRTUAL_NETWORK_CONFIG_OPERATION_CONFIG_UPDATE : ZT_VIRTUAL_NETWORK_CONFIG_OPERATION_UP,&ctmp);
if (saveToDisk) {
- char n[64];
- Utils::snprintf(n,sizeof(n),"networks.d/%.16llx.conf",_id);
- Dictionary<ZT_NETWORKCONFIG_DICT_CAPACITY> d;
- if (nconf.toDictionary(d,false))
- RR->node->dataStorePut(n,(const void *)d.data(),d.sizeBytes(),true);
+ Dictionary<ZT_NETWORKCONFIG_DICT_CAPACITY> *d = new Dictionary<ZT_NETWORKCONFIG_DICT_CAPACITY>();
+ try {
+ char n[64];
+ Utils::snprintf(n,sizeof(n),"networks.d/%.16llx.conf",_id);
+ if (nconf.toDictionary(*d,false))
+ RR->node->dataStorePut(n,(const void *)d->data(),d->sizeBytes(),true);
+ } catch ( ... ) {}
+ delete d;
}
return 2; // OK and configuration has changed
--
cgit v1.2.3