Sanitize Download Names Before Choosing an Unused One

The unused-name search probed the name as given and sanitized the
result afterwards, so a name whose special characters are replaced
could be mapped onto a file that already exists.

Move the search into libslic3r as find_unused_filename, sanitize first
and probe the name that is actually written. The download marker path
is shared through download_marker_path. Restore the last tried name in
the error reported when no free name is found, and cover the search
with unit tests.

src/libslic3r/Utils.hpp

@@ -293,6 +293,13 @@ inline std::string sanitize_file_basename(const std::string &name){
     // Names made only of dots and spaces refer to the folder or its parent, or are stripped to nothing on Windows.
     return base.find_first_not_of(". ") == std::string::npos ? std::string() : base;
 }
+// Marker file a download of this process writes to before it is renamed to filename.
+boost::filesystem::path download_marker_path(const boost::filesystem::path &dest_folder, const std::string &filename);
+// Finds a sanitized variant of filename, "name(N).ext" if needed, that neither an entry of dest_folder
+// nor the download marker of another download uses. The marker at ignored_marker does not count.
+// Returns true and the name in result, or false and the last name tried.
+bool find_unused_filename(const boost::filesystem::path &dest_folder, const std::string &filename,
+                          const boost::filesystem::path &ignored_marker, std::string &result);
 // File path / name / extension splitting utilities, working with UTF-8,
 // to be published to Perl.
 namespace PerlUtils {

src/libslic3r/utils.cpp

@@ -1321,6 +1321,31 @@ unsigned get_current_pid()
 #endif
 }
 
+boost::filesystem::path download_marker_path(const boost::filesystem::path &dest_folder, const std::string &filename)
+{
+    return dest_folder / (filename + "." + std::to_string(get_current_pid()) + ".download");
+}
+
+bool find_unused_filename(const boost::filesystem::path &dest_folder, const std::string &filename,
+                          const boost::filesystem::path &ignored_marker, std::string &result)
+{
+    // Probe the name that will be written, so a name the sanitizing maps onto an existing file is versioned too.
+    const std::string sanitized = sanitize_filename(filename);
+    const std::string extension = boost::filesystem::path(sanitized).extension().string();
+    const std::string stem      = sanitized.substr(0, sanitized.size() - extension.size());
+    auto is_used = [&](const std::string &name) {
+        const boost::filesystem::path marker = download_marker_path(dest_folder, name);
+        return boost::filesystem::exists(dest_folder / name) || (marker != ignored_marker && boost::filesystem::exists(marker));
+    };
+    result = sanitized;
+    for (size_t version = 1; is_used(result); ++version) {
+        if (version > 999)
+            return false;
+        result = stem + "(" + std::to_string(version) + ")" + extension;
+    }
+    return true;
+}
+
 std::string per_user_temp_id()
 {
 #ifdef WIN32

src/slic3r/GUI/DownloaderFileGet.cpp

@@ -71,17 +71,6 @@ bool FileGet::is_subdomain(const std::string& url, const std::string& domain)
 	return false;
 }
 
-namespace {
-unsigned get_current_pid()
-{
-#ifdef WIN32
-	return GetCurrentProcessId();
-#else
-	return ::getpid();
-#endif
-}
-}
-
 // int = DOWNLOAD ID; string = file path
 wxDEFINE_EVENT(EVT_DWNLDR_FILE_COMPLETE, wxCommandEvent);
 // int = DOWNLOAD ID; string = error msg
@@ -112,7 +101,6 @@ struct FileGet::priv
 	priv(int ID, std::string&& url, const std::string& filename, wxEvtHandler* evt_handler, const boost::filesystem::path& dest_folder);
 
 	void get_perform();
-	std::string find_unused_filename(const std::string& filename) const;
 };
 
 FileGet::priv::priv(int ID, std::string&& url, const std::string& filename, wxEvtHandler* evt_handler, const boost::filesystem::path& dest_folder)
@@ -132,24 +120,6 @@ std::string extract_remote_filename(const std::string& str) {
         return "";
     }
 }
-// Returns a sanitized name that no file or other download in the destination folder uses yet,
-// or an empty string when none is found.
-std::string FileGet::priv::find_unused_filename(const std::string& filename) const
-{
-	const std::string extension = boost::filesystem::path(filename).extension().string();
-	const std::string just_filename = filename.substr(0, filename.size() - extension.size());
-	std::string final_filename = just_filename;
-	auto is_used = [this, &extension](const std::string& name) {
-		const boost::filesystem::path tmp_path = m_dest_folder / (name + extension + "." + std::to_string(get_current_pid()) + ".download");
-		return boost::filesystem::exists(m_dest_folder / (name + extension)) || (tmp_path != m_tmp_path && boost::filesystem::exists(tmp_path));
-	};
-	for (size_t version = 1; is_used(final_filename); ++version) {
-		if (version > 999)
-			return {};
-		final_filename = GUI::format("%1%(%2%)", just_filename, std::to_string(version));
-	}
-	return sanitize_filename(final_filename + extension);
-}
 void FileGet::priv::get_perform()
 {
 	assert(m_evt_handler);
@@ -163,10 +133,10 @@ void FileGet::priv::get_perform()
 	std::string extension;
 	if (m_written == 0)
 	{
-		extension = (m_dest_folder / m_filename).extension().string();
 		std::string final_filename;
+		bool found = false;
 		try {
-			final_filename = find_unused_filename(m_filename);
+			found = find_unused_filename(m_dest_folder, m_filename, m_tmp_path, final_filename);
 		} catch (const boost::filesystem::filesystem_error& e)
 		{
 			wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_ERROR);
@@ -175,17 +145,18 @@ void FileGet::priv::get_perform()
 			m_evt_handler->QueueEvent(evt);
 			return;
 		}
-		if (final_filename.empty()) {
+		if (!found) {
 			wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_ERROR);
-			evt->SetString(GUI::format_wxstr(L"Failed to find suitable filename. Last name: %1%." , (m_dest_folder / m_filename).string()));
+			evt->SetString(GUI::format_wxstr(L"Failed to find suitable filename. Last name: %1%." , (m_dest_folder / final_filename).string()));
 			evt->SetInt(m_id);
 			m_evt_handler->QueueEvent(evt);
 			return;
 		}
 
         m_filename = final_filename;
+		extension = boost::filesystem::path(m_filename).extension().string();
 
-        m_tmp_path = m_dest_folder / (m_filename + "." + std::to_string(get_current_pid()) + ".download");
+        m_tmp_path = download_marker_path(m_dest_folder, m_filename);
 
 		wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_NAME_CHANGE);
 		evt->SetString(boost::nowide::widen(m_filename));
