2008-08-26 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

	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

@@ -1,3 +1,20 @@
+2008-08-26  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	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  <tujikawa at rednoah dot com>
 
 	Bump up version number of dht.dat file to 3. In version 3 format, time

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)
 {

src/AbstractSingleDiskAdaptor.h

@@ -78,6 +78,8 @@ public:
   
   virtual bool directIOAllowed() const;
   
+  virtual void cutTrailingGarbage();
+
   void setDiskWriter(const SharedHandle<DiskWriter>& diskWriter);
 
   SharedHandle<DiskWriter> getDiskWriter() const;

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

src/CheckIntegrityEntry.h

@@ -70,6 +70,8 @@ public:
 
   virtual void onDownloadIncomplete(std::deque<Command*>& commands,
 				    DownloadEngine* e) = 0;
+
+  void cutTrailingGarbage();
 };
 
 typedef SharedHandle<CheckIntegrityEntry> CheckIntegrityEntryHandle;

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<DiskAdaptor> DiskAdaptorHandle;

src/MultiDiskAdaptor.cc

@@ -442,6 +442,19 @@ void MultiDiskAdaptor::disableDirectIO()
   }
 }
 
+void MultiDiskAdaptor::cutTrailingGarbage()
+{
+  for(std::deque<SharedHandle<DiskWriterEntry> >::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;

src/MultiDiskAdaptor.h

@@ -181,6 +181,8 @@ public:
     _directIOAllowed = b;
   }
 
+  virtual void cutTrailingGarbage();
+
   void setMaxOpenFiles(size_t maxOpenFiles);
 };
 

src/RequestGroup.cc

@@ -318,6 +318,7 @@ void RequestGroup::processCheckIntegrityEntry(std::deque<Command*>& 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);

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 <iostream>
+#include <cppunit/extensions/HelperMacros.h>
+
+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<FileEntry> entry
+    (new FileEntry("aria2_DirectDiskAdaptorTest_testCutTrailingGarbage",
+		   256, 0));
+  createFile(dir+"/"+entry->getPath(), entry->getLength()+100);
+
+  std::deque<SharedHandle<FileEntry> > fileEntries;
+  fileEntries.push_back(entry);
+
+  DirectDiskAdaptor adaptor;
+  adaptor.setDiskWriter(SharedHandle<DiskWriter>(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

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

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@

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 <string>
 #include <cerrno>
 #include <cstring>
@@ -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<MultiDiskAdaptor> 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<FileEntry> entries[] = {
+    SharedHandle<FileEntry>(new FileEntry(prefix+"1", 256, 0)),
+    SharedHandle<FileEntry>(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<SharedHandle<FileEntry> > 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

test/TestUtil.cc

@@ -0,0 +1,15 @@
+#include "TestUtil.h"
+#include "a2io.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+namespace aria2 {
+
+void createFile(const std::string& path, size_t length)
+{
+  int fd = creat(path.c_str(), OPEN_MODE);
+  ftruncate(fd, length);
+}
+
+};

test/TestUtil.h

@@ -0,0 +1,8 @@
+#include "common.h"
+#include <string>
+
+namespace aria2 {
+
+void createFile(const std::string& filename, size_t length);
+
+} // namespace aria2