Handle Filesystem Errors When Finishing a Model Import Download

Choosing the final name and moving the downloaded project into place
could throw from inside the download callback. Any such error now removes
the temporary file and reports the existing import failure message.

src/libslic3r/Utils.hpp

@@ -256,13 +256,10 @@ extern bool is_gallery_file(const std::string& path, char const* type);
 extern bool is_shapes_dir(const std::string& dir);
 //BBS: add json support
 extern bool is_json_file(const std::string& path);
-// True if rel_path is relative, has no ".." component or embedded NUL and, joined to root, still resolves inside it.
+// True if rel_path is relative, has no ".." component and, joined to root, still resolves inside it.
 // Both '/' and '\\' are treated as separators on every platform, so an archive rejected on one OS
 // is rejected on all of them.
 extern bool is_path_within_root(const std::string &rel_path, const boost::filesystem::path &root);
-// True if a symlink stored at link_rel_path (relative to root) with this target stays inside root: the target
-// must be relative, and joined to the link's directory it must pass is_path_within_root.
-extern bool is_symlink_target_within_root(const std::string &link_rel_path, const std::string &target, const boost::filesystem::path &root);
 
 // Orca: custom protocal support utils
 inline bool is_orca_open(const std::string& url) { return boost::starts_with(url, "orcaslicer://open"); }
@@ -288,6 +285,21 @@ inline std::string sanitize_filename(const std::string &filename){
     const std::regex special_chars("[/\\\\:*?\"<>|]");
     return std::regex_replace(filename, special_chars, "_");
 }
