Fix use-after-free on exit with multi-file torrent download + DHT

DefaultPieceStorage may be referenced by one of DHT task (e.g.,
DHTPeerLookupTask), after RequestGroup was deleted, and even after
RequestGroupMan was deleted.  DefaultPieceStorage has a reference to
MultiDiskAdaptor which calls RequestGroupMan object on destruction.
So when DHT task is destroyed, DefaultPieceStorage is destroyed, which
in turn destroys MultiDiskAdaptor.  DHT task is destroyed after
RequestGroupMan was destroyed, MultiDiskAdaptor will use now freed
RequestGroupMan object, this is use-after-free.

src/DiskAdaptor.cc

@@ -34,12 +34,12 @@
 /* copyright --> */
 #include "DiskAdaptor.h"
 #include "FileEntry.h"
+#include "OpenedFileCounter.h"
 
 namespace aria2 {
 
 DiskAdaptor::DiskAdaptor()
-  : fileAllocationMethod_(FILE_ALLOC_ADAPTIVE),
-    requestGroupMan_(nullptr)
+  : fileAllocationMethod_(FILE_ALLOC_ADAPTIVE)
 {}
 
 DiskAdaptor::~DiskAdaptor() {}

src/DiskAdaptor.h

@@ -48,7 +48,7 @@ namespace aria2 {
 class FileEntry;
 class FileAllocationIterator;
 class WrDiskCacheEntry;
-class RequestGroupMan;
+class OpenedFileCounter;
 
 class DiskAdaptor:public BinaryStream {
 public:
@@ -125,21 +125,23 @@ public:
   // Returns the number of closed files.
   virtual size_t tryCloseFile(size_t numClose) { return 0; }
 
-  void setRequestGroupMan(RequestGroupMan* rgman)
+  void setOpenedFileCounter
+  (std::shared_ptr<OpenedFileCounter> openedFileCounter)
   {
-    requestGroupMan_ = rgman;
+    openedFileCounter_ = std::move(openedFileCounter);
   }
 
-  RequestGroupMan* getRequestGroupMan() const
+  const std::shared_ptr<OpenedFileCounter>& getOpenedFileCounter() const
   {
-    return requestGroupMan_;
+    return openedFileCounter_;
   }
+
 private:
   std::vector<std::shared_ptr<FileEntry> > fileEntries_;
 
   FileAllocationMethod fileAllocationMethod_;
 
-  RequestGroupMan* requestGroupMan_;
+  std::shared_ptr<OpenedFileCounter> openedFileCounter_;
 };
 
 } // namespace aria2

src/Makefile.am

@@ -264,7 +264,8 @@ SRCS =  \
 	WatchProcessCommand.cc WatchProcessCommand.h\
 	WrDiskCache.cc WrDiskCache.h\
 	WrDiskCacheEntry.cc WrDiskCacheEntry.h\
-	XmlRpcRequestParserController.cc XmlRpcRequestParserController.h
+	XmlRpcRequestParserController.cc XmlRpcRequestParserController.h\
+	OpenedFileCounter.cc OpenedFileCounter.h
 
 if ANDROID
 SRCS += android/android.c

src/MultiDiskAdaptor.cc

@@ -51,7 +51,7 @@
 #include "LogFactory.h"
 #include "SimpleRandomizer.h"
 #include "WrDiskCacheEntry.h"
-#include "RequestGroupMan.h"
+#include "OpenedFileCounter.h"
 
 namespace aria2 {
 
@@ -226,8 +226,9 @@ void MultiDiskAdaptor::openIfNot
   if(!entry->isOpen()) {
         // A2_LOG_NOTICE(fmt("DiskWriterEntry: Cache MISS. offset=%s",
         //        util::itos(entry->getFileEntry()->getOffset()).c_str()));
-    if(getRequestGroupMan()) {
-      getRequestGroupMan()->ensureMaxOpenFileLimit(1);
+    auto& openedFileCounter = getOpenedFileCounter();
+    if(openedFileCounter) {
+      openedFileCounter->ensureMaxOpenFileLimit(1);
     }
     (entry->*open)();
     openedDiskWriterEntries_.push_back(entry);
@@ -278,8 +279,9 @@ void MultiDiskAdaptor::closeFile()
       dwent->closeFile();
     }
   }
-  if(getRequestGroupMan()) {
-    getRequestGroupMan()->reduceNumOfOpenedFile(n);
+  auto& openedFileCounter = getOpenedFileCounter();
+  if(openedFileCounter) {
+    openedFileCounter->reduceNumOfOpenedFile(n);
   }
 }
 

src/OpenedFileCounter.cc

@@ -0,0 +1,100 @@
+/* <!-- copyright */
+/*
+ * aria2 - The high speed download utility
+ *
+ * Copyright (C) 2014 Tatsuhiro Tsujikawa
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * In addition, as a special exception, the copyright holders give
+ * permission to link the code of portions of this program with the
+ * OpenSSL library under certain conditions as described in each
+ * individual source file, and distribute linked combinations
+ * including the two.
+ * You must obey the GNU General Public License in all respects
+ * for all of the code used other than OpenSSL.  If you modify
+ * file(s) with this exception, you may extend this exception to your
+ * version of the file(s), but you are not obligated to do so.  If you
+ * do not wish to do so, delete this exception statement from your
+ * version.  If you delete this exception statement from all source
+ * files in the program, then also delete it here.
+ */
+/* copyright --> */
+#include "OpenedFileCounter.h"
+
+#include <cassert>
+
+#include "RequestGroupMan.h"
+#include "RequestGroup.h"
+#include "PieceStorage.h"
+#include "DiskAdaptor.h"
+#include "SimpleRandomizer.h"
+
+namespace aria2 {
+
+OpenedFileCounter::OpenedFileCounter(RequestGroupMan* rgman,
+                                     size_t maxOpenFiles)
+  : rgman_(rgman),
+    maxOpenFiles_(maxOpenFiles),
+    numOpenFiles_(0)
+{}
+
+void OpenedFileCounter::ensureMaxOpenFileLimit(size_t numNewFiles)
+{
+  if(!rgman_) {
+    return;
+  }
+
+  if(numOpenFiles_ + numNewFiles <= maxOpenFiles_) {
+    numOpenFiles_ += numNewFiles;
+    return;
+  }
+  assert(numNewFiles <= maxOpenFiles_);
+  size_t numClose = numOpenFiles_ + numNewFiles - maxOpenFiles_;
+  size_t left = numClose;
+
+  auto requestGroups = rgman_->getRequestGroups();
+
+  auto mark = std::begin(requestGroups);
+  std::advance(mark,
+               SimpleRandomizer::getInstance()->
+               getRandomNumber(requestGroups.size()));
+  for(auto i = mark; i != std::end(requestGroups) && left > 0; ++i) {
+    left -= (*i)->getPieceStorage()->getDiskAdaptor()->tryCloseFile(left);
+  }
+  for(auto i = std::begin(requestGroups); i != mark && left > 0; ++i) {
+    left -= (*i)->getPieceStorage()->getDiskAdaptor()->tryCloseFile(left);
+  }
+  assert(left == 0);
+  numOpenFiles_ += numNewFiles - numClose;
+}
+
+void OpenedFileCounter::reduceNumOfOpenedFile(size_t numCloseFiles)
+{
+  if(!rgman_) {
+    return;
+  }
+
+  assert(numOpenFiles_ >= numCloseFiles);
+  numOpenFiles_ -= numCloseFiles;
+}
+
+void OpenedFileCounter::deactivate()
+{
+  rgman_ = nullptr;
+}
+
+} // namespace aria2
+

src/OpenedFileCounter.h

@@ -0,0 +1,76 @@
+/* <!-- copyright */
+/*
+ * aria2 - The high speed download utility
+ *
+ * Copyright (C) 2014 Tatsuhiro Tsujikawa
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * In addition, as a special exception, the copyright holders give
+ * permission to link the code of portions of this program with the
+ * OpenSSL library under certain conditions as described in each
+ * individual source file, and distribute linked combinations
+ * including the two.
+ * You must obey the GNU General Public License in all respects
+ * for all of the code used other than OpenSSL.  If you modify
+ * file(s) with this exception, you may extend this exception to your
+ * version of the file(s), but you are not obligated to do so.  If you
+ * do not wish to do so, delete this exception statement from your
+ * version.  If you delete this exception statement from all source
+ * files in the program, then also delete it here.
+ */
+/* copyright --> */
+#ifndef D_OPENED_FILE_COUNTER_H
+#define D_OPENED_FILE_COUNTER_H
+
+#include "common.h"
+
+namespace aria2 {
+
+class RequestGroupMan;
+
+class OpenedFileCounter {
+public:
+  OpenedFileCounter(RequestGroupMan* rgman, size_t maxOpenFiles);
+
+  // Keeps the number of open files under the global limit specified
+  // in the option.  The caller requests that |numNewFiles| files are
+  // going to be opened.  This function requires that |numNewFiles| is
+  // less than or equal to the limit.
+  //
+  // Currently the only download using MultiDiskAdaptor is affected by
+  // the global limit.
+  void ensureMaxOpenFileLimit(size_t numNewFiles);
+
+  // Reduces the number of open files managed by this object.
+  void reduceNumOfOpenedFile(size_t numCloseFiles);
+
+  void setMaxOpenFiles(size_t maxOpenFiles)
+  {
+    maxOpenFiles_ = maxOpenFiles;
+  }
+
+  // Deactivates this object.
+  void deactivate();
+
+private:
+  RequestGroupMan* rgman_;
+  size_t maxOpenFiles_;
+  size_t numOpenFiles_;
+};
+
+} // namespace aria2
+
+#endif // D_OPENED_FILE_COUNTER_H

src/RequestGroup.cc

@@ -593,7 +593,10 @@ void RequestGroup::initPieceStorage()
     tempPieceStorage = ps;
   }
   tempPieceStorage->initStorage();
-  tempPieceStorage->getDiskAdaptor()->setRequestGroupMan(requestGroupMan_);
+  if(requestGroupMan_) {
+    tempPieceStorage->getDiskAdaptor()->setOpenedFileCounter
+      (requestGroupMan_->getOpenedFileCounter());
+  }
   segmentMan_ = std::make_shared<SegmentMan>(downloadContext_, tempPieceStorage);
   pieceStorage_ = tempPieceStorage;
 }

