From bdb1a25ca71f6f81a504f7ad694931afbdb6c007 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 5 Feb 2008 14:15:50 +0000 Subject: [PATCH] 2008-02-05 Tatsuhiro Tsujikawa Catch exception inside DHTMessageReceiver::receiveMessage(). Log unknown message. * src/DHTMessageReceiver.{h, cc} * src/DHTMessageFactory.h * src/DHTMessageFactoryImpl.{h, cc} * src/DHTMessageTracker.cc (handleTimeout): Catch and handle exception. * src/DHTInteractionCommand.cc * src/DHTUnknownMessage.{h, cc} * test/DHTUnknownMessageTest.cc * test/MockDHTMessageFactory.h --- ChangeLog | 13 ++++++ src/DHTInteractionCommand.cc | 12 ++--- src/DHTMessageFactory.h | 3 ++ src/DHTMessageFactoryImpl.cc | 11 +++++ src/DHTMessageFactoryImpl.h | 4 ++ src/DHTMessageReceiver.cc | 80 +++++++++++++++++++------------- src/DHTMessageReceiver.h | 5 +- src/DHTMessageTracker.cc | 33 +++++++------ src/DHTUnknownMessage.cc | 87 +++++++++++++++++++++++++++++++++++ src/DHTUnknownMessage.h | 72 +++++++++++++++++++++++++++++ src/Makefile.am | 1 + src/Makefile.in | 25 +++++----- test/DHTUnknownMessageTest.cc | 44 ++++++++++++++++++ test/Makefile.am | 1 + test/Makefile.in | 1 + test/MockDHTMessageFactory.h | 7 +++ 16 files changed, 335 insertions(+), 64 deletions(-) create mode 100644 src/DHTUnknownMessage.cc create mode 100644 src/DHTUnknownMessage.h create mode 100644 test/DHTUnknownMessageTest.cc diff --git a/ChangeLog b/ChangeLog index 82cfa9bd..6aaf36d3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2008-02-05 Tatsuhiro Tsujikawa + + Catch exception inside DHTMessageReceiver::receiveMessage(). + Log unknown message. + * src/DHTMessageReceiver.{h, cc} + * src/DHTMessageFactory.h + * src/DHTMessageFactoryImpl.{h, cc} + * src/DHTMessageTracker.cc (handleTimeout): Catch and handle exception. + * src/DHTInteractionCommand.cc + * src/DHTUnknownMessage.{h, cc} + * test/DHTUnknownMessageTest.cc + * test/MockDHTMessageFactory.h + 2008-02-02 Tatsuhiro Tsujikawa Commented out ip address comparison because a host can have multiple diff --git a/src/DHTInteractionCommand.cc b/src/DHTInteractionCommand.cc index 0b4b0323..068fdafe 100644 --- a/src/DHTInteractionCommand.cc +++ b/src/DHTInteractionCommand.cc @@ -76,16 +76,14 @@ bool DHTInteractionCommand::execute() _taskQueue->executeTask(); - for(size_t i = 0; i < 20 && _readCheckSocket->isReadable(0); ++i) { - try { - _receiver->receiveMessage(); - } catch(RecoverableException* e) { - logger->error(EX_EXCEPTION_CAUGHT, e); - delete e; + for(size_t i = 0; i < 20; ++i) { + SharedHandle m = _receiver->receiveMessage(); + if(m.isNull()) { + break; } } + _receiver->handleTimeout(); try { - _receiver->handleTimeout(); _dispatcher->sendMessages(); } catch(RecoverableException* e) { logger->error(EX_EXCEPTION_CAUGHT, e); diff --git a/src/DHTMessageFactory.h b/src/DHTMessageFactory.h index 59472164..460019ab 100644 --- a/src/DHTMessageFactory.h +++ b/src/DHTMessageFactory.h @@ -102,6 +102,9 @@ public: createAnnouncePeerReplyMessage(const DHTNodeHandle& remoteNode, const string& transactionID) = 0; + virtual DHTMessageHandle + createUnknownMessage(const char* data, size_t length, const string& ipaddr, + uint16_t port) = 0; }; #endif // _D_DHT_MESSAGE_FACTORY_H_ diff --git a/src/DHTMessageFactoryImpl.cc b/src/DHTMessageFactoryImpl.cc index 12bf3d65..b3ffe51c 100644 --- a/src/DHTMessageFactoryImpl.cc +++ b/src/DHTMessageFactoryImpl.cc @@ -48,6 +48,7 @@ #include "DHTGetPeersReplyMessage.h" #include "DHTAnnouncePeerMessage.h" #include "DHTAnnouncePeerReplyMessage.h" +#include "DHTUnknownMessage.h" #include "DHTConnection.h" #include "DHTMessageDispatcher.h" #include "DHTPeerAnnounceStorage.h" @@ -387,6 +388,16 @@ DHTMessageFactoryImpl::createAnnouncePeerReplyMessage(const DHTNodeHandle& remot return m; } +DHTMessageHandle +DHTMessageFactoryImpl::createUnknownMessage(const char* data, size_t length, + const string& ipaddr, uint16_t port) + +{ + SharedHandle m = + new DHTUnknownMessage(_localNode, data, length, ipaddr, port); + return m; +} + void DHTMessageFactoryImpl::setRoutingTable(const DHTRoutingTableHandle& routingTable) { _routingTable = routingTable; diff --git a/src/DHTMessageFactoryImpl.h b/src/DHTMessageFactoryImpl.h index 1176740c..7f118ff4 100644 --- a/src/DHTMessageFactoryImpl.h +++ b/src/DHTMessageFactoryImpl.h @@ -152,6 +152,10 @@ public: createAnnouncePeerReplyMessage(const DHTNodeHandle& remoteNode, const string& transactionID); + virtual DHTMessageHandle + createUnknownMessage(const char* data, size_t length, const string& ipaddr, + uint16_t port); + void setRoutingTable(const DHTRoutingTableHandle& routingTable); void setConnection(const DHTConnectionHandle& connection); diff --git a/src/DHTMessageReceiver.cc b/src/DHTMessageReceiver.cc index f763f813..50acf2f3 100644 --- a/src/DHTMessageReceiver.cc +++ b/src/DHTMessageReceiver.cc @@ -67,40 +67,47 @@ DHTMessageHandle DHTMessageReceiver::receiveMessage() if(length <= 0) { return 0; } - bool isReply = false; - MetaEntryHandle msgroot = MetaFileUtil::bdecoding(data, length); - const Dictionary* d = dynamic_cast(msgroot.get()); - if(d) { - const Data* y = dynamic_cast(d->get("y")); - if(y->toString() == "r" || y->toString() == "e") { - isReply = true; + try { + bool isReply = false; + MetaEntryHandle msgroot = MetaFileUtil::bdecoding(data, length); + const Dictionary* d = dynamic_cast(msgroot.get()); + if(d) { + const Data* y = dynamic_cast(d->get("y")); + if(y->toString() == "r" || y->toString() == "e") { + isReply = true; + } + } else { + _logger->info("Malformed DHT message. This is not a bencoded directory. From:%s:%u", remoteAddr.c_str(), remotePort); + return handleUnknownMessage(data, sizeof(data), remoteAddr, remotePort); } - } else { - throw new DlAbortEx("Malformed DHT message. From:%s:%u", remoteAddr.c_str(), remotePort); - } - DHTMessageHandle message = 0; - DHTMessageCallbackHandle callback = 0; - if(isReply) { - std::pair p = _tracker->messageArrived(d, remoteAddr, remotePort); - message = p.first; - callback = p.second; - if(message.isNull()) { - // timeout or malicious message - return 0; + DHTMessageHandle message = 0; + DHTMessageCallbackHandle callback = 0; + if(isReply) { + std::pair p = _tracker->messageArrived(d, remoteAddr, remotePort); + message = p.first; + callback = p.second; + if(message.isNull()) { + // timeout or malicious? message + return handleUnknownMessage(data, sizeof(data), remoteAddr, remotePort); + } + } else { + message = _factory->createQueryMessage(d, remoteAddr, remotePort); } - } else { - message = _factory->createQueryMessage(d, remoteAddr, remotePort); + _logger->info("Message received: %s", message->toString().c_str()); + message->validate(); + message->doReceivedAction(); + message->getRemoteNode()->markGood(); + message->getRemoteNode()->updateLastContact(); + _routingTable->addGoodNode(message->getRemoteNode()); + if(!callback.isNull()) { + callback->onReceived(message); + } + return message; + } catch(RecoverableException* e) { + _logger->info("Exception thrown while receiving DHT message.", e); + delete e; + return handleUnknownMessage(data, sizeof(data), remoteAddr, remotePort); } - _logger->info("Message received: %s", message->toString().c_str()); - message->validate(); - message->doReceivedAction(); - message->getRemoteNode()->markGood(); - message->getRemoteNode()->updateLastContact(); - _routingTable->addGoodNode(message->getRemoteNode()); - if(!callback.isNull()) { - callback->onReceived(message); - } - return message; } void DHTMessageReceiver::handleTimeout() @@ -108,6 +115,17 @@ void DHTMessageReceiver::handleTimeout() _tracker->handleTimeout(); } +SharedHandle +DHTMessageReceiver::handleUnknownMessage(const char* data, size_t length, + const string& remoteAddr, + uint16_t remotePort) +{ + SharedHandle m = + _factory->createUnknownMessage(data, length, remoteAddr, remotePort); + _logger->info("Message received: %s", m->toString().c_str()); + return m; +} + DHTConnectionHandle DHTMessageReceiver::getConnection() const { return _connection; diff --git a/src/DHTMessageReceiver.h b/src/DHTMessageReceiver.h index b5b30d72..85dc1ddc 100644 --- a/src/DHTMessageReceiver.h +++ b/src/DHTMessageReceiver.h @@ -56,12 +56,15 @@ private: DHTRoutingTableHandle _routingTable; const Logger* _logger; + + SharedHandle + handleUnknownMessage(const char* data, size_t length, + const string& remoteAddr, uint16_t remotePort); public: DHTMessageReceiver(const DHTMessageTrackerHandle& tracker); ~DHTMessageReceiver(); - // caller must handle thrown exception. DHTMessageHandle receiveMessage(); void handleTimeout(); diff --git a/src/DHTMessageTracker.cc b/src/DHTMessageTracker.cc index cd9753a7..3c427d86 100644 --- a/src/DHTMessageTracker.cc +++ b/src/DHTMessageTracker.cc @@ -99,21 +99,26 @@ void DHTMessageTracker::handleTimeout() for(DHTMessageTrackerEntries::iterator i = _entries.begin(); i != _entries.end();) { if((*i)->isTimeout()) { - DHTMessageTrackerEntryHandle entry = *i; - i = _entries.erase(i); - DHTNodeHandle node = entry->getTargetNode(); - _logger->debug("Message timeout: To:%s:%u", - node->getIPAddress().c_str(), node->getPort()); - node->updateRTT(entry->getElapsedMillis()); - node->timeout(); - if(node->isBad()) { - _logger->debug("Marked bad: %s:%u", + try { + DHTMessageTrackerEntryHandle entry = *i; + i = _entries.erase(i); + DHTNodeHandle node = entry->getTargetNode(); + _logger->debug("Message timeout: To:%s:%u", node->getIPAddress().c_str(), node->getPort()); - _routingTable->dropNode(node); - } - DHTMessageCallbackHandle callback = entry->getCallback(); - if(!callback.isNull()) { - callback->onTimeout(node); + node->updateRTT(entry->getElapsedMillis()); + node->timeout(); + if(node->isBad()) { + _logger->debug("Marked bad: %s:%u", + node->getIPAddress().c_str(), node->getPort()); + _routingTable->dropNode(node); + } + DHTMessageCallbackHandle callback = entry->getCallback(); + if(!callback.isNull()) { + callback->onTimeout(node); + } + } catch(RecoverableException* e) { + delete e; + _logger->info("Exception thrown while handling timeouts."); } } else { ++i; diff --git a/src/DHTUnknownMessage.cc b/src/DHTUnknownMessage.cc new file mode 100644 index 00000000..bccc8ed9 --- /dev/null +++ b/src/DHTUnknownMessage.cc @@ -0,0 +1,87 @@ +/* */ +#include "DHTUnknownMessage.h" +#include "DHTNode.h" +#include "Util.h" +#include + +DHTUnknownMessage::DHTUnknownMessage(const DHTNodeHandle& localNode, + const char* data, size_t length, + const string& ipaddr, uint16_t port): + DHTMessage(localNode, 0), + _length(length), + _ipaddr(ipaddr), + _port(port) +{ + if(_length == 0) { + _data = 0; + } else { + _data = new char[length]; + memcpy(_data, data, length); + } +} + +DHTUnknownMessage::~DHTUnknownMessage() +{ + delete [] _data; +} + +void DHTUnknownMessage::doReceivedAction() {} + +void DHTUnknownMessage::send() {} + +bool DHTUnknownMessage::isReply() const +{ + return false; +} + +void DHTUnknownMessage::validate() const {} + +string DHTUnknownMessage::getMessageType() const +{ + return "unknown"; +} + +string DHTUnknownMessage::toString() const +{ + size_t sampleLength = 8; + if(_length < sampleLength) { + sampleLength = _length; + } + string sample = string(&_data[0], &_data[sampleLength]); + + return "dht unknown Remote:"+_ipaddr+":"+Util::uitos(_port)+" length="+ + Util::uitos(_length)+", first 8 bytes(hex)="+Util::toHex(sample); +} diff --git a/src/DHTUnknownMessage.h b/src/DHTUnknownMessage.h new file mode 100644 index 00000000..7034a417 --- /dev/null +++ b/src/DHTUnknownMessage.h @@ -0,0 +1,72 @@ +/* */ +#ifndef _D_DHT_UNKNOWN_MESSAGE_H_ +#define _D_DHT_UNKNOWN_MESSAGE_H_ + +#include "DHTMessage.h" + +class DHTUnknownMessage:public DHTMessage { +private: + char* _data; + size_t _length; + string _ipaddr; + uint16_t _port; +public: + // _remoteNode is always null + DHTUnknownMessage(const DHTNodeHandle& localNode, + const char* data, size_t length, + const string& ipaddr, uint16_t port); + + virtual ~DHTUnknownMessage(); + + // do nothing + virtual void doReceivedAction(); + + // do nothing; we don't use this message as outgoing message. + virtual void send(); + + // always return false + virtual bool isReply() const; + + virtual void validate() const; + + // returns "unknown" + virtual string getMessageType() const; + + // show some sample bytes + virtual string toString() const; +}; + +#endif // _D_DHT_UNKNOWN_MESSAGE_H_ diff --git a/src/Makefile.am b/src/Makefile.am index 66214950..e1690af3 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -280,6 +280,7 @@ SRCS += MetaEntry.h\ DHTGetPeersReplyMessage.cc\ DHTAnnouncePeerMessage.cc\ DHTAnnouncePeerReplyMessage.cc\ + DHTUnknownMessage.cc\ DHTMessageFactoryImpl.cc\ DHTNodeLookupTask.cc\ DHTNodeLookupEntry.cc\ diff --git a/src/Makefile.in b/src/Makefile.in index 10afee0e..98205efe 100644 --- a/src/Makefile.in +++ b/src/Makefile.in @@ -163,6 +163,7 @@ bin_PROGRAMS = aria2c$(EXEEXT) @ENABLE_BITTORRENT_TRUE@ DHTGetPeersReplyMessage.cc\ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerMessage.cc\ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerReplyMessage.cc\ +@ENABLE_BITTORRENT_TRUE@ DHTUnknownMessage.cc\ @ENABLE_BITTORRENT_TRUE@ DHTMessageFactoryImpl.cc\ @ENABLE_BITTORRENT_TRUE@ DHTNodeLookupTask.cc\ @ENABLE_BITTORRENT_TRUE@ DHTNodeLookupEntry.cc\ @@ -427,17 +428,17 @@ am__libaria2c_a_SOURCES_DIST = Socket.h SocketCore.cc SocketCore.h \ DHTPingMessage.cc DHTPingReplyMessage.cc DHTFindNodeMessage.cc \ DHTFindNodeReplyMessage.cc DHTGetPeersMessage.cc \ DHTGetPeersReplyMessage.cc DHTAnnouncePeerMessage.cc \ - DHTAnnouncePeerReplyMessage.cc DHTMessageFactoryImpl.cc \ - DHTNodeLookupTask.cc DHTNodeLookupEntry.cc BNode.cc \ - DHTMessageCallbackImpl.cc DHTAbstractTask.cc DHTPingTask.cc \ - DHTTaskQueueImpl.cc DHTBucketRefreshTask.cc \ - DHTAbstractNodeLookupTask.cc DHTPeerLookupTask.cc DHTSetup.cc \ - DHTTaskFactoryImpl.cc DHTInteractionCommand.cc \ - DHTPeerAnnounceEntry.cc DHTPeerAnnounceStorage.cc \ - DHTTokenTracker.cc DHTGetPeersCommand.cc \ - DHTTokenUpdateCommand.cc DHTBucketRefreshCommand.cc \ - DHTPeerAnnounceCommand.cc DHTReplaceNodeTask.cc \ - DHTEntryPointNameResolveCommand.cc \ + DHTAnnouncePeerReplyMessage.cc DHTUnknownMessage.cc \ + DHTMessageFactoryImpl.cc DHTNodeLookupTask.cc \ + DHTNodeLookupEntry.cc BNode.cc DHTMessageCallbackImpl.cc \ + DHTAbstractTask.cc DHTPingTask.cc DHTTaskQueueImpl.cc \ + DHTBucketRefreshTask.cc DHTAbstractNodeLookupTask.cc \ + DHTPeerLookupTask.cc DHTSetup.cc DHTTaskFactoryImpl.cc \ + DHTInteractionCommand.cc DHTPeerAnnounceEntry.cc \ + DHTPeerAnnounceStorage.cc DHTTokenTracker.cc \ + DHTGetPeersCommand.cc DHTTokenUpdateCommand.cc \ + DHTBucketRefreshCommand.cc DHTPeerAnnounceCommand.cc \ + DHTReplaceNodeTask.cc DHTEntryPointNameResolveCommand.cc \ DHTRoutingTableSerializer.cc DHTRoutingTableDeserializer.cc \ DHTAutoSaveCommand.cc DHTRegistry.cc Metalinker.cc \ Metalinker.h MetalinkEntry.cc MetalinkEntry.h \ @@ -544,6 +545,7 @@ am__libaria2c_a_SOURCES_DIST = Socket.h SocketCore.cc SocketCore.h \ @ENABLE_BITTORRENT_TRUE@ DHTGetPeersReplyMessage.$(OBJEXT) \ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerMessage.$(OBJEXT) \ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerReplyMessage.$(OBJEXT) \ +@ENABLE_BITTORRENT_TRUE@ DHTUnknownMessage.$(OBJEXT) \ @ENABLE_BITTORRENT_TRUE@ DHTMessageFactoryImpl.$(OBJEXT) \ @ENABLE_BITTORRENT_TRUE@ DHTNodeLookupTask.$(OBJEXT) \ @ENABLE_BITTORRENT_TRUE@ DHTNodeLookupEntry.$(OBJEXT) \ @@ -1189,6 +1191,7 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DHTTaskQueueImpl.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DHTTokenTracker.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DHTTokenUpdateCommand.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DHTUnknownMessage.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DHTUtil.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/Data.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DefaultAuthResolver.Po@am__quote@ diff --git a/test/DHTUnknownMessageTest.cc b/test/DHTUnknownMessageTest.cc new file mode 100644 index 00000000..b9f966f6 --- /dev/null +++ b/test/DHTUnknownMessageTest.cc @@ -0,0 +1,44 @@ +#include "DHTUnknownMessage.h" +#include "DHTNode.h" +#include "Exception.h" +#include + +class DHTUnknownMessageTest:public CppUnit::TestFixture { + + CPPUNIT_TEST_SUITE(DHTUnknownMessageTest); + CPPUNIT_TEST(testToString); + CPPUNIT_TEST_SUITE_END(); +public: + void setUp() {} + + void tearDown() {} + + void testToString(); +}; + + +CPPUNIT_TEST_SUITE_REGISTRATION(DHTUnknownMessageTest); + +void DHTUnknownMessageTest::testToString() +{ + DHTNodeHandle localNode = new DHTNode(); + string ipaddr = "192.168.0.1"; + uint16_t port = 6881; + + { + // data.size() > 8 + string data = "chocolate"; + DHTUnknownMessage msg(localNode, remoteNode, data.c_str(), data.size(), + ipaddr, port); + + CPPUNIT_ASSERT_EQUAL(string("dht unknown Remote:192.168.0.1:6881 length=9, first 8 bytes(hex)=63686f636f6c617465"), msg.toString()); + } + { + // data.size() == 3 + string data = "foo"; + DHTUnknownMessage msg(localNode, remoteNode, data.c_str(), data.size(), + ipaddr, port); + + CPPUNIT_ASSERT_EQUAL(string("dht unknown Remote:192.168.0.1:6881 length=9, first 8 bytes(hex)=66666f"), msg.toString()); + } +} diff --git a/test/Makefile.am b/test/Makefile.am index 098332d8..e8506096 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -121,6 +121,7 @@ aria2c_SOURCES += BtAllowedFastMessageTest.cc\ DHTGetPeersReplyMessageTest.cc\ DHTAnnouncePeerMessageTest.cc\ DHTAnnouncePeerReplyMessageTest.cc\ + DHTUnknownMessageTest.cc\ DHTMessageFactoryImplTest.cc\ BNodeTest.cc\ DHTPeerAnnounceEntryTest.cc\ diff --git a/test/Makefile.in b/test/Makefile.in index 14fc21ff..f0b7d25f 100644 --- a/test/Makefile.in +++ b/test/Makefile.in @@ -935,6 +935,7 @@ uninstall-am: uninstall-info-am @ENABLE_BITTORRENT_TRUE@ DHTGetPeersReplyMessageTest.cc\ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerMessageTest.cc\ @ENABLE_BITTORRENT_TRUE@ DHTAnnouncePeerReplyMessageTest.cc\ +@ENABLE_BITTORRENT_TRUE@ DHTUnknownMessageTest.cc\ @ENABLE_BITTORRENT_TRUE@ DHTMessageFactoryImplTest.cc\ @ENABLE_BITTORRENT_TRUE@ BNodeTest.cc\ @ENABLE_BITTORRENT_TRUE@ DHTPeerAnnounceEntryTest.cc\ diff --git a/test/MockDHTMessageFactory.h b/test/MockDHTMessageFactory.h index f54617fc..bbf83964 100644 --- a/test/MockDHTMessageFactory.h +++ b/test/MockDHTMessageFactory.h @@ -107,6 +107,13 @@ public: return 0; } + virtual DHTMessageHandle + createUnknownMessage(const char* data, size_t length, const string& ipaddr, + uint16_t port) + { + return 0; + } + void setLocalNode(const DHTNodeHandle& node) { _localNode = node;