+// Reduce an untrusted, possibly path-qualified name to a single sanitized file name.
+// Returns an empty string when nothing usable remains.
+inline std::string sanitize_file_basename(const std::string &name){
+    const size_t sep = name.find_last_of("/\\");
+    const std::string base = sanitize_filename(sep == std::string::npos ? name : name.substr(sep + 1));
+    // 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/miniz_extension.cpp

@@ -4,9 +4,6 @@
 #include "miniz_extension.hpp"
 #include "Utils.hpp"
 
-#include <boost/filesystem.hpp>
-#include <boost/log/trivial.hpp>
-
 #if defined(_MSC_VER) || defined(__MINGW64__)
 #include "boost/nowide/cstdio.hpp"
 #endif
@@ -118,66 +115,6 @@ std::string decode_archive_entry_path(mz_zip_archive *zip, const mz_zip_archive_
     return decode_zip_unicode_path_extra_field(extra.substr(0, extra_size > 0 ? extra_size - 1 : 0), stat.m_filename);
 }
 
-bool extract_archive_confined(const std::string &zip_path_utf8, const std::string &dest_dir)
-{
-    mz_zip_archive archive;
-    mz_zip_zero_struct(&archive);
-
-    if (!open_zip_reader(&archive, zip_path_utf8)) {
-        BOOST_LOG_TRIVIAL(error) << "Unable to open zip reader for " << zip_path_utf8;
-        return false;
-    }
-
-    const mz_uint num_entries = mz_zip_reader_get_num_files(&archive);
-    mz_zip_archive_file_stat stat;
-
-    // Validate every entry first so an archive with a single escaping entry leaves no partial output behind.
-    const boost::filesystem::path root(dest_dir);
-    for (mz_uint i = 0; i < num_entries; ++i) {
-        if (mz_zip_reader_file_stat(&archive, i, &stat) && !is_path_within_root(stat.m_filename, root)) {
-            BOOST_LOG_TRIVIAL(error) << "Unzip: rejecting " << zip_path_utf8 << ", entry " << stat.m_filename << " resolves outside " << dest_dir;
-            close_zip_reader(&archive);
-            return false;
-        }
-    }
-
-    for (mz_uint i = 0; i < num_entries; ++i) {
-        if (!mz_zip_reader_file_stat(&archive, i, &stat)) {
-            BOOST_LOG_TRIVIAL(warning) << "Unzip: read file stat failed";
-            continue;
-        }
-        const std::string dest_file = dest_dir + "/" + stat.m_filename;
-        try {
-            if (stat.m_is_directory) {
-                const boost::filesystem::path dest_path(dest_file);
-                if (!boost::filesystem::exists(dest_path))
-                    boost::filesystem::create_directories(dest_path);
-                continue;
-            }
-            if (stat.m_uncomp_size == 0) {
-                BOOST_LOG_TRIVIAL(warning) << "Unzip: invalid size for file " << stat.m_filename;
-                continue;
-            }
-            // Replace a symlink at the destination rather than writing through it.
-            const boost::filesystem::path dest_path(dest_file);
-            if (boost::filesystem::is_symlink(boost::filesystem::symlink_status(dest_path)))
-                boost::filesystem::remove(dest_path);
-            if (!mz_zip_reader_extract_to_file(&archive, stat.m_file_index, dest_file.c_str(), 0)) {
-                BOOST_LOG_TRIVIAL(error) << "Unzip: extract file " << stat.m_filename << " to dest " << dest_file << " failed";
-                close_zip_reader(&archive);
-                return false;
-            }
-            BOOST_LOG_TRIVIAL(info) << "Unzip: successfully extract file " << stat.m_file_index << " to " << dest_file;
-        } catch (const std::exception &e) {
-            close_zip_reader(&archive);
-            BOOST_LOG_TRIVIAL(error) << "Unzip: archive read exception: " << e.what();
-            return false;
-        }
-    }
-    close_zip_reader(&archive);
-    return true;
-}
-
 MZ_Archive::MZ_Archive()
 {
     mz_zip_zero_struct(&arch);

src/libslic3r/miniz_extension.hpp

@@ -11,8 +11,6 @@ bool open_zip_writer(mz_zip_archive *zip, const std::string &fname_utf8);
 bool close_zip_reader(mz_zip_archive *zip);
 bool close_zip_writer(mz_zip_archive *zip);
 std::string decode_archive_entry_path(mz_zip_archive *zip, const mz_zip_archive_file_stat &stat);
-// Extracts every entry of the archive under dest_dir. Nothing is written if any entry would resolve outside dest_dir.
-bool extract_archive_confined(const std::string &zip_path_utf8, const std::string &dest_dir);
 
 class MZ_Archive {
 public:

src/libslic3r/utils.cpp

@@ -1093,9 +1093,6 @@ bool is_path_within_root(const std::string &rel_path, const boost::filesystem::p
     auto is_separator = [](char c) { return c == '/' || c == '\\'; };
     if (rel_path.empty() || is_separator(rel_path.front()) || (rel_path.size() > 1 && rel_path[1] == ':'))
         return false;
-    // The filesystem calls stop at a NUL, so they would act on a shorter path than the one checked here.
-    if (rel_path.find('\0') != std::string::npos)
-        return false;
     for (size_t start = 0; start <= rel_path.size();) {
         size_t end = start;
         while (end < rel_path.size() && !is_separator(rel_path[end]))
@@ -1106,10 +1103,7 @@ bool is_path_within_root(const std::string &rel_path, const boost::filesystem::p
     }
     // Resolve against the canonical root so a symlink inside it cannot lead back out.
     try {
-        std::string root_str = boost::filesystem::weakly_canonical(root).string();
-        // A trailing separator on root would otherwise fail the prefix match below for every path.
-        while (!root_str.empty() && (root_str.back() == '/' || root_str.back() == boost::filesystem::path::preferred_separator))
-            root_str.pop_back();
+        const std::string root_str = boost::filesystem::weakly_canonical(root).string();
         const std::string full_str = boost::filesystem::weakly_canonical(root / rel_path).string();
         return full_str.compare(0, root_str.size(), root_str) == 0 &&
                (full_str.size() == root_str.size() || full_str[root_str.size()] == boost::filesystem::path::preferred_separator);
@@ -1118,15 +1112,6 @@ bool is_path_within_root(const std::string &rel_path, const boost::filesystem::p
     }
 }
 
-bool is_symlink_target_within_root(const std::string &link_rel_path, const std::string &target, const boost::filesystem::path &root)
-{
-    if (target.empty() || target.front() == '/' || target.front() == '\\' || (target.size() > 1 && target[1] == ':'))
-        return false;
-    // A relative target without ".." only descends from the link's directory, so no chain of such links can leave root.
-    const size_t sep = link_rel_path.find_last_of("/\\");
-    return is_path_within_root((sep == std::string::npos ? std::string() : link_rel_path.substr(0, sep + 1)) + target, root);
-}
-
 bool is_img_file(const std::string &path)
 {
 	return boost::iends_with(path, ".png") || boost::iends_with(path, ".svg");
@@ -1336,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
@@ -144,25 +133,10 @@ void FileGet::priv::get_perform()
 	std::string extension;
 	if (m_written == 0)
 	{
-		boost::filesystem::path dest_path = m_dest_folder / m_filename;
-		extension = dest_path.extension().string();
-		std::string just_filename = m_filename.substr(0, m_filename.size() - extension.size());
-		std::string final_filename = just_filename;
-        // Find unsed filename 
+		std::string final_filename;
+		bool found = false;
 		try {
-			size_t version = 0;
-			while (boost::filesystem::exists(m_dest_folder / (final_filename + extension)) || boost::filesystem::exists(m_dest_folder / (final_filename + extension + "." + std::to_string(get_current_pid()) + ".download")))
-			{
-				++version;
-				if (version > 999) {
-					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 / (final_filename + extension)).string()));
-					evt->SetInt(m_id);
-					m_evt_handler->QueueEvent(evt);
-					return;
-				}
-				final_filename = GUI::format("%1%(%2%)", just_filename, std::to_string(version));
-			}
+			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);
@@ -171,10 +145,18 @@ void FileGet::priv::get_perform()
 			m_evt_handler->QueueEvent(evt);
 			return;
 		}
+		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 / final_filename).string()));
+			evt->SetInt(m_id);
+			m_evt_handler->QueueEvent(evt);
+			return;
+		}
 
