2009-11-26 Tatsuhiro Tsujikawa <t-tujikawa@users.sourceforge.net>

	Fixed the bug which causes segmentation fault with tellWaiting
	XML-RPC method when BitTorrent download is waiting.  The key of
	BtRegistry is changed from info hash to GID, because it is
	registered per RequestGroup, not info hash.
	* src/BtRegistry.cc
	* src/BtRegistry.h
	* src/BtSetup.cc
	* src/ConsoleStatCalc.cc
	* src/InitiatorMSEHandshakeCommand.cc
	* src/PeerInitiateConnectionCommand.cc
	* src/PeerInteractionCommand.cc
	* src/PeerInteractionCommand.h
	* src/PeerReceiveHandshakeCommand.cc
	* src/RequestGroup.cc
	* src/XmlRpcMethodImpl.cc
	* test/BtRegistryTest.cc
	* test/XmlRpcMethodTest.cc

ChangeLog

@@ -1,3 +1,23 @@
+2009-11-26  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
+
+	Fixed the bug which causes segmentation fault with tellWaiting
+	XML-RPC method when BitTorrent download is waiting.  The key of
+	BtRegistry is changed from info hash to GID, because it is
+	registered per RequestGroup, not info hash.
+	* src/BtRegistry.cc
+	* src/BtRegistry.h
+	* src/BtSetup.cc
+	* src/ConsoleStatCalc.cc
+	* src/InitiatorMSEHandshakeCommand.cc
+	* src/PeerInitiateConnectionCommand.cc
+	* src/PeerInteractionCommand.cc
+	* src/PeerInteractionCommand.h
+	* src/PeerReceiveHandshakeCommand.cc
+	* src/RequestGroup.cc
+	* src/XmlRpcMethodImpl.cc
+	* test/BtRegistryTest.cc
+	* test/XmlRpcMethodTest.cc
+
 2009-11-25  Tatsuhiro Tsujikawa  <t-tujikawa@users.sourceforge.net>
 
 	Updated man page for changeOption XML-RPC method.

src/BtRegistry.cc

@@ -40,25 +40,40 @@
 #include "BtAnnounce.h"
 #include "BtRuntime.h"
 #include "BtProgressInfoFile.h"