@@ -233,16 +204,17 @@ void FileGet::priv::get_perform()
 				std::string filename = extract_remote_filename(header);
 				if (!filename.empty()) {
 					// The name comes from the server: keep it inside the destination folder and never
-					// replace an existing file. Fall back to the URL-derived name if nothing usable remains.
+					// replace an existing file. Keep the current name if nothing usable remains.
 					filename = sanitize_file_basename(filename);
+					std::string unused;
 					try {
-						if (!filename.empty())
-							filename = find_unused_filename(filename);
+						if (filename.empty() || !find_unused_filename(m_dest_folder, filename, m_tmp_path, unused))
+							unused.clear();
 					} catch (const boost::filesystem::filesystem_error&) {
-						filename.clear();
+						unused.clear();
 					}
-					if (!filename.empty())
-						m_filename = filename;
+					if (!unused.empty())
+						m_filename = unused;
 					dest_path = m_dest_folder / m_filename;
 					wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_NAME_CHANGE);
 					evt->SetString(boost::nowide::widen(m_filename));

tests/libslic3r/test_utils.cpp

@@ -163,12 +163,13 @@ TEST_CASE("sanitize_file_basename keeps only a plain file name from an untrusted
         {"C:x.3mf", "C_x.3mf"},
         {"/etc/x", "x"},
         {"a/b\\c.gcode", "c.gcode"},
+        {"x:stream", "x_stream"}, // no NTFS alternate data stream
+        {"x.", "x."},
+        {".3mf", ".3mf"},
         {unicode, unicode},
     }));
     CAPTURE(input);
-    const std::string name = sanitize_file_basename(input);
-    CHECK(name == expected);
-    CHECK(name.find_first_of("/\\:") == std::string::npos);
+    CHECK(sanitize_file_basename(input) == expected);
 }
 
 TEST_CASE("sanitize_file_basename rejects names that do not name a file", "[Utils]") {
@@ -176,3 +177,69 @@ TEST_CASE("sanitize_file_basename rejects names that do not name a file", "[Util
     CAPTURE(input);
     CHECK(sanitize_file_basename(input).empty());
 }
+
+namespace {
+void touch(const boost::filesystem::path &path) { std::ofstream(path.string()) << "existing"; }
+std::string file_contents(const boost::filesystem::path &path)
+{
+    std::ifstream file(path.string());
+    return std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>());
+}
+} // namespace
+
+TEST_CASE("find_unused_filename keeps a name nothing uses", "[Utils]") {
+    ScopedTemporaryDir dir;
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name));
+    CHECK(name == "model.3mf");
+}
+
+TEST_CASE("find_unused_filename versions a name an existing file uses", "[Utils]") {
+    ScopedTemporaryDir dir;
+    touch(dir.path() / "model.3mf");
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name));
+    CHECK(name == "model(1).3mf");
+}
+
+TEST_CASE("find_unused_filename versions a name that maps onto an existing file once sanitized", "[Utils]") {
+    ScopedTemporaryDir dir;
+    touch(dir.path() / "my_model.3mf");
+    const std::string input = GENERATE(as<std::string>{}, "my?model.3mf", "my:model.3mf", "my*model.3mf");
+    CAPTURE(input);
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), input, {}, name));
+    CHECK(name == "my_model(1).3mf");
+    CHECK(file_contents(dir.path() / "my_model.3mf") == "existing");
+}
+
+TEST_CASE("find_unused_filename treats the marker of another download as used", "[Utils]") {
+    ScopedTemporaryDir dir;
+    touch(download_marker_path(dir.path(), "model.3mf"));
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name));
+    CHECK(name == "model(1).3mf");
+}
+
+TEST_CASE("find_unused_filename ignores the marker of the download asking", "[Utils]") {
+    ScopedTemporaryDir dir;
+    const boost::filesystem::path own_marker = download_marker_path(dir.path(), "model.3mf");
+    touch(own_marker);
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), "model.3mf", own_marker, name));
+    CHECK(name == "model.3mf");
+}
+
+TEST_CASE("find_unused_filename gives up after 999 versions", "[Utils]") {
+    ScopedTemporaryDir dir;
+    touch(dir.path() / "model.3mf");
+    for (int version = 1; version < 999; ++version)
+        touch(dir.path() / ("model(" + std::to_string(version) + ").3mf"));
+    std::string name;
+    REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name));
+    CHECK(name == "model(999).3mf");
+
+    touch(dir.path() / name);
+    REQUIRE_FALSE(find_unused_filename(dir.path(), "model.3mf", {}, name));
+    CHECK(name == "model(999).3mf");
+}