fix file collision logic in torrent_info

include/libtorrent/file_storage.hpp

@@ -37,6 +37,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include <vector>
 #include <ctime>
 #include <boost/cstdint.hpp>
+#include <boost/unordered_set.hpp>
 
 #include "libtorrent/assert.hpp"
 #include "libtorrent/peer_request.hpp"
@@ -468,9 +469,13 @@ namespace libtorrent
 		// returns the crc32 hash of file_path(index)
 		boost::uint32_t file_path_hash(int index, std::string const& save_path) const;
 
-		// returns the crc32 hash of the path at index. Note, this index does not
+		// this will add the CRC32 hash of all directory entries to the table. No
-		// refer to a file, but to a path in the vector returned by paths().
+		// filename will be included, just directories. Every depth of directories
-		boost::uint32_t path_hash(int index, std::string const& save_path) const;
+		// are added separately to allow test for collisions with files at all
+		// levels. i.e. if one path in the torrent is ``foo/bar/baz``, the CRC32
+		// hashes for ``foo``, ``foo/bar`` and ``foo/bar/baz`` will be added to
+		// the set.
+		void all_path_hashes(boost::unordered_set<boost::uint32_t>& table) const;
 
 		// flags indicating various attributes for files in
 		// a file_storage.

src/file.cpp

@@ -30,10 +30,6 @@ POSSIBILITY OF SUCH DAMAGE.
 
 */
 
-/*
-	Physical file offset patch by Morten Husveit
-*/
-
 #define _FILE_OFFSET_BITS 64
 #define _LARGE_FILES 1
 

src/file_storage.cpp

@@ -575,26 +575,41 @@ namespace libtorrent
 			for (int i = 0; i < len; ++i, ++str)
 				crc.process_byte(to_lower(*str));
 		}
+
+		template <class CRC>
+		void process_path_lowercase(
+			boost::unordered_set<boost::uint32_t>& table
+			, CRC crc
+			, char const* str, int len)
+		{
+			if (len == 0) return;
+			for (int i = 0; i < len; ++i, ++str)
+			{
+				if (*str == TORRENT_SEPARATOR)
+					table.insert(crc.checksum());
+				crc.process_byte(to_lower(*str));
+			}
+			table.insert(crc.checksum());
+		}
 	}
 