src/RequestGroupMan.cc

@@ -82,6 +82,7 @@
 #include "DiskAdaptor.h"
 #include "SimpleRandomizer.h"
 #include "array_fun.h"
+#include "OpenedFileCounter.h"
 #ifdef ENABLE_BITTORRENT
 #  include "bittorrent_helper.h"
 #endif // ENABLE_BITTORRENT
@@ -116,7 +117,8 @@ RequestGroupMan::RequestGroupMan
     removedLastErrorResult_(error_code::FINISHED),
     maxDownloadResult_(option->getAsInt(PREF_MAX_DOWNLOAD_RESULT)),
     wrDiskCache_(nullptr),
-    numOpenFile_(0),
+    openedFileCounter_(std::make_shared<OpenedFileCounter>
+                       (this, option->getAsInt(PREF_BT_MAX_OPEN_FILES))),
     numStoppedTotal_(0)
 {
   appendReservedGroup(reservedGroups_,
@@ -125,6 +127,7 @@ RequestGroupMan::RequestGroupMan
 
 RequestGroupMan::~RequestGroupMan()
 {
+  openedFileCounter_->deactivate();
   delete wrDiskCache_;
 }
 
@@ -964,34 +967,4 @@ void RequestGroupMan::initWrDiskCache()
   }
 }
 