+#include "bittorrent_helper.h"
 
 namespace aria2 {
 
+SharedHandle<DownloadContext>
+BtRegistry::getDownloadContext(int32_t gid) const
+{
+  return get(gid)._downloadContext;
+}
+
 SharedHandle<DownloadContext>
 BtRegistry::getDownloadContext(const std::string& infoHash) const
 {
-  return get(infoHash)._downloadContext;
+  SharedHandle<DownloadContext> dctx;
+  for(std::map<int32_t, BtObject>::const_iterator i = _pool.begin();
+      i != _pool.end(); ++i) {
+    const BDE& attrs =
+      (*i).second._downloadContext->getAttribute(bittorrent::BITTORRENT);
+    if(attrs[bittorrent::INFO_HASH].s() == infoHash) {
+      dctx = (*i).second._downloadContext;
+      break;
+    }
+  }
+  return dctx;
 }
 
-void BtRegistry::put(const std::string& infoHash, const BtObject& obj)
+void BtRegistry::put(int32_t gid, const BtObject& obj)
 {
-  remove(infoHash);
-  std::map<std::string, BtObject>::value_type p(infoHash, obj);
-  _pool.insert(p);
+  _pool[gid] = obj;
 }
 
-BtObject BtRegistry::get(const std::string& infoHash) const
+BtObject BtRegistry::get(int32_t gid) const
 {
-  std::map<std::string, BtObject>::const_iterator i = _pool.find(infoHash);
+  std::map<int32_t, BtObject>::const_iterator i = _pool.find(gid);
   if(i == _pool.end()) {
     return BtObject();
   } else {
@@ -66,9 +81,9 @@ BtObject BtRegistry::get(const std::string& infoHash) const
   }
 }
 
-bool BtRegistry::remove(const std::string& infoHash)
+bool BtRegistry::remove(int32_t gid)
 {
-  return _pool.erase(infoHash);
+  return _pool.erase(gid);
 }
 
 void BtRegistry::removeAll() {

src/BtRegistry.h

@@ -37,7 +37,6 @@
 
 #include "common.h"
 
-#include <string>
 #include <map>
 
 #include "SharedHandle.h"
@@ -87,19 +86,22 @@ struct BtObject {
 
 class BtRegistry {
 private:
-  std::map<std::string, BtObject> _pool;
+  std::map<int32_t, BtObject> _pool;
 public:
+  SharedHandle<DownloadContext>
+  getDownloadContext(int32_t gid) const;
+
   SharedHandle<DownloadContext>
   getDownloadContext(const std::string& infoHash) const;
 
-  void put(const std::string& infoHash, const BtObject& obj);
+  void put(int32_t gid, const BtObject& obj);
 
-  BtObject get(const std::string& infoHash) const;
+  BtObject get(int32_t gid) const;
 
   template<typename OutputIterator>
   OutputIterator getAllDownloadContext(OutputIterator dest)
   {
-    for(std::map<std::string, BtObject>::const_iterator i = _pool.begin();
+    for(std::map<int32_t, BtObject>::const_iterator i = _pool.begin();
 	i != _pool.end(); ++i) {
       *dest++ = (*i).second._downloadContext;
     }
@@ -108,7 +110,7 @@ public:
 
   void removeAll();
 
-  bool remove(const std::string& infoHash);
+  bool remove(int32_t gid);
 };
 
 } // namespace aria2

src/BtSetup.cc

@@ -76,8 +76,7 @@ void BtSetup::setup(std::deque<Command*>& commands,
   const BDE& torrentAttrs =
     requestGroup->getDownloadContext()->getAttribute(bittorrent::BITTORRENT);
   bool metadataGetMode = !torrentAttrs.containsKey(bittorrent::METADATA);
-  BtObject btObject =
-    e->getBtRegistry()->get(torrentAttrs[bittorrent::INFO_HASH].s());
+  BtObject btObject = e->getBtRegistry()->get(requestGroup->getGID());
   SharedHandle<PieceStorage> pieceStorage = btObject._pieceStorage;
   SharedHandle<PeerStorage> peerStorage = btObject._peerStorage;
   SharedHandle<BtRuntime> btRuntime = btObject._btRuntime;

src/ConsoleStatCalc.cc

@@ -116,10 +116,8 @@ static void printProgress
     << rg->getNumConnection();
 #ifdef ENABLE_BITTORRENT
   if(rg->getDownloadContext()->hasAttribute(bittorrent::BITTORRENT)) {
-    const BDE& torrentAttrs =
-      rg->getDownloadContext()->getAttribute(bittorrent::BITTORRENT);
     SharedHandle<PeerStorage> ps =
-      e->getBtRegistry()->get(torrentAttrs[bittorrent::INFO_HASH].s())._peerStorage;
+      e->getBtRegistry()->get(rg->getGID())._peerStorage;
     std::deque<SharedHandle<Peer> > peers;
     ps->getActivePeers(peers);
     o << " " << "SEED:"

src/InitiatorMSEHandshakeCommand.cc

@@ -152,11 +152,10 @@ bool InitiatorMSEHandshakeCommand::executeInternal() {
       }
       PeerInteractionCommand* c =
 	  new PeerInteractionCommand
-	(cuid, _requestGroup, peer, e, _btRuntime, _pieceStorage,
+	(cuid, _requestGroup, peer, e, _btRuntime, _pieceStorage, _peerStorage,
 	 socket,
 	 PeerInteractionCommand::INITIATOR_SEND_HANDSHAKE,
 	 peerConnection);
-      c->setPeerStorage(_peerStorage);
       e->commands.push_back(c);
       return true;
     }

src/PeerInitiateConnectionCommand.cc

@@ -89,9 +89,8 @@ bool PeerInitiateConnectionCommand::executeInternal() {
   } else {
     PeerInteractionCommand* command =
       new PeerInteractionCommand
-      (cuid, _requestGroup, peer, e, _btRuntime, _pieceStorage,
+      (cuid, _requestGroup, peer, e, _btRuntime, _pieceStorage, _peerStorage,
        socket, PeerInteractionCommand::INITIATOR_SEND_HANDSHAKE);
-    command->setPeerStorage(_peerStorage);
     e->commands.push_back(command);
   }
   return true;

src/PeerInteractionCommand.cc

@@ -45,7 +45,6 @@
 #include "Socket.h"
 #include "Option.h"
 #include "DownloadContext.h"
-#include "BtRegistry.h"
 #include "Peer.h"
 #include "BtMessage.h"
 #include "BtRuntime.h"
@@ -83,6 +82,7 @@ PeerInteractionCommand::PeerInteractionCommand
  DownloadEngine* e,
  const SharedHandle<BtRuntime>& btRuntime,
  const SharedHandle<PieceStorage>& pieceStorage,
+ const SharedHandle<PeerStorage>& peerStorage,
  const SocketHandle& s,
  Seq sequence,
  const PeerConnectionHandle& passedPeerConnection)
@@ -90,6 +90,7 @@ PeerInteractionCommand::PeerInteractionCommand
    _requestGroup(requestGroup),
    _btRuntime(btRuntime),
    _pieceStorage(pieceStorage),
+   _peerStorage(peerStorage),
    sequence(sequence)
 {
   // TODO move following bunch of processing to separate method, like init()
@@ -101,9 +102,6 @@ PeerInteractionCommand::PeerInteractionCommand
 
   const BDE& torrentAttrs =
     _requestGroup->getDownloadContext()->getAttribute(bittorrent::BITTORRENT);
-  SharedHandle<BtRegistry> btRegistry = e->getBtRegistry();
-  SharedHandle<PeerStorage> peerStorage =
-    btRegistry->get(torrentAttrs[bittorrent::INFO_HASH].s())._peerStorage;
 
   bool metadataGetMode = !torrentAttrs.containsKey(bittorrent::METADATA);
 
@@ -354,12 +352,6 @@ bool PeerInteractionCommand::exitBeforeExecute()
   return _btRuntime->isHalt();
 }
 
-void PeerInteractionCommand::setPeerStorage
-(const SharedHandle<PeerStorage>& peerStorage)
-{
-  _peerStorage = peerStorage;
-}
-
 const SharedHandle<Option>& PeerInteractionCommand::getOption() const
 {
   return _requestGroup->getOption();

src/PeerInteractionCommand.h

@@ -80,14 +80,13 @@ public:
 			 DownloadEngine* e,
 			 const SharedHandle<BtRuntime>& btRuntime,
 			 const SharedHandle<PieceStorage>& pieceStorage,
+			 const SharedHandle<PeerStorage>& peerStorage,
 			 const SharedHandle<SocketCore>& s,
 			 Seq sequence,
 			 const SharedHandle<PeerConnection>& peerConnection =
 			 SharedHandle<PeerConnection>());
 
   virtual ~PeerInteractionCommand();
-
-  void setPeerStorage(const SharedHandle<PeerStorage>& peerStorage);
 };
 
 } // namespace aria2

src/PeerReceiveHandshakeCommand.cc

@@ -93,17 +93,24 @@ bool PeerReceiveHandshakeCommand::executeInternal()
     // check info_hash
     std::string infoHash = std::string(&data[28], &data[28+INFO_HASH_LENGTH]);
 
-    BtObject btObject = e->getBtRegistry()->get(infoHash);
-    SharedHandle<DownloadContext> downloadContext = btObject._downloadContext;
+    SharedHandle<DownloadContext> downloadContext =
+      e->getBtRegistry()->getDownloadContext(infoHash);
+    if(downloadContext.isNull()) {
+      throw DL_ABORT_EX
+	(StringFormat("Unknown info hash %s",
+		      util::toHex(infoHash).c_str()).str());
+    }
+    BtObject btObject = e->getBtRegistry()->get
+      (downloadContext->getOwnerRequestGroup()->getGID());
     SharedHandle<BtRuntime> btRuntime = btObject._btRuntime;
     SharedHandle<PieceStorage> pieceStorage = btObject._pieceStorage;
     SharedHandle<PeerStorage> peerStorage = btObject._peerStorage;
-
-    if(downloadContext.isNull() || !btRuntime->ready()) {
+    if(!btRuntime->ready()) {
       throw DL_ABORT_EX
 	(StringFormat("Unknown info hash %s",
 		      util::toHex(infoHash).c_str()).str());
     }
+
     TransferStat tstat =
       downloadContext->getOwnerRequestGroup()->calculateStat();
     const unsigned int maxDownloadLimit =
@@ -130,10 +137,10 @@ bool PeerReceiveHandshakeCommand::executeInternal()
 	   e,
 	   btRuntime,
 	   pieceStorage,
+	   peerStorage,
 	   socket,
 	   PeerInteractionCommand::RECEIVER_WAIT_HANDSHAKE,
 	   _peerConnection);
-	command->setPeerStorage(peerStorage);
 	e->commands.push_back(command);
 	logger->debug(MSG_INCOMING_PEER_CONNECTION, cuid, peer->usedBy());
       }

src/RequestGroup.cc

@@ -264,7 +264,7 @@ void RequestGroup::createInitialCommand
 	(_option->getAsInt(PREF_BT_TRACKER_INTERVAL));
       btAnnounce->shuffleAnnounce();
       
-      btRegistry->put(torrentAttrs[bittorrent::INFO_HASH].s(),
+      btRegistry->put(_gid,
 		      BtObject(_downloadContext,
 			       _pieceStorage,
 			       peerStorage,
@@ -774,22 +774,7 @@ void RequestGroup::releaseRuntimeResource(DownloadEngine* e)
 {
 #ifdef ENABLE_BITTORRENT
   if(_downloadContext->hasAttribute(bittorrent::BITTORRENT)) {
-    SharedHandle<BtRegistry> btRegistry = e->getBtRegistry();
-    const BDE& torrentAttrs =
-      _downloadContext->getAttribute(bittorrent::BITTORRENT);
-    const std::string& infoHash = torrentAttrs[bittorrent::INFO_HASH].s();
-    SharedHandle<DownloadContext> contextInReg =
-      btRegistry->getDownloadContext(infoHash);
-    // Make sure that the registered DownloadContext's GID is equal to
-    // _gid.  Even if createInitialCommand() throws exception without
-    // registering this DownloadContext, after that, this method is
-    // called. In this case, just finding DownloadContext using
-    // infoHash may detect another download's DownloadContext and
-    // deleting it from BtRegistry causes Segmentation Fault.
-    if(!contextInReg.isNull() &&
-       contextInReg->getOwnerRequestGroup()->getGID() == _gid) {
-      btRegistry->remove(infoHash);
-    }
+    e->getBtRegistry()->remove(_gid);
   }
 #endif // ENABLE_BITTORRENT
   if(!_pieceStorage.isNull()) {

src/XmlRpcMethodImpl.cc

@@ -290,17 +290,19 @@ static void gatherProgressCommon
 
 #ifdef ENABLE_BITTORRENT
 static void gatherProgressBitTorrent
-(BDE& entryDict, const BDE& torrentAttrs, const SharedHandle<BtRegistry>& btreg)
+(BDE& entryDict, const BDE& torrentAttrs, const BtObject& btObject)
 {
   const std::string& infoHash = torrentAttrs[bittorrent::INFO_HASH].s();
   entryDict["infoHash"] = util::toHex(infoHash);
 
-  SharedHandle<PeerStorage> peerStorage = btreg->get(infoHash)._peerStorage;
-  assert(!peerStorage.isNull());
+  if(!btObject.isNull()) {
+    SharedHandle<PeerStorage> peerStorage = btObject._peerStorage;
+    assert(!peerStorage.isNull());
 
-  std::deque<SharedHandle<Peer> > peers;
-  peerStorage->getActivePeers(peers);
-  entryDict["numSeeders"] = countSeeder(peers.begin(), peers.end());
+    std::deque<SharedHandle<Peer> > peers;
+    peerStorage->getActivePeers(peers);
+    entryDict["numSeeders"] = countSeeder(peers.begin(), peers.end());
+  }
 }
 
 static void gatherPeer(BDE& peers, const SharedHandle<PeerStorage>& ps)
@@ -335,8 +337,8 @@ static void gatherProgress
   if(group->getDownloadContext()->hasAttribute(bittorrent::BITTORRENT)) {
     const BDE& torrentAttrs =
       group->getDownloadContext()->getAttribute(bittorrent::BITTORRENT);
-    SharedHandle<BtRegistry> btreg = e->getBtRegistry();
-    gatherProgressBitTorrent(entryDict, torrentAttrs, btreg);
+    BtObject btObject = e->getBtRegistry()->get(group->getGID());
+    gatherProgressBitTorrent(entryDict, torrentAttrs, btObject);
   }
 #endif // ENABLE_BITTORRENT
 }
@@ -463,14 +465,11 @@ BDE GetPeersXmlRpcMethod::process
   }
   BDE peers = BDE::list();
   if(group->getDownloadContext()->hasAttribute(bittorrent::BITTORRENT)) {
-    SharedHandle<BtRegistry> btreg = e->getBtRegistry();
-    const BDE& torrentAttrs =
-      group->getDownloadContext()->getAttribute(bittorrent::BITTORRENT);
-    SharedHandle<PeerStorage> peerStorage =
-      btreg->get(torrentAttrs[bittorrent::INFO_HASH].s())._peerStorage;
-    assert(!peerStorage.isNull());
-    BDE entry = BDE::dict();
-    gatherPeer(peers, peerStorage);
+    BtObject btObject = e->getBtRegistry()->get(group->getGID());
+    if(!btObject.isNull()) {
+      assert(!btObject._peerStorage.isNull());
+      gatherPeer(peers, btObject._peerStorage);
+    }
   }
   return peers;
 }

test/BtRegistryTest.cc

@@ -10,6 +10,7 @@
 #include "MockBtProgressInfoFile.h"
 #include "BtRuntime.h"
 #include "FileEntry.h"
+#include "bittorrent_helper.h"
 
 namespace aria2 {
 
@@ -17,6 +18,7 @@ class BtRegistryTest:public CppUnit::TestFixture {
 
   CPPUNIT_TEST_SUITE(BtRegistryTest);
   CPPUNIT_TEST(testGetDownloadContext);
+  CPPUNIT_TEST(testGetDownloadContext_infoHash);
   CPPUNIT_TEST(testGetAllDownloadContext);
   CPPUNIT_TEST(testRemove);
   CPPUNIT_TEST(testRemoveAll);
@@ -25,6 +27,7 @@ private:
 
 public:
   void testGetDownloadContext();
+  void testGetDownloadContext_infoHash();
   void testGetAllDownloadContext();
   void testRemove();
   void testRemoveAll();
@@ -36,12 +39,12 @@ CPPUNIT_TEST_SUITE_REGISTRATION( BtRegistryTest );
 void BtRegistryTest::testGetDownloadContext()
 {
   BtRegistry btRegistry;
-  CPPUNIT_ASSERT(btRegistry.getDownloadContext("test").isNull());
+  CPPUNIT_ASSERT(btRegistry.getDownloadContext(1).isNull());
   SharedHandle<DownloadContext> dctx(new DownloadContext());
   BtObject btObject;
   btObject._downloadContext = dctx;
-  btRegistry.put("test", btObject);
-  CPPUNIT_ASSERT_EQUAL(dctx.get(), btRegistry.getDownloadContext("test").get());
+  btRegistry.put(1, btObject);
+  CPPUNIT_ASSERT_EQUAL(dctx.get(), btRegistry.getDownloadContext(1).get());
 }
 
 static void addTwoDownloadContext(BtRegistry& btRegistry)
@@ -52,8 +55,27 @@ static void addTwoDownloadContext(BtRegistry& btRegistry)
   btObject1._downloadContext = dctx1;
   BtObject btObject2;
   btObject2._downloadContext = dctx2;
-  btRegistry.put("ctx1", btObject1);
-  btRegistry.put("ctx2", btObject2);
+  btRegistry.put(1, btObject1);
+  btRegistry.put(2, btObject2);
+}
+
+void BtRegistryTest::testGetDownloadContext_infoHash()
+{
+  BtRegistry btRegistry;
+  addTwoDownloadContext(btRegistry);
+  BDE attrs1 = BDE::dict();
+  attrs1[bittorrent::INFO_HASH] = std::string("hash1");
+  BDE attrs2 = BDE::dict();
+  attrs2[bittorrent::INFO_HASH] = std::string("hash2");
+  btRegistry.getDownloadContext(1)->setAttribute
+    (bittorrent::BITTORRENT, attrs1);
+  btRegistry.getDownloadContext(2)->setAttribute
+    (bittorrent::BITTORRENT, attrs2);
+
+  CPPUNIT_ASSERT(!btRegistry.getDownloadContext("hash1").isNull());
+  CPPUNIT_ASSERT(btRegistry.getDownloadContext("hash1").get() ==
+		 btRegistry.getDownloadContext(1).get());
+  CPPUNIT_ASSERT(btRegistry.getDownloadContext("not exists").isNull());
 }
 
 void BtRegistryTest::testGetAllDownloadContext()
@@ -70,9 +92,9 @@ void BtRegistryTest::testRemove()
 {
   BtRegistry btRegistry;
   addTwoDownloadContext(btRegistry);
-  CPPUNIT_ASSERT(btRegistry.remove("ctx1"));
-  CPPUNIT_ASSERT(btRegistry.get("ctx1").isNull());
-  CPPUNIT_ASSERT(!btRegistry.get("ctx2").isNull());
+  CPPUNIT_ASSERT(btRegistry.remove(1));
+  CPPUNIT_ASSERT(btRegistry.get(1).isNull());
+  CPPUNIT_ASSERT(!btRegistry.get(2).isNull());
 }
 
 void BtRegistryTest::testRemoveAll()
@@ -80,8 +102,8 @@ void BtRegistryTest::testRemoveAll()
   BtRegistry btRegistry;
   addTwoDownloadContext(btRegistry);
   btRegistry.removeAll();
-  CPPUNIT_ASSERT(btRegistry.get("ctx1").isNull());
-  CPPUNIT_ASSERT(btRegistry.get("ctx2").isNull());
+  CPPUNIT_ASSERT(btRegistry.get(1).isNull());
+  CPPUNIT_ASSERT(btRegistry.get(2).isNull());
 }
 
 } // namespace aria2

test/XmlRpcMethodTest.cc

@@ -515,12 +515,21 @@ static void addUri(const std::string& uri,
   CPPUNIT_ASSERT_EQUAL(0, m.execute(req, e.get())._code);
 }
 
+static void addTorrent
+(const std::string& torrentFile, const SharedHandle<DownloadEngine>& e)
+{
+  AddTorrentXmlRpcMethod m;
+  XmlRpcRequest req("aria2.addTorrent", BDE::list());
+  req._params << BDE(readFile(torrentFile));
+  XmlRpcResponse res = m.execute(req, e.get());
+}
+
 void XmlRpcMethodTest::testTellWaiting()
 {
   addUri("http://1/", _e);
   addUri("http://2/", _e);
   addUri("http://3/", _e);
-  addUri("http://4/", _e);
+  addTorrent("single.torrent", _e);
 
   TellWaitingXmlRpcMethod m;
   XmlRpcRequest req("aria2.tellWaiting", BDE::list());