From 570bc24fb98d72d1bce9261fdc29139224ec86ec Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 8 Jun 2014 17:03:34 +0900 Subject: [PATCH] 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 | 4 +- src/DiskAdaptor.h | 14 +++--- src/Makefile.am | 3 +- src/MultiDiskAdaptor.cc | 12 +++-- src/OpenedFileCounter.cc | 100 +++++++++++++++++++++++++++++++++++++++ src/OpenedFileCounter.h | 76 +++++++++++++++++++++++++++++ src/RequestGroup.cc | 5 +- src/RequestGroupMan.cc | 35 ++------------ src/RequestGroupMan.h | 19 +++----- src/RpcMethodImpl.cc | 6 +++ 10 files changed, 216 insertions(+), 58 deletions(-) create mode 100644 src/OpenedFileCounter.cc create mode 100644 src/OpenedFileCounter.h diff --git a/src/DiskAdaptor.cc b/src/DiskAdaptor.cc index 641c94de..a307c377 100644 --- a/src/DiskAdaptor.cc +++ b/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() {} diff --git a/src/DiskAdaptor.h b/src/DiskAdaptor.h index 114f904c..bb070707 100644 --- a/src/DiskAdaptor.h +++ b/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) { - requestGroupMan_ = rgman; + openedFileCounter_ = std::move(openedFileCounter); } - RequestGroupMan* getRequestGroupMan() const + const std::shared_ptr& getOpenedFileCounter() const { - return requestGroupMan_; + return openedFileCounter_; } + private: std::vector > fileEntries_; FileAllocationMethod fileAllocationMethod_; - RequestGroupMan* requestGroupMan_; + std::shared_ptr openedFileCounter_; }; } // namespace aria2 diff --git a/src/Makefile.am b/src/Makefile.am index 89f4b8ae..fb201e33 100644 --- a/src/Makefile.am +++ b/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 diff --git a/src/MultiDiskAdaptor.cc b/src/MultiDiskAdaptor.cc index 825b04cc..4a2b0ec4 100644 --- a/src/MultiDiskAdaptor.cc +++ b/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); } } diff --git a/src/OpenedFileCounter.cc b/src/OpenedFileCounter.cc new file mode 100644 index 00000000..c5bdeade --- /dev/null +++ b/src/OpenedFileCounter.cc @@ -0,0 +1,100 @@ +/* */ +#include "OpenedFileCounter.h" + +#include + +#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 + diff --git a/src/OpenedFileCounter.h b/src/OpenedFileCounter.h new file mode 100644 index 00000000..960292b1 --- /dev/null +++ b/src/OpenedFileCounter.h @@ -0,0 +1,76 @@ +/* */ +#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 diff --git a/src/RequestGroup.cc b/src/RequestGroup.cc index 066e0f21..8a111ddf 100644 --- a/src/RequestGroup.cc +++ b/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(downloadContext_, tempPieceStorage); pieceStorage_ = tempPieceStorage; } diff --git a/src/RequestGroupMan.cc b/src/RequestGroupMan.cc index 92c3b8c6..938f83e5 100644 --- a/src/RequestGroupMan.cc +++ b/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 + (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 diff --git a/src/RequestGroupMan.h b/src/RequestGroupMan.h index 773184e8..ecf9675d 100644 --- a/src/RequestGroupMan.h +++ b/src/RequestGroupMan.h @@ -60,6 +60,7 @@ class Option; class OutputFile; class UriListParser; class WrDiskCache; +class OpenedFileCounter; typedef IndexedList > RequestGroupList; @@ -104,7 +105,7 @@ private: WrDiskCache* wrDiskCache_; - size_t numOpenFile_; + std::shared_ptr 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& getOpenedFileCounter() const + { + return openedFileCounter_; + } }; } // namespace aria2 diff --git a/src/RpcMethodImpl.cc b/src/RpcMethodImpl.cc index d1a7df29..fd4c3d49 100644 --- a/src/RpcMethodImpl.cc +++ b/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