-        m_filename = sanitize_filename(final_filename + extension);
+        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));
@@ -221,7 +203,32 @@ void FileGet::priv::get_perform()
 			if(dest_path.empty()) {
 				std::string filename = extract_remote_filename(header);
 				if (!filename.empty()) {
-					m_filename = filename;
+					// The name comes from the server: keep it inside the destination folder and never
+					// replace an existing file. Keep the current name if nothing usable remains.
+					filename = sanitize_file_basename(filename);
+					std::string unused;
+					try {
+						if (filename.empty() || !find_unused_filename(m_dest_folder, filename, m_tmp_path, unused))
+							unused.clear();
+					} catch (const boost::filesystem::filesystem_error&) {
+						unused.clear();
+					}
+					const boost::filesystem::path tmp_path = unused.empty() ? m_tmp_path : download_marker_path(m_dest_folder, unused);
+					if (tmp_path != m_tmp_path) {
+						// Move the marker to the adopted name so that other downloads see the name as taken.
+						// Only before anything is written, so that no downloaded data has to be carried over.
+						FILE* tmp_file = m_written == 0 ? fopen(wxString(tmp_path.wstring()).c_str(), "wb") : nullptr;
+						if (tmp_file != nullptr) {
+							fclose(file);
+							boost::system::error_code ec;
+							boost::filesystem::remove(m_tmp_path, ec);
+							file = tmp_file;
+							m_tmp_path = tmp_path;
+						} else
+							unused.clear();
+					}
+					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));
@@ -327,6 +334,18 @@ void FileGet::priv::get_perform()
                     m_evt_handler->QueueEvent(evt);
                 }
 				fclose(file);
+				// Another file may have taken the name while downloading.
+				if (!dest_path.empty() && boost::filesystem::exists(dest_path)) {
+					std::string unused;
+					if (!find_unused_filename(m_dest_folder, m_filename, m_tmp_path, unused))
+						throw std::runtime_error("No unused file name.");
+					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));
+					evt->SetInt(m_id);
+					m_evt_handler->QueueEvent(evt);
+				}
 				boost::filesystem::rename(m_tmp_path, dest_path);
 			}
 			catch (const std::exception& /*e*/)

src/slic3r/GUI/GUI_App.cpp

@@ -1512,33 +1512,11 @@ int GUI_App::install_plugin(std::string name, std::string package_name, InstallP
                     size_t n = mz_zip_reader_get_extra(&archive, stat.m_file_index, extra.data(), extra.size());
                     dest_file = decode(extra.substr(0, n), stat.m_filename);
                 }
-                if (!is_path_within_root(dest_file, plugin_folder)) {
-                    BOOST_LOG_TRIVIAL(error) << "[install_plugin] entry " << dest_file << " resolves outside " << plugin_folder.string();
-                    close_zip_reader(&archive);
-                    if (pro_fn) { pro_fn(InstallStatusUnzipFailed, 0, cancel); }
-                    return InstallStatusUnzipFailed;
-                }
                 auto dest_path = plugin_folder / dest_file;
+                boost::filesystem::create_directories(dest_path.parent_path());
                 std::string dest_zip_file = encode_path(dest_path.string().c_str());
-#ifndef WIN32
-                // Validate a symlink's target before anything at the destination is replaced.
-                const bool is_link = S_ISLNK(stat.m_external_attr >> 16);
-                std::string link;
-                if (is_link) {
-                    link.assign(stat.m_uncomp_size, 0);
-                    if (!mz_zip_reader_extract_to_mem(&archive, stat.m_file_index, link.data(), stat.m_uncomp_size, 0) ||
-                        !is_symlink_target_within_root(dest_file, link, plugin_folder)) {
-                        BOOST_LOG_TRIVIAL(error) << "[install_plugin] link " << dest_file << " -> " << link << " is unreadable or resolves outside " << plugin_folder.string();
-                        close_zip_reader(&archive);
-                        if (pro_fn) { pro_fn(InstallStatusUnzipFailed, 0, cancel); }
-                        return InstallStatusUnzipFailed;
-                    }
-                }
-#endif
                 try {
-                    boost::filesystem::create_directories(dest_path.parent_path());
-                    // symlink_status so that an existing symlink, dangling or not, is replaced rather than written through.
-                    if (fs::exists(fs::symlink_status(dest_path))) {
+                    if (fs::exists(dest_path)) {
                         boost::system::error_code ec;
                         fs::remove(dest_path, ec);
                         if (ec) {
@@ -1566,8 +1544,9 @@ int GUI_App::install_plugin(std::string name, std::string package_name, InstallP
                     }
                     mz_bool res = 0;
 #ifndef WIN32
-                    if (is_link) {
-                        res = 1;
+                    if (S_ISLNK(stat.m_external_attr >> 16)) {
+                        std::string link(stat.m_uncomp_size + 1, 0);
+                        res = mz_zip_reader_extract_to_mem(&archive, stat.m_file_index, link.data(), stat.m_uncomp_size, 0);
                         try {
                             boost::filesystem::create_symlink(link, dest_path);
                         } catch (const std::exception &e) {

src/slic3r/GUI/Plater.cpp

@@ -15681,6 +15681,11 @@ void Plater::import_model_id(wxString download_info)
         //wxString sError = error.what();
     }
 
+    // The name comes from the link: reduce it to a plain file name inside the download folder.
+    filename = from_u8(sanitize_file_basename(into_u8(filename)));
+    if (filename.empty())
+        filename = "untitled.3mf";
+
     bool download_ok = false;
     int retry_count = 0;
     const int max_retries = 3;
@@ -15722,51 +15727,28 @@ void Plater::import_model_id(wxString download_info)
 
         msg = _L("Preparing 3MF file...");
 
-        //gets the number of files with the same name
-        std::vector<wxString>   vecFiles;
-        bool                    is_already_exist = false;
-
-
         target_path = fs::path(wxGetApp().app_config->get("download_path"));
 
-        try
-        {
-            vecFiles.clear();
-            wxString extension = fs::path(filename.wx_str()).extension().c_str();
-
-
-            //check file suffix
-            if (!extension.Contains(".3mf")) {
-                msg = _L("Download failed; unknown file format.");
-                return;
-            }
-
-            auto name = filename.substr(0, filename.length() - extension.length() - 1);
-
-            for (const auto& iter : boost::filesystem::directory_iterator(target_path))
-            {
-                if (boost::filesystem::is_directory(iter.path()))
-                    continue;
-
-                wxString sFile = iter.path().filename().string().c_str();
-                if (strstr(sFile.c_str(), name.c_str()) != NULL) {
-                    vecFiles.push_back(sFile);
-                }
-
-                if (sFile == filename) is_already_exist = true;
-            }
-        }
-        catch (const std::exception&)
-        {
-            //wxString sError = error.what();
+        //check file suffix
+        wxString extension = fs::path(filename.wx_str()).extension().c_str();
+        if (!extension.Contains(".3mf")) {
+            msg = _L("Download failed; unknown file format.");
+            return;
         }
 
-        //update filename
-        if (is_already_exist && vecFiles.size() >= 1) {
-            wxString extension = fs::path(filename.wx_str()).extension().c_str();
-            wxString name = filename.substr(0, filename.length() - extension.length());
-            filename = wxString::Format("%s(%d)%s", name, vecFiles.size() + 1, extension).ToStdString();
+        //never replace an existing file
+        std::string unused_filename;
+        try {
+            if (!find_unused_filename(target_path, into_u8(filename), {}, unused_filename))
+                unused_filename.clear();
+        } catch (const std::exception&) {
+            unused_filename.clear();
         }
+        if (unused_filename.empty()) {
+            msg = _L("Importing to Orca Slicer failed. Please download the file and manually import it.");
+            return;
+        }
+        filename = from_u8(unused_filename);
 
 
         msg = _L("Downloading project...");
@@ -15778,10 +15760,6 @@ void Plater::import_model_id(wxString download_info)
         boost::uuids::uuid uuid = boost::uuids::random_generator()();
         std::string unique = to_string(uuid).substr(0, 6);
 
-        if (filename.empty()) {
-            filename = "untitled.3mf";
-        }
-
         //target_path /= (boost::format("%1%_%2%.3mf") % filename % unique).str();
         target_path /= fs::path(filename.wc_str());
 
@@ -15830,13 +15808,26 @@ void Plater::import_model_id(wxString download_info)
                         cont = false;
                     }
                 })
-                .on_complete([&cont, &download_ok, tmp_path, target_path](std::string body, unsigned /* http_status */) {
+                .on_complete([&cont, &download_ok, &msg, tmp_path, &target_path](std::string body, unsigned /* http_status */) {
                         fs::fstream file(tmp_path, std::ios::out | std::ios::binary | std::ios::trunc);
                         file.write(body.c_str(), body.size());
                         file.close();
-                        fs::rename(tmp_path, target_path);
                         cont = false;
-                        download_ok = true;
+                        try {
+                            // Another file may have taken the name while downloading.
+                            std::string unused_filename;
+                            if (find_unused_filename(target_path.parent_path(), target_path.filename().string(), {}, unused_filename)) {
+                                target_path = target_path.parent_path() / unused_filename;
+                                fs::rename(tmp_path, target_path);
+                                download_ok = true;
+                                return;
+                            }
+                        } catch (const std::exception &e) {
+                            BOOST_LOG_TRIVIAL(error) << "import_model_id: failed to move the download into place: " << e.what();
+                        }
+                        boost::system::error_code ec;
+                        fs::remove(tmp_path, ec);
+                        msg = _L("Importing to Orca Slicer failed. Please download the file and manually import it.");
                 }).perform_sync();
 
                 // for break while

src/slic3r/Utils/PresetUpdater.cpp

@@ -339,8 +339,62 @@ bool PresetUpdater::priv::get_file(const std::string &url, const fs::path &targe
 //BBS: refine preset update logic
 bool PresetUpdater::priv::extract_file(const fs::path &source_path, const fs::path &dest_path)
 {
-    const std::string parent_path = (!dest_path.empty() ? dest_path : source_path.parent_path()).string();
-    return extract_archive_confined(source_path.string(), parent_path);
+    bool res = true;
+    std::string file_path = source_path.string();
+    std::string parent_path = (!dest_path.empty() ? dest_path : source_path.parent_path()).string();
+    mz_zip_archive archive;
+    mz_zip_zero_struct(&archive);
+
+    if (!open_zip_reader(&archive, file_path))
+    {
+        BOOST_LOG_TRIVIAL(error) << "Unable to open zip reader for "<<file_path;
+        return false;
+    }
+
+    mz_uint num_entries = mz_zip_reader_get_num_files(&archive);
+
+    mz_zip_archive_file_stat stat;
+    // we first loop the entries to read from the archive the .amf file only, in order to extract the version from it
+    for (mz_uint i = 0; i < num_entries; ++i)
+    {
+        if (mz_zip_reader_file_stat(&archive, i, &stat))
+        {
+            std::string dest_file = parent_path+"/"+stat.m_filename;
+            if (stat.m_is_directory) {
+                fs::path dest_path(dest_file);
+                if (!fs::exists(dest_path))
+                    fs::create_directories(dest_path);
+				continue;
+            }
+            else if (stat.m_uncomp_size == 0) {
+                BOOST_LOG_TRIVIAL(warning) << "[Orca Updater]Unzip: invalid size for file "<<stat.m_filename;
+                continue;
+            }
+            try
+            {
+                res = mz_zip_reader_extract_to_file(&archive, stat.m_file_index, dest_file.c_str(), 0);
+                if (!res) {
+                    BOOST_LOG_TRIVIAL(error) << "[Orca Updater]extract file "<<stat.m_filename<<" to dest "<<dest_file<<" failed";
+                    close_zip_reader(&archive);
+                    return res;
+                }
+                BOOST_LOG_TRIVIAL(info) << "[Orca Updater]successfully extract file " << stat.m_file_index << " to "<<dest_file;
+            }
+            catch (const std::exception& e)
+            {
+                // ensure the zip archive is closed and rethrow the exception
+                close_zip_reader(&archive);
+                BOOST_LOG_TRIVIAL(error) << "[Orca Updater]Archive read exception:"<<e.what();
+                return false;
+            }
+        }
+        else {
+            BOOST_LOG_TRIVIAL(warning) << "[Orca Updater]Unzip: read file stat failed";
+        }
+    }
+    close_zip_reader(&archive);
+
+	return true;
 }
 
 // Remove a leftover partial archive for the vendor about to be synchronized.

tests/libslic3r/CMakeLists.txt

@@ -40,7 +40,6 @@ add_executable(${_TEST_NAME}_tests
     test_lay_on_face.cpp
     test_model.cpp
     test_utils.cpp
-    test_miniz_extension.cpp
     test_timeutils.cpp
     test_voronoi.cpp
     test_wipe_tower_estimate.cpp

tests/libslic3r/test_miniz_extension.cpp

@@ -1,194 +0,0 @@
-#include <catch2/catch_all.hpp>
-
-#include "libslic3r/miniz_extension.hpp"
-
-#include "test_utils.hpp"
-
-#include <boost/filesystem.hpp>
-
-#include <algorithm>
-#include <fstream>
-#include <iterator>
-#include <string>
-#include <utility>
-#include <vector>
-
-using namespace Slic3r;
-namespace fs = boost::filesystem;
-
-namespace {
-
-void write_zip(const fs::path &zip_file, const std::vector<std::pair<std::string, std::string>> &entries)
-{
-    mz_zip_archive zip;
-    mz_zip_zero_struct(&zip);
-    REQUIRE(open_zip_writer(&zip, zip_file.string()));
-    for (const auto &[name, content] : entries)
-        REQUIRE(mz_zip_writer_add_mem(&zip, name.c_str(), content.data(), content.size(), MZ_DEFAULT_COMPRESSION));
-    REQUIRE(mz_zip_writer_finalize_archive(&zip));
-    REQUIRE(close_zip_writer(&zip));
-}
-
-// miniz refuses to write a name starting with '/', so write a placeholder of the same length and patch it in place.
-void rename_entry(const fs::path &zip_file, const std::string &from, const std::string &to)
-{
-    REQUIRE(from.size() == to.size());
-    std::string bytes;
-    {
-        std::ifstream in(zip_file.string(), std::ios::binary);
-        bytes.assign(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>());
-    }
-    size_t count = 0;
-    for (size_t pos = bytes.find(from); pos != std::string::npos; pos = bytes.find(from, pos + to.size()), ++count)
-        bytes.replace(pos, from.size(), to);
-    // Once in the local header and once in the central directory.
-    REQUIRE(count == 2);
-    std::ofstream out(zip_file.string(), std::ios::binary | std::ios::trunc);
-    out << bytes;
-}
-
-std::vector<std::string> list_dir(const fs::path &dir)
-{
-    std::vector<std::string> names;
-    for (const fs::directory_entry &entry : fs::directory_iterator(dir))
-        names.push_back(entry.path().filename().string());
-    std::sort(names.begin(), names.end());
-    return names;
-}
-
-std::string read_file(const fs::path &file)
-{
-    std::ifstream in(file.string(), std::ios::binary);
-    return std::string(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>());
-}
-
-} // namespace
-
-TEST_CASE("Confined extraction writes a well-formed archive under the target directory", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-    write_zip(zip_file, {{"vendor/", ""}, {"vendor/machine/", ""}, {"vendor.json", "{\"a\":1}"}, {"vendor/machine/printer.json", "{\"b\":2}"}});
-
-    REQUIRE(extract_archive_confined(zip_file.string(), target.string()));
-    CHECK(fs::is_directory(target / "vendor"));
-    CHECK(read_file(target / "vendor.json") == "{\"a\":1}");
-    CHECK(read_file(target / "vendor" / "machine" / "printer.json") == "{\"b\":2}");
-}
-
-TEST_CASE("Confined extraction rejects an archive with an entry outside the target directory", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-
-    const std::string escaping_entry = GENERATE(std::string("../escape.txt"), std::string("..\\escape.txt"),
-                                                std::string("sub/../../escape.txt"), std::string("C:/escape.txt"),
-                                                std::string("C:escape.txt"), std::string("\\escape.txt"));
-    // The normal entry comes first so a per-entry check would already have written it.
-    write_zip(zip_file, {{"normal.json", "{}"}, {escaping_entry, "escaped"}});
-
-    CAPTURE(escaping_entry);
-    CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
-    CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
-    CHECK(fs::is_empty(target));
-}
-
-TEST_CASE("Confined extraction rejects an archive with an absolute entry name", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-
-    const std::string absolute    = (tmp.path() / "escape.txt").generic_string();
-    const std::string placeholder = "#" + absolute.substr(1);
-    write_zip(zip_file, {{"normal.json", "{}"}, {placeholder, "escaped"}});
-    rename_entry(zip_file, placeholder, absolute);
-
-    CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
-    CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
-    CHECK(fs::is_empty(target));
-}
-
-TEST_CASE("Confined extraction rejects a directory entry outside the target directory", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-    write_zip(zip_file, {{"vendor/", ""}, {"../outside/", ""}});
-
-    CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
-    CHECK_FALSE(fs::exists(tmp.path() / "outside"));
-    CHECK(fs::is_empty(target));
-}
-
-TEST_CASE("Confined extraction validates zero-size entries like any other", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-
-    SECTION("an empty file inside the target does not fail the archive") {
-        write_zip(zip_file, {{"empty.json", ""}, {"vendor.json", "{}"}});
-        CHECK(extract_archive_confined(zip_file.string(), target.string()));
-        CHECK(read_file(target / "vendor.json") == "{}");
-    }
-    SECTION("an empty file outside the target rejects the archive") {
-        write_zip(zip_file, {{"vendor.json", "{}"}, {"../escape.txt", ""}});
-        CHECK_FALSE(extract_archive_confined(zip_file.string(), target.string()));
-        CHECK_FALSE(fs::exists(tmp.path() / "escape.txt"));
-        CHECK(fs::is_empty(target));
-    }
-}
-
-TEST_CASE("Confined extraction writes nothing outside the target for Windows-specific name forms", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    fs::create_directories(target);
-
-    // Windows strips trailing dots and spaces and maps device names; whether these extract depends on the
-    // platform, but none of them may land beside the target.
-    const std::string name = GENERATE(std::string("name."), std::string("name "), std::string("..."), std::string(".. "),
-                                      std::string(".. /escape.txt"), std::string(".../escape.txt"), std::string("CON"),
-                                      std::string("sub/NUL.txt"), std::string("C:escape.txt"));
-    write_zip(zip_file, {{name, "payload"}});
-
-    CAPTURE(name);
-    extract_archive_confined(zip_file.string(), target.string());
-    CHECK(list_dir(tmp.path()) == std::vector<std::string>{"bundle.zip", "cache"});
-}
-
-#ifndef _WIN32
-TEST_CASE("Confined extraction replaces a symlink at the destination instead of writing through it", "[MinizExtension]")
-{
-    ScopedTemporaryDir tmp;
-    const fs::path     zip_file = tmp.path() / "bundle.zip";
-    const fs::path     target   = tmp.path() / "cache";
-    const fs::path     outside  = tmp.path() / "outside";
-    fs::create_directories(target);
-    fs::create_directories(outside);
-    write_zip(zip_file, {{"vendor.json", "{\"a\":1}"}});
-
-    SECTION("a dangling symlink") {
-        fs::create_symlink(outside / "vendor.json", target / "vendor.json");
-        CHECK(extract_archive_confined(zip_file.string(), target.string()));
-        CHECK_FALSE(fs::exists(outside / "vendor.json"));
-        CHECK_FALSE(fs::is_symlink(fs::symlink_status(target / "vendor.json")));
-        CHECK(read_file(target / "vendor.json") == "{\"a\":1}");
-    }
-    SECTION("a symlink to an existing file") {
-        { std::ofstream((outside / "vendor.json").string()) << "original"; }
-        fs::create_symlink(outside / "vendor.json", target / "vendor.json");
-        extract_archive_confined(zip_file.string(), target.string());
-        CHECK(read_file(outside / "vendor.json") == "original");
-    }
-}
-#endif

tests/libslic3r/test_utils.cpp

@@ -153,81 +153,93 @@ TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed uncha
     }
 }
 
-TEST_CASE("is_path_within_root accepts a root given with a trailing separator", "[utils]") {
-    ScopedTemporaryDir tmp;
-    const std::string root = tmp.path().string();
-    const std::string with_separator = GENERATE_COPY(root + "/", root + std::string(1, static_cast<char>(boost::filesystem::path::preferred_separator)));
-
-    CAPTURE(with_separator);
-    CHECK(is_path_within_root("vendor.json", with_separator));
-    CHECK(is_path_within_root("vendor/machine/printer.json", with_separator));
-    CHECK_FALSE(is_path_within_root("../vendor.json", with_separator));
+TEST_CASE("sanitize_file_basename keeps only a plain file name from an untrusted name", "[Utils]") {
+    const std::string unicode = "\xe6\xa8\xa1\xe5\x9e\x8b \xc3\xa9t\xc3\xa9.3mf"; // UTF-8 CJK and accented Latin
+    const auto [input, expected] = GENERATE_COPY(table<std::string, std::string>({
+        {"normal.3mf", "normal.3mf"},
+        {"../../x.3mf", "x.3mf"},
+        {"..\\..\\x.3mf", "x.3mf"},
+        {"C:\\x.3mf", "x.3mf"},
+        {"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);
+    CHECK(sanitize_file_basename(input) == expected);
 }
 
-TEST_CASE("is_path_within_root treats Windows-specific name forms the same on every platform", "[utils]") {
-    ScopedTemporaryDir tmp;
-
-    SECTION("names ending in dots or spaces stay inside the root") {
-        const std::string name = GENERATE(std::string("name."), std::string("name "), std::string("dir./file.json"), std::string("dir /file.json"));
-        CAPTURE(name);
-        CHECK(is_path_within_root(name, tmp.path()));
-    }
-    SECTION("drive-relative names are rejected") {
-        const std::string name = GENERATE(std::string("C:x"), std::string("c:x/y.json"), std::string("C:"));
-        CAPTURE(name);
-        CHECK_FALSE(is_path_within_root(name, tmp.path()));
-    }
+TEST_CASE("sanitize_file_basename rejects names that do not name a file", "[Utils]") {
+    const std::string input = GENERATE(as<std::string>{}, "", ".", "..", "../..", "dir/", "..\\", " ", ". .", "...");
+    CAPTURE(input);
+    CHECK(sanitize_file_basename(input).empty());
 }
 
-TEST_CASE("is_path_within_root rejects a name with an embedded NUL", "[utils]") {
-    ScopedTemporaryDir tmp;
-    // The filesystem calls stop at the NUL, so they would act on a different path than the one checked.
-    const std::string name = GENERATE(std::string("..\0", 3), std::string("..\0x/file.json", 14), std::string("sub/..\0x", 8),
-                                      std::string("file.json\0", 10), std::string("\0file.json", 10));
-    CAPTURE(name.size());
-    CHECK_FALSE(is_path_within_root(name, tmp.path()));
+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("is_symlink_target_within_root accepts relative targets that stay inside the root", "[utils]") {
-    ScopedTemporaryDir tmp;
-    const auto [link, target] = GENERATE(std::make_pair(std::string("Versions/Current"), std::string("A")),
-                                         std::make_pair(std::string("Foo.framework/Foo"), std::string("Versions/Current/Foo")),
-                                         std::make_pair(std::string("libfoo.so"), std::string("libfoo.so.1")),
-                                         std::make_pair(std::string("a/b/link"), std::string("c/d")));
-    CAPTURE(link, target);
-    CHECK(is_symlink_target_within_root(link, target, tmp.path()));
+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("is_symlink_target_within_root rejects absolute targets and targets that climb out", "[utils]") {
-    ScopedTemporaryDir tmp;
-    const std::string outside = (tmp.path().parent_path() / "outside").generic_string();
-    const auto [link, target] = GENERATE_COPY(std::make_pair(std::string("sub/link"), outside),
-                                              std::make_pair(std::string("sub/link"), std::string("/etc/passwd")),
-                                              std::make_pair(std::string("sub/link"), std::string("\\outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("C:/outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("C:outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("")),
-                                              std::make_pair(std::string("link"), std::string("..")),
-                                              std::make_pair(std::string("link"), std::string("../outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("../../outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("x/../../../outside")),
-                                              std::make_pair(std::string("sub/link"), std::string("..\\..\\outside")),
-                                              // symlink() stops at the NUL, so this target would be created as "..".
-                                              std::make_pair(std::string("link"), std::string("..\0", 3)));
-    CAPTURE(link, target);
-    CHECK_FALSE(is_symlink_target_within_root(link, target, tmp.path()));
+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");
 }
 
-#ifndef _WIN32
-TEST_CASE("is_symlink_target_within_root rejects a target that passes through a symlink leading out", "[utils]") {
-    ScopedTemporaryDir tmp;
-    const boost::filesystem::path root    = tmp.path() / "root";
-    const boost::filesystem::path outside = tmp.path() / "outside";
-    boost::filesystem::create_directories(root);
-    boost::filesystem::create_directories(outside);
-    boost::filesystem::create_symlink(outside, root / "out");
+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");
+}
 
-    CHECK_FALSE(is_symlink_target_within_root("link", "out/lib.so", root));
-    CHECK(is_symlink_target_within_root("link", "in/lib.so", root));
+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");
 }
-#endif