-	boost::uint32_t file_storage::path_hash(int index
+	void file_storage::all_path_hashes(
-		, std::string const& save_path) const
+		boost::unordered_set<boost::uint32_t>& table) const
 	{
-		TORRENT_ASSERT_PRECOND(index >= 0 && index < int(m_paths.size()));
-		
 		boost::crc_optimal<32, 0x1EDC6F41, 0xFFFFFFFF, 0xFFFFFFFF, true, true> crc;
 
-		if (!save_path.empty())
+		if (!m_name.empty())
 		{
-			process_string_lowercase(crc, save_path.c_str(), save_path.size());
+			process_string_lowercase(crc, m_name.c_str(), m_name.size());
-			TORRENT_ASSERT(save_path[save_path.size()-1] != TORRENT_SEPARATOR);
+			TORRENT_ASSERT(m_name[m_name.size()-1] != TORRENT_SEPARATOR);
 			crc.process_byte(TORRENT_SEPARATOR);
 		}
 
-		process_string_lowercase(crc, m_name.c_str(), m_name.size());
+		for (int i = 0; i != int(m_paths.size()); ++i)
-		crc.process_byte(TORRENT_SEPARATOR);
+		{
-		process_string_lowercase(crc, m_paths[index].c_str(), m_paths[index].size());
+			std::string const& p = m_paths[i];
-		return crc.checksum();
+			process_path_lowercase(table, crc, p.c_str(), p.size());
+		}
 	}
 
 	boost::uint32_t file_storage::file_path_hash(int index

src/torrent_info.cpp

@@ -65,9 +65,7 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include <boost/bind.hpp>
 #include <boost/assert.hpp>
-#if TORRENT_HAS_BOOST_UNORDERED
 #include <boost/unordered_set.hpp>
-#endif
 
 #include <set>
 
@@ -776,22 +774,13 @@ namespace libtorrent
 	{
 		INVARIANT_CHECK;
 
-#if TORRENT_HAS_BOOST_UNORDERED
 		boost::unordered_set<boost::uint32_t> files;
-#else
-		std::set<boost::uint32_t> files;
-#endif
 
 		std::string empty_str;
 
 		// insert all directories first, to make sure no files
 		// are allowed to collied with them
-		std::vector<std::string> const& paths = m_files.paths();
+		m_files.all_path_hashes(files);
-		for (int i = 0; i != int(paths.size()); ++i)
-		{
-			files.insert(m_files.path_hash(i, empty_str));
-		}
-
 		for (int i = 0; i < m_files.num_files(); ++i)
 		{
 			// as long as this file already exists
@@ -827,7 +816,16 @@ namespace libtorrent
 		for (std::vector<std::string>::const_iterator i = paths.begin()
 			, end(paths.end()); i != end; ++i)
 		{
-			files.insert(combine_path(m_files.name(), *i));
+			std::string p = combine_path(m_files.name(), *i);
+			files.insert(p);
+			while (has_parent_path(p))
+			{
+				p = parent_path(p);
+				// we don't want trailing slashes here
+				TORRENT_ASSERT(p.back() == *TORRENT_SEPARATOR);
+				p.pop_back();
+				files.insert(p);
+			}
 		}
 
 		for (int i = 0; i < m_files.num_files(); ++i)

test/test_rss.cpp

@@ -97,6 +97,7 @@ void test_feed(std::string const& filename, rss_expect const& expect)
 	settings_pack pack;
 	pack.set_int(settings_pack::max_retry_port_bind, 100);
 	pack.set_str(settings_pack::listen_interfaces, "0.0.0.0:100");
+	// TODO: 4 we can't use session_impl here
 	boost::shared_ptr<aux::session_impl> s = boost::make_shared<aux::session_impl>();
 	settings_pack p;
 	s->start_session(p);

test/test_torrent_info.cpp

@@ -574,22 +574,37 @@ int test_torrent_parse()
 	return 0;
 }
 
-void test_resolve_duplicates()
+void test_resolve_duplicates(int test_case)
 {
 	file_storage fs;
 
-	fs.add_file("test/temporary.txt", 0x4000);
+	switch (test_case)
-	fs.add_file("test/A/tmp", 0x4000);
+	{
-	fs.add_file("test/Temporary.txt", 0x4000);
+		case 0:
-	fs.add_file("test/TeMPorArY.txT", 0x4000);
+			fs.add_file("test/temporary.txt", 0x4000);
-	fs.add_file("test/a", 0x4000);
+			fs.add_file("test/Temporary.txt", 0x4000);
-	fs.add_file("test/b.exe", 0x4000);
+			fs.add_file("test/TeMPorArY.txT", 0x4000);
-	fs.add_file("test/B.ExE", 0x4000);
+			fs.add_file("test/test/TEMPORARY.TXT", 0x4000);
-	fs.add_file("test/B.exe", 0x4000);
+			break;
-	fs.add_file("test/test/TEMPORARY.TXT", 0x4000);
+		case 1:
-	fs.add_file("test/A", 0x4000);
+			fs.add_file("test/b.exe", 0x4000);
-	fs.add_file("test/long/path/name/that/collides", 0x4000);
+			fs.add_file("test/B.ExE", 0x4000);
-	fs.add_file("test/long/path", 0x4000);
+			fs.add_file("test/B.exe", 0x4000);
+			fs.add_file("test/filler", 0x4000);
+			break;
+		case 2:
+			fs.add_file("test/A/tmp", 0x4000);
+			fs.add_file("test/a", 0x4000);
+			fs.add_file("test/A", 0x4000);
+			fs.add_file("test/filler", 0x4000);
+			break;
+		case 3:
+			fs.add_file("test/long/path/name/that/collides", 0x4000);
+			fs.add_file("test/long/path", 0x4000);
+			fs.add_file("test/filler-1", 0x4000);
+			fs.add_file("test/filler-2", 0x4000);
+			break;
+	}
 
 	libtorrent::create_torrent t(fs, 0x4000);
 
@@ -607,30 +622,45 @@ void test_resolve_duplicates()
 
 	torrent_info ti(&tmp[0], tmp.size());
 
-	char const* filenames[] =
+	char const* filenames[4][4] =
 	{
-	"test/temporary.txt",
+		{ // case 0
-	"test/A/tmp",
+			"test/temporary.txt",
-	"test/Temporary.1.txt", // duplicate of temporary.txt
+			"test/Temporary.1.txt", // duplicate of temporary.txt
-	"test/TeMPorArY.2.txT", // duplicate of temporary.txt
+			"test/TeMPorArY.2.txT", // duplicate of temporary.txt
-	"test/a.1", // a file may not have the same name as a directory
+			// a file with the same name in a seprate directory is fine
-	"test/b.exe",
+			"test/test/TEMPORARY.TXT",
-	"test/B.1.ExE", // duplicate of b.exe
+		},
-	"test/B.2.exe", // duplicate of b.exe
+		{ // case 1
-	"test/test/TEMPORARY.TXT", // a file with the same name in a seprate directory is fine
+			"test/b.exe",
-	"test/A.2", // duplicate of directory a
+			"test/B.1.ExE", // duplicate of b.exe
-	"test/long/path/name/that/collides", // a subset of this path collides with the next filename
+			"test/B.2.exe", // duplicate of b.exe
-	"test/long/path.1" // so this file needs to be renamed, to not collide with the path name
+			"test/filler",
+		},
+		{ // case 2
+			"test/A/tmp",
+			"test/a.1", // a file may not have the same name as a directory
+			"test/A.2", // duplicate of directory a
+			"test/filler",
+		},
+		{ // case 3
+			// a subset of this path collides with the next filename
+			"test/long/path/name/that/collides",
+			// so this file needs to be renamed, to not collide with the path name
+			"test/long/path.1",
+			"test/filler-1",
+			"test/filler-2",
+		}
 	};
 
 	for (int i = 0; i < ti.num_files(); ++i)
 	{
 		std::string p = ti.files().file_path(i);
 		convert_path_to_posix(p);
-		fprintf(stderr, "%s == %s\n", p.c_str(), filenames[i]);
+		fprintf(stderr, "%s == %s\n", p.c_str(), filenames[test_case][i]);
 
 		// TODO: 3 fix duplicate name detection to make this test pass
-		TEST_EQUAL(p, filenames[i]);
+		TEST_EQUAL(p, filenames[test_case][i]);
 	}
 }
 
@@ -692,7 +722,9 @@ void test_copy()
 
 int test_main()
 {
-	test_resolve_duplicates();
+	for (int i = 0; i < 4; ++i)
+		test_resolve_duplicates(i);
+
 	test_copy();
 #ifndef TORRENT_DISABLE_MUTABLE_TORRENTS
 	test_mutable_torrents();