From 76a9ad9c84cd07b3264ad347d33630056ddf6aef Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 26 Aug 2008 12:39:07 +0000 Subject: [PATCH] 2008-08-26 Tatsuhiro Tsujikawa Fixed chunk checksum validation cannot detect trailing garbage data. BUG#2074141 * src/AbstractSingleDiskAdaptor.cc * src/AbstractSingleDiskAdaptor.h * src/CheckIntegrityEntry.cc * src/CheckIntegrityEntry.h * src/DiskAdaptor.h * src/MultiDiskAdaptor.cc * src/MultiDiskAdaptor.h * src/RequestGroup.cc * test/DirectDiskAdaptorTest.cc * test/MultiDiskAdaptorTest.cc * test/TestUtil.cc * test/TestUtil.h --- ChangeLog | 17 ++++++++++ src/AbstractSingleDiskAdaptor.cc | 7 ++++ src/AbstractSingleDiskAdaptor.h | 2 ++ src/CheckIntegrityEntry.cc | 8 +++++ src/CheckIntegrityEntry.h | 2 ++ src/DiskAdaptor.h | 7 ++++ src/MultiDiskAdaptor.cc | 13 ++++++++ src/MultiDiskAdaptor.h | 2 ++ src/RequestGroup.cc | 1 + test/DirectDiskAdaptorTest.cc | 52 ++++++++++++++++++++++++++++++ test/Makefile.am | 4 ++- test/Makefile.in | 55 ++++++++++++++++++-------------- test/MultiDiskAdaptorTest.cc | 38 ++++++++++++++++++++++ test/TestUtil.cc | 15 +++++++++ test/TestUtil.h | 8 +++++ 15 files changed, 206 insertions(+), 25 deletions(-) create mode 100644 test/DirectDiskAdaptorTest.cc create mode 100644 test/TestUtil.cc create mode 100644 test/TestUtil.h diff --git a/ChangeLog b/ChangeLog index bfffa54f..4d5bbcfd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,20 @@ +2008-08-26 Tatsuhiro Tsujikawa + + Fixed chunk checksum validation cannot detect trailing garbage data. + BUG#2074141 + * src/AbstractSingleDiskAdaptor.cc + * src/AbstractSingleDiskAdaptor.h + * src/CheckIntegrityEntry.cc + * src/CheckIntegrityEntry.h + * src/DiskAdaptor.h + * src/MultiDiskAdaptor.cc + * src/MultiDiskAdaptor.h + * src/RequestGroup.cc + * test/DirectDiskAdaptorTest.cc + * test/MultiDiskAdaptorTest.cc + * test/TestUtil.cc + * test/TestUtil.h + 2008-08-25 Tatsuhiro Tsujikawa Bump up version number of dht.dat file to 3. In version 3 format, time diff --git a/src/AbstractSingleDiskAdaptor.cc b/src/AbstractSingleDiskAdaptor.cc index 81fcdf4c..0114a646 100644 --- a/src/AbstractSingleDiskAdaptor.cc +++ b/src/AbstractSingleDiskAdaptor.cc @@ -110,6 +110,13 @@ bool AbstractSingleDiskAdaptor::directIOAllowed() const { return diskWriter->directIOAllowed(); } + +void AbstractSingleDiskAdaptor::cutTrailingGarbage() +{ + if(File(getFilePath()).size() > totalLength) { + diskWriter->truncate(totalLength); + } +} void AbstractSingleDiskAdaptor::setDiskWriter(const DiskWriterHandle& diskWriter) { diff --git a/src/AbstractSingleDiskAdaptor.h b/src/AbstractSingleDiskAdaptor.h index ac3668b4..aa776a28 100644 --- a/src/AbstractSingleDiskAdaptor.h +++ b/src/AbstractSingleDiskAdaptor.h @@ -78,6 +78,8 @@ public: virtual bool directIOAllowed() const; + virtual void cutTrailingGarbage(); + void setDiskWriter(const SharedHandle& diskWriter); SharedHandle getDiskWriter() const; diff --git a/src/CheckIntegrityEntry.cc b/src/CheckIntegrityEntry.cc index e961d9b7..5bfec8c5 100644 --- a/src/CheckIntegrityEntry.cc +++ b/src/CheckIntegrityEntry.cc @@ -34,6 +34,9 @@ /* copyright --> */ #include "CheckIntegrityEntry.h" #include "IteratableValidator.h" +#include "RequestGroup.h" +#include "PieceStorage.h" +#include "DiskAdaptor.h" namespace aria2 { @@ -72,4 +75,9 @@ bool CheckIntegrityEntry::finished() return _validator->finished(); } +void CheckIntegrityEntry::cutTrailingGarbage() +{ + _requestGroup->getPieceStorage()->getDiskAdaptor()->cutTrailingGarbage(); +} + } // namespace aria2 diff --git a/src/CheckIntegrityEntry.h b/src/CheckIntegrityEntry.h index c8042e0b..300c7536 100644 --- a/src/CheckIntegrityEntry.h +++ b/src/CheckIntegrityEntry.h @@ -70,6 +70,8 @@ public: virtual void onDownloadIncomplete(std::deque& commands, DownloadEngine* e) = 0; + + void cutTrailingGarbage(); }; typedef SharedHandle CheckIntegrityEntryHandle; diff --git a/src/DiskAdaptor.h b/src/DiskAdaptor.h index df4acf1e..ddf562b7 100644 --- a/src/DiskAdaptor.h +++ b/src/DiskAdaptor.h @@ -96,6 +96,13 @@ public: virtual void enableDirectIO() {} virtual void disableDirectIO() {} + + // Assumed each file length is stored in fileEntries or DiskAdaptor knows it. + // If each actual file's length is larger than that, truncate file to that + // length. + // Call one of openFile/openExistingFile/initAndOpenFile before calling this + // function. + virtual void cutTrailingGarbage() = 0; }; typedef SharedHandle DiskAdaptorHandle; diff --git a/src/MultiDiskAdaptor.cc b/src/MultiDiskAdaptor.cc index cf74b4b9..9ce2ecb9 100644 --- a/src/MultiDiskAdaptor.cc +++ b/src/MultiDiskAdaptor.cc @@ -442,6 +442,19 @@ void MultiDiskAdaptor::disableDirectIO() } } +void MultiDiskAdaptor::cutTrailingGarbage() +{ + for(std::deque >::const_iterator i = + diskWriterEntries.begin(); i != diskWriterEntries.end(); ++i) { + uint64_t length = (*i)->getFileEntry()->getLength(); + if(File((*i)->getFilePath(_cachedTopDirPath)).size() > length) { + // We need open file before calling DiskWriter::truncate(uint64_t) + openIfNot(*i, &DiskWriterEntry::openFile, _cachedTopDirPath); + (*i)->getDiskWriter()->truncate(length); + } + } +} + void MultiDiskAdaptor::setMaxOpenFiles(size_t maxOpenFiles) { _maxOpenFiles = maxOpenFiles; diff --git a/src/MultiDiskAdaptor.h b/src/MultiDiskAdaptor.h index 5a8b2484..cabd1f20 100644 --- a/src/MultiDiskAdaptor.h +++ b/src/MultiDiskAdaptor.h @@ -181,6 +181,8 @@ public: _directIOAllowed = b; } + virtual void cutTrailingGarbage(); + void setMaxOpenFiles(size_t maxOpenFiles); }; diff --git a/src/RequestGroup.cc b/src/RequestGroup.cc index ddde492d..263a55e6 100644 --- a/src/RequestGroup.cc +++ b/src/RequestGroup.cc @@ -318,6 +318,7 @@ void RequestGroup::processCheckIntegrityEntry(std::deque& commands, if(e->option->getAsBool(PREF_CHECK_INTEGRITY) && entry->isValidationReady()) { entry->initValidator(); + entry->cutTrailingGarbage(); CheckIntegrityCommand* command = new CheckIntegrityCommand(CUIDCounterSingletonHolder::instance()->newID(), this, e, entry); commands.push_back(command); diff --git a/test/DirectDiskAdaptorTest.cc b/test/DirectDiskAdaptorTest.cc new file mode 100644 index 00000000..b5254008 --- /dev/null +++ b/test/DirectDiskAdaptorTest.cc @@ -0,0 +1,52 @@ +#include "DirectDiskAdaptor.h" +#include "FileEntry.h" +#include "DefaultDiskWriter.h" +#include "Exception.h" +#include "Util.h" +#include "TestUtil.h" +#include +#include + +namespace aria2 { + +class DirectDiskAdaptorTest:public CppUnit::TestFixture { + + CPPUNIT_TEST_SUITE(DirectDiskAdaptorTest); + CPPUNIT_TEST(testCutTrailingGarbage); + CPPUNIT_TEST_SUITE_END(); +public: + void setUp() {} + + void tearDown() {} + + void testCutTrailingGarbage(); +}; + + +CPPUNIT_TEST_SUITE_REGISTRATION(DirectDiskAdaptorTest); + +void DirectDiskAdaptorTest::testCutTrailingGarbage() +{ + std::string dir = "/tmp"; + SharedHandle entry + (new FileEntry("aria2_DirectDiskAdaptorTest_testCutTrailingGarbage", + 256, 0)); + createFile(dir+"/"+entry->getPath(), entry->getLength()+100); + + std::deque > fileEntries; + fileEntries.push_back(entry); + + DirectDiskAdaptor adaptor; + adaptor.setDiskWriter(SharedHandle(new DefaultDiskWriter())); + adaptor.setTotalLength(entry->getLength()); + adaptor.setStoreDir(dir); + adaptor.setFileEntries(fileEntries); + adaptor.openFile(); + + adaptor.cutTrailingGarbage(); + + CPPUNIT_ASSERT_EQUAL((uint64_t)entry->getLength(), + File(dir+"/"+entry->getPath()).size()); +} + +} // namespace aria2 diff --git a/test/Makefile.am b/test/Makefile.am index d67c0edf..64b52235 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -1,6 +1,7 @@ TESTS = aria2c check_PROGRAMS = $(TESTS) aria2c_SOURCES = AllTest.cc\ + TestUtil.cc TestUtil.h\ SocketCoreTest.cc\ array_funTest.cc\ HelpItemTest.cc\ @@ -58,7 +59,8 @@ aria2c_SOURCES = AllTest.cc\ ServerStatURISelectorTest.cc\ InOrderURISelectorTest.cc\ ServerStatTest.cc\ - NsCookieParserTest.cc + NsCookieParserTest.cc\ + DirectDiskAdaptorTest.cc DirectDiskAdaptorTest.h if HAVE_LIBZ aria2c_SOURCES += GZipDecoderTest.cc diff --git a/test/Makefile.in b/test/Makefile.in index 70234821..98fe198a 100644 --- a/test/Makefile.in +++ b/test/Makefile.in @@ -168,11 +168,11 @@ mkinstalldirs = $(SHELL) $(top_srcdir)/mkinstalldirs CONFIG_HEADER = $(top_builddir)/config.h CONFIG_CLEAN_FILES = am__EXEEXT_1 = aria2c$(EXEEXT) -am__aria2c_SOURCES_DIST = AllTest.cc SocketCoreTest.cc \ - array_funTest.cc HelpItemTest.cc TaggedItemTest.cc \ - TagContainerTest.cc Base64Test.cc SequenceTest.cc \ - a2functionalTest.cc FileEntryTest.cc PieceTest.cc \ - SegmentTest.cc GrowSegmentTest.cc \ +am__aria2c_SOURCES_DIST = AllTest.cc TestUtil.cc TestUtil.h \ + SocketCoreTest.cc array_funTest.cc HelpItemTest.cc \ + TaggedItemTest.cc TagContainerTest.cc Base64Test.cc \ + SequenceTest.cc a2functionalTest.cc FileEntryTest.cc \ + PieceTest.cc SegmentTest.cc GrowSegmentTest.cc \ SingleFileAllocationIteratorTest.cc \ DefaultBtProgressInfoFileTest.cc \ SingleFileDownloadContextTest.cc RequestGroupTest.cc \ @@ -192,8 +192,10 @@ am__aria2c_SOURCES_DIST = AllTest.cc SocketCoreTest.cc \ DownloadHandlerFactoryTest.cc ChunkedDecoderTest.cc \ SignatureTest.cc ServerStatManTest.cc \ ServerStatURISelectorTest.cc InOrderURISelectorTest.cc \ - ServerStatTest.cc NsCookieParserTest.cc GZipDecoderTest.cc \ - Sqlite3MozCookieParserTest.cc MessageDigestHelperTest.cc \ + ServerStatTest.cc NsCookieParserTest.cc \ + DirectDiskAdaptorTest.cc DirectDiskAdaptorTest.h \ + GZipDecoderTest.cc Sqlite3MozCookieParserTest.cc \ + MessageDigestHelperTest.cc \ IteratableChunkChecksumValidatorTest.cc \ IteratableChecksumValidatorTest.cc BtAllowedFastMessageTest.cc \ BtBitfieldMessageTest.cc BtCancelMessageTest.cc \ @@ -330,13 +332,13 @@ am__aria2c_SOURCES_DIST = AllTest.cc SocketCoreTest.cc \ @ENABLE_METALINK_TRUE@ MetalinkHelperTest.$(OBJEXT) \ @ENABLE_METALINK_TRUE@ MetalinkParserControllerTest.$(OBJEXT) \ @ENABLE_METALINK_TRUE@ MetalinkProcessorTest.$(OBJEXT) -am_aria2c_OBJECTS = AllTest.$(OBJEXT) SocketCoreTest.$(OBJEXT) \ - array_funTest.$(OBJEXT) HelpItemTest.$(OBJEXT) \ - TaggedItemTest.$(OBJEXT) TagContainerTest.$(OBJEXT) \ - Base64Test.$(OBJEXT) SequenceTest.$(OBJEXT) \ - a2functionalTest.$(OBJEXT) FileEntryTest.$(OBJEXT) \ - PieceTest.$(OBJEXT) SegmentTest.$(OBJEXT) \ - GrowSegmentTest.$(OBJEXT) \ +am_aria2c_OBJECTS = AllTest.$(OBJEXT) TestUtil.$(OBJEXT) \ + SocketCoreTest.$(OBJEXT) array_funTest.$(OBJEXT) \ + HelpItemTest.$(OBJEXT) TaggedItemTest.$(OBJEXT) \ + TagContainerTest.$(OBJEXT) Base64Test.$(OBJEXT) \ + SequenceTest.$(OBJEXT) a2functionalTest.$(OBJEXT) \ + FileEntryTest.$(OBJEXT) PieceTest.$(OBJEXT) \ + SegmentTest.$(OBJEXT) GrowSegmentTest.$(OBJEXT) \ SingleFileAllocationIteratorTest.$(OBJEXT) \ DefaultBtProgressInfoFileTest.$(OBJEXT) \ SingleFileDownloadContextTest.$(OBJEXT) \ @@ -363,8 +365,9 @@ am_aria2c_OBJECTS = AllTest.$(OBJEXT) SocketCoreTest.$(OBJEXT) \ ServerStatManTest.$(OBJEXT) \ ServerStatURISelectorTest.$(OBJEXT) \ InOrderURISelectorTest.$(OBJEXT) ServerStatTest.$(OBJEXT) \ - NsCookieParserTest.$(OBJEXT) $(am__objects_1) $(am__objects_2) \ - $(am__objects_3) $(am__objects_4) $(am__objects_5) + NsCookieParserTest.$(OBJEXT) DirectDiskAdaptorTest.$(OBJEXT) \ + $(am__objects_1) $(am__objects_2) $(am__objects_3) \ + $(am__objects_4) $(am__objects_5) aria2c_OBJECTS = $(am_aria2c_OBJECTS) am__DEPENDENCIES_1 = aria2c_DEPENDENCIES = ../src/libaria2c.a $(am__DEPENDENCIES_1) @@ -560,11 +563,12 @@ target_os = @target_os@ target_vendor = @target_vendor@ top_builddir = @top_builddir@ top_srcdir = @top_srcdir@ -aria2c_SOURCES = AllTest.cc SocketCoreTest.cc array_funTest.cc \ - HelpItemTest.cc TaggedItemTest.cc TagContainerTest.cc \ - Base64Test.cc SequenceTest.cc a2functionalTest.cc \ - FileEntryTest.cc PieceTest.cc SegmentTest.cc \ - GrowSegmentTest.cc SingleFileAllocationIteratorTest.cc \ +aria2c_SOURCES = AllTest.cc TestUtil.cc TestUtil.h SocketCoreTest.cc \ + array_funTest.cc HelpItemTest.cc TaggedItemTest.cc \ + TagContainerTest.cc Base64Test.cc SequenceTest.cc \ + a2functionalTest.cc FileEntryTest.cc PieceTest.cc \ + SegmentTest.cc GrowSegmentTest.cc \ + SingleFileAllocationIteratorTest.cc \ DefaultBtProgressInfoFileTest.cc \ SingleFileDownloadContextTest.cc RequestGroupTest.cc \ PStringBuildVisitorTest.cc ParameterizedStringParserTest.cc \ @@ -583,9 +587,10 @@ aria2c_SOURCES = AllTest.cc SocketCoreTest.cc array_funTest.cc \ DownloadHandlerFactoryTest.cc ChunkedDecoderTest.cc \ SignatureTest.cc ServerStatManTest.cc \ ServerStatURISelectorTest.cc InOrderURISelectorTest.cc \ - ServerStatTest.cc NsCookieParserTest.cc $(am__append_1) \ - $(am__append_2) $(am__append_3) $(am__append_4) \ - $(am__append_5) + ServerStatTest.cc NsCookieParserTest.cc \ + DirectDiskAdaptorTest.cc DirectDiskAdaptorTest.h \ + $(am__append_1) $(am__append_2) $(am__append_3) \ + $(am__append_4) $(am__append_5) #aria2c_CXXFLAGS = ${CPPUNIT_CFLAGS} -I../src -I../lib -Wall -D_FILE_OFFSET_BITS=64 #aria2c_LDFLAGS = ${CPPUNIT_LIBS} @@ -745,6 +750,7 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DefaultPeerStorageTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DefaultPieceStorageTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DictionaryTest.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DirectDiskAdaptorTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/DownloadHandlerFactoryTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ExceptionTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/FeatureConfigTest.Po@am__quote@ @@ -808,6 +814,7 @@ distclean-compile: @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/StringFormatTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/TagContainerTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/TaggedItemTest.Po@am__quote@ +@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/TestUtil.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/TimeSeedCriteriaTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/UTPexExtensionMessageTest.Po@am__quote@ @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/UriListParserTest.Po@am__quote@ diff --git a/test/MultiDiskAdaptorTest.cc b/test/MultiDiskAdaptorTest.cc index 2c517670..ca5960f6 100644 --- a/test/MultiDiskAdaptorTest.cc +++ b/test/MultiDiskAdaptorTest.cc @@ -1,6 +1,9 @@ #include "MultiDiskAdaptor.h" #include "FileEntry.h" #include "Exception.h" +#include "a2io.h" +#include "array_fun.h" +#include "TestUtil.h" #include #include #include @@ -13,6 +16,7 @@ class MultiDiskAdaptorTest:public CppUnit::TestFixture { CPPUNIT_TEST_SUITE(MultiDiskAdaptorTest); CPPUNIT_TEST(testWriteData); CPPUNIT_TEST(testReadData); + CPPUNIT_TEST(testCutTrailingGarbage); CPPUNIT_TEST_SUITE_END(); private: SharedHandle adaptor; @@ -26,6 +30,7 @@ public: void testWriteData(); void testReadData(); + void testCutTrailingGarbage(); }; @@ -129,4 +134,37 @@ void MultiDiskAdaptorTest::testReadData() { CPPUNIT_ASSERT_EQUAL(std::string("1234567890ABCDEFGHIJKLMNO"), std::string((char*)buf)); } +void MultiDiskAdaptorTest::testCutTrailingGarbage() +{ + std::string dir = "/tmp"; + std::string topDir = "."; + std::string topDirPath = dir+"/"+topDir; + std::string prefix = "aria2_MultiDiskAdaptorTest_testCutTrailingGarbage_"; + SharedHandle entries[] = { + SharedHandle(new FileEntry(prefix+"1", 256, 0)), + SharedHandle(new FileEntry(prefix+"2", 512, 256)) + }; + for(size_t i = 0; i < arrayLength(entries); ++i) { + createFile(topDirPath+"/"+entries[i]->getPath(), + entries[i]->getLength()+100); + } + std::deque > fileEntries + (&entries[0], &entries[arrayLength(entries)]); + + MultiDiskAdaptor adaptor; + adaptor.setStoreDir(dir); + adaptor.setTopDir(topDir); + adaptor.setFileEntries(fileEntries); + adaptor.setMaxOpenFiles(1); + + adaptor.openFile(); + + adaptor.cutTrailingGarbage(); + + CPPUNIT_ASSERT_EQUAL((uint64_t)256, + File(topDirPath+"/"+entries[0]->getPath()).size()); + CPPUNIT_ASSERT_EQUAL((uint64_t)512, + File(topDirPath+"/"+entries[1]->getPath()).size()); +} + } // namespace aria2 diff --git a/test/TestUtil.cc b/test/TestUtil.cc new file mode 100644 index 00000000..5a10e6fd --- /dev/null +++ b/test/TestUtil.cc @@ -0,0 +1,15 @@ +#include "TestUtil.h" +#include "a2io.h" +#include +#include +#include + +namespace aria2 { + +void createFile(const std::string& path, size_t length) +{ + int fd = creat(path.c_str(), OPEN_MODE); + ftruncate(fd, length); +} + +}; diff --git a/test/TestUtil.h b/test/TestUtil.h new file mode 100644 index 00000000..0662a101 --- /dev/null +++ b/test/TestUtil.h @@ -0,0 +1,8 @@ +#include "common.h" +#include + +namespace aria2 { + +void createFile(const std::string& filename, size_t length); + +} // namespace aria2