-void RequestGroupMan::ensureMaxOpenFileLimit(size_t numNewFile)
-{
-  size_t max = option_->getAsInt(PREF_BT_MAX_OPEN_FILES);
-  if(numOpenFile_ + numNewFile <= max) {
-    numOpenFile_ += numNewFile;
-    return;
-  }
-  assert(numNewFile <= max);
-  size_t numClose = numOpenFile_ + numNewFile - max;
-  size_t left = numClose;
-  auto mark = std::begin(requestGroups_);
-  std::advance(mark,
-               SimpleRandomizer::getInstance()->
-               getRandomNumber(requestGroups_.size()));
-  for(auto i = mark; i != std::end(requestGroups_) && left > 0; ++i) {
-    left -= (*i)->getPieceStorage()->getDiskAdaptor()->tryCloseFile(left);
-  }
-  for(auto i = std::begin(requestGroups_); i != mark && left > 0; ++i) {
-    left -= (*i)->getPieceStorage()->getDiskAdaptor()->tryCloseFile(left);
-  }
-  assert(left == 0);
-  numOpenFile_ += numNewFile - numClose;
-}
-
-void RequestGroupMan::reduceNumOfOpenedFile(size_t numCloseFile)
-{
-  assert(numOpenFile_ >= numCloseFile);
-  numOpenFile_ -= numCloseFile;
-}
-
 } // namespace aria2

src/RequestGroupMan.h

@@ -60,6 +60,7 @@ class Option;
 class OutputFile;
 class UriListParser;
 class WrDiskCache;
+class OpenedFileCounter;
 
 typedef IndexedList<a2_gid_t,
                     std::shared_ptr<RequestGroup> > RequestGroupList;
@@ -104,7 +105,7 @@ private:
 
   WrDiskCache* wrDiskCache_;
 
-  size_t numOpenFile_;
+  std::shared_ptr<OpenedFileCounter> openedFileCounter_;
 
   // The number of stopped downloads so far in total, including
   // evicted DownloadResults.
@@ -356,17 +357,6 @@ public:
     return keepRunning_;
   }
 
-  // Keeps the number of open files under the global limit specified
-  // in the option. The caller requests that |numNewFile| files are
-  // going to be opened. This function requires that |numNewFile| is
-  // less than or equal to the limit.
-  //
-  // Currently the only download using MultiDiskAdaptor is affected by
-  // the global limit.
-  void ensureMaxOpenFileLimit(size_t numNewFile);
-  // Reduces the number of open files managed by this object.
-  void reduceNumOfOpenedFile(size_t numCloseFile);
-
   size_t getNumStoppedTotal() const
   {
     return numStoppedTotal_;
@@ -381,6 +371,11 @@ public:
   {
     return lastSessionHash_;
   }
+
+  const std::shared_ptr<OpenedFileCounter>& getOpenedFileCounter() const
+  {
+    return openedFileCounter_;
+  }
 };
 
 } // namespace aria2

src/RpcMethodImpl.cc

@@ -68,6 +68,7 @@
 #include "SessionSerializer.h"
 #include "MessageDigest.h"
 #include "message_digest_helper.h"
+#include "OpenedFileCounter.h"
 #ifdef ENABLE_BITTORRENT
 # include "bittorrent_helper.h"
 # include "BtRegistry.h"
@@ -1567,6 +1568,11 @@ void changeGlobalOption(const Option& option, DownloadEngine* e)
       // TODO no exception handling
     }
   }
+  if(option.defined(PREF_BT_MAX_OPEN_FILES)) {
+    auto& openedFileCounter = e->getRequestGroupMan()->getOpenedFileCounter();
+    openedFileCounter->setMaxOpenFiles
+      (option.getAsInt(PREF_BT_MAX_OPEN_FILES));
+  }
 }
 
 } // namespace aria2