File::create() with pRetrieveContents=false makes the File's destructor delete "/some/path/file"

VERSION.txt

@@ -1 +1 @@
-1.20261009.4
+1.20261009.5

src/api/libopencor/file.h

@@ -111,7 +111,8 @@ class LIBOPENCOR_EXPORT File: public Logger
     /**
      * @brief Return the file name.
      *
-     * Return the file name. If the file is remote then we return the file name of its local copy.
+     * Return the file name. If the file is remote then we return the file name of its local copy or an empty string if
+     * it has no local copy (i.e. its contents were not retrieved or could not be downloaded).
      *
      * @sa url()
      * @sa path()

src/file/file.cpp

@@ -36,9 +36,11 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents)
     }
 
 #ifndef __EMSCRIPTEN__
-    // Download a local copy of the remote file, if needed and requested.
+    // Download a local copy of the remote file, if requested.
+    // Note: a remote file that is not downloaded (or that cannot be downloaded) has no local copy and therefore no file
+    //       path.
 
-    if (mFilePath.empty()) {
+    if (!mUrl.empty()) {
         if (pRetrieveContents) {
             auto [res, filePath] {downloadFile(mUrl)};
 
@@ -50,8 +52,6 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents)
 
                 addError("The file could not be downloaded.");
             }
-        } else {
-            mFilePath = stringToPath("/some/path/file");
         }
     } else if (pRetrieveContents) {
         // Note: we use the std::error_code version of std::filesystem::exists() so that nothing gets thrown (e.g., if
@@ -65,10 +65,6 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents)
             addError("The file does not exist.");
         }
     }
-#else
-    if (mFilePath.empty()) {
-        mFilePath = stringToPath("/some/path/file");
-    }
 #endif
 
     mFileName = pathToString(mFilePath);
@@ -77,7 +73,8 @@ File::Impl::Impl(const std::string &pFileNameOrUrl, bool pRetrieveContents)
 File::Impl::~Impl()
 {
     // Delete the local copy of a remote file, if we downloaded it.
-    // Note #1: a remote file that we didn't download has a dummy file path, which must obviously not be deleted.
+    // Note #1: we must only ever delete a file that we downloaded ourselves, never a file that just happens to have the
+    //          same file path.
     // Note #2: we use the std::error_code version of std::filesystem::remove() so that nothing gets thrown since we are
     //          in a destructor.
 
@@ -90,12 +87,19 @@ File::Impl::~Impl()
 
 void File::Impl::checkType(const FilePtr &pOwner, bool pResetType)
 {
-    // Reset he type of the file, if needed.
+    // Reset the type of the file, if needed.
+    // Note: we also release our previous CellML file, SED-ML file, or COMBINE archive, if any, so that we don't keep
+    //       using it (and, for a COMBINE archive, keep its child files managed) if our new contents are of a different
+    //       type or are not recognised.
 
     if (pResetType) {
         removeAllIssues();
 
         mType = Type::UNKNOWN_FILE;
+
+        mCellmlFile = nullptr;
+        mSedmlFile = nullptr;
+        mCombineArchive = nullptr;
 #ifndef __EMSCRIPTEN__
     } else if (mType == Type::IRRETRIEVABLE_FILE) {
         return;

src/file/filemanager.cpp

@@ -51,7 +51,10 @@ FilePtr FileManager::Impl::managedFile(bool pIsLocalFile, const std::string &pFi
     std::erase(pLockedFiles, nullptr);
 
     for (const auto &lockedFile : pLockedFiles) {
-        if (pIsLocalFile ? lockedFile->fileName() == pFileNameOrUrl : lockedFile->url() == pFileNameOrUrl) {
+        // Note: a local file must only ever match a local file, not the local copy of a remote file (or a remote file
+        //       that has no local copy, i.e. an empty file name).
+
+        if (pIsLocalFile ? lockedFile->url().empty() && (lockedFile->fileName() == pFileNameOrUrl) : lockedFile->url() == pFileNameOrUrl) {
             return lockedFile;
         }
     }

src/misc/utils.cpp

@@ -35,6 +35,7 @@ limitations under the License.
 #include <format>
 #include <fstream>
 #include <iostream>
+#include <mutex>
 #include <regex>
 #include <sstream>
 #include <vector>
@@ -470,16 +471,20 @@ std::filesystem::path uniqueFilePath()
     static const std::string LETTERS {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"};
     static const uint64_t LETTERS_SIZE {LETTERS.size()};
 
-    static auto testFile {pathToString(std::filesystem::temp_directory_path() / "libOpenCOR_XXXXXX.tmp")};
+    // Note: our caller must ensure that only one thread at a time calls us and creates the file (see downloadFile()),
+    //       so that two threads cannot end up with the same file path.
 
-    static const size_t XXXXXX_POS {testFile.size() - 6 - 4};
+    static const auto TEST_FILE {pathToString(std::filesystem::temp_directory_path() / "libOpenCOR_XXXXXX.tmp")};
+
+    static const size_t XXXXXX_POS {TEST_FILE.size() - 6 - 4};
     static constexpr uint64_t MICROSECONDS_SHIFT {16U};
     static constexpr uint64_t PID_SHIFT {32U};
 #    ifndef CODE_COVERAGE_ENABLED
     static constexpr uint64_t VALUE_SHIFT {7777U};
 #    endif
     static constexpr uint64_t XXXXXX_POS_SHIFT {6U};
 
+    auto testFile {TEST_FILE};
     TimeVal timeVal;
 
     getTimeOfDay(timeVal);
@@ -555,8 +560,23 @@ std::tuple<bool, std::filesystem::path> downloadFile(const std::string &pUrl)
 {
     static const std::tuple<bool, std::filesystem::path> NO_TUPLE {false, std::filesystem::path()};
 
-    auto filePath {uniqueFilePath()};
-    std::ofstream file(filePath, std::ios_base::binary);
+    // Create a file with a unique file path.
+    // Note: we must generate the file path and create the file in one go, so that another thread cannot generate the
+    //       same file path in between (in which case both threads would download to the same file and the local copy
+    //       of one remote file would get deleted with the other remote file).
+
+    static std::mutex uniqueFilePathMutex;
+
+    std::filesystem::path filePath;
+    std::ofstream file;
+
+    {
+        const std::scoped_lock<std::mutex> lock(uniqueFilePathMutex);
+
+        filePath = uniqueFilePath();
+
+        file.open(filePath, std::ios_base::binary);
+    }
 
 #    ifndef CODE_COVERAGE_ENABLED
     if (!file.is_open()) {

src/support/combine/combinearchive.cpp

@@ -22,6 +22,8 @@ limitations under the License.
 #include "combine/combinearchive.h"
 #include "omex/CaContent.h"
 
+#include <atomic>
+#include <filesystem>
 #include <regex>
 
 namespace libOpenCOR {
@@ -44,12 +46,37 @@ bool isSedmlFormat(const std::string &pFormat)
     return pFormat == SEDML_FORMAT;
 }
 
+std::string archiveLocation(const FilePtr &pFile)
+{
+    // Determine the location of the COMBINE archive, i.e. the location where its files are "virtually" extracted (e.g.,
+    // "/some/path/archive.omex.contents/").
+    // Note #1: the files of a COMBINE archive are managed by the file manager using their file name, so each COMBINE
+    //          archive must have its own location. This means that a remote COMBINE archive that has no local copy
+    //          (i.e. it was not downloaded), and therefore no file name, must use a virtual file name that is unique to
+    //          it.
+    // Note #2: we canonicalise the location since the files of the COMBINE archive have a canonical file name (see
+    //          File::create()), which we need to be able to compare against (see CombineArchive::Impl::file()). For
+    //          instance, the temporary directory on macOS is a symbolic link (i.e. "/var/..." vs. "/private/var/...").
+
+    static std::atomic<uint64_t> virtualFileNameCounter {0};
+
+    auto fileName {pFile->fileName()};
+
+    if (fileName.empty()) {
+        std::error_code errorCode;
+
+        fileName = pathToString(std::filesystem::temp_directory_path(errorCode) / ("libOpenCOR_" + std::to_string(++virtualFileNameCounter) + ".virtual"));
+    }
+
+    return pathToString(stringToPath(canonicalFileName(fileName + ".contents") + "/"));
+}
+
 } // namespace
 
 CombineArchive::Impl::Impl(const FilePtr &pFile, libcombine::CombineArchive *pArchive, UnsignedChars &&pArchiveContents)
     : mArchiveContents(std::move(pArchiveContents))
     , mArchive(pArchive)
-    , mArchiveLocation(libOpenCOR::pathToString(libOpenCOR::stringToPath(pFile->fileName() + ".contents/")))
+    , mArchiveLocation(archiveLocation(pFile))
     , mArchiveLocationSize(mArchiveLocation.size())
 {
     // Extract all the files contained in the COMBINE archive.
@@ -65,7 +92,32 @@ CombineArchive::Impl::Impl(const FilePtr &pFile, libcombine::CombineArchive *pAr
     for (int i {0}; i < mArchive->getNumEntries(); ++i) {
         const auto *entry {mArchive->getEntry(i)};
         auto location {entry->getLocation()};
-        auto file {File::create(mArchiveLocation + location)};
+
+        // Make sure that the file is located within the COMBINE archive.
+        // Note: a location may have some ".." segments (e.g., "../file.cellml"), possibly percent-encoded (e.g.,
+        //       "%2E%2E/file.cellml"), in which case the file would be located outside of the COMBINE archive. It
+        //       could then be confused with another managed file (which contents would get replaced) or even with the
+        //       COMBINE archive itself (resulting in an infinite recursion), so we ignore such a file. To determine
+        //       where the file is located, we use its file name as determined by File::create().
+
+        auto fileNameOrUrl {mArchiveLocation + location};
+        auto [isLocalFile, fileName] {retrieveFileInfo(decodeUrl(fileNameOrUrl))};
+
+        if (!fileName.starts_with(mArchiveLocation)) {
+            std::string warning;
+
+            warning.reserve(location.size() + 64); // NOLINT
+
+            warning += "The file '";
+            warning += location;
+            warning += "' is not located within the COMBINE archive. It has been ignored.";
+
+            addWarning(warning);
+
+            continue;
+        }
+
+        auto file {File::create(fileNameOrUrl)};
 
         file->setContents(mArchive->extractEntryToBuffer(location));
 

tests/api/file/basictests.cpp

@@ -22,6 +22,7 @@ limitations under the License.
 #include <filesystem>
 #include <fstream>
 #include <libopencor>
+#include <set>
 #include <thread>
 
 namespace {
@@ -170,6 +171,45 @@ TEST(BasicFileTest, remoteFile)
     EXPECT_FALSE(file->contents().empty());
 }
 
+TEST(BasicFileTest, remoteFileLocalCopy)
+{
+    // The local copy of a remote file must exist for as long as the remote file exists, and be deleted with it.
+
+    std::string fileName;
+
+    {
+        auto file {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE)};
+
+        fileName = file->fileName();
+
+        EXPECT_TRUE(std::filesystem::exists(fileName));
+    }
+
+    EXPECT_FALSE(std::filesystem::exists(fileName));
+}
+
+TEST(BasicFileTest, localFileMatchingLocalCopyOfRemoteFile)
+{
+    // A local file must never be confused with the local copy of a remote file, and deleting it must not delete that
+    // local copy (since it is not a file that we downloaded).
+
+    auto remoteFile {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE)};
+    const auto &fileName {remoteFile->fileName()};
+
+    {
+        auto localFile {libOpenCOR::File::create(fileName)};
+
+        EXPECT_NE(localFile, remoteFile);
+        EXPECT_EQ(localFile->fileName(), libOpenCOR::canonicalFileName(fileName));
+        EXPECT_EQ(localFile->url(), "");
+        EXPECT_EQ(localFile->contents(), remoteFile->contents());
+        EXPECT_EQ(libOpenCOR::FileManager::instance().fileCount(), 2U);
+    }
+
+    EXPECT_TRUE(std::filesystem::exists(fileName));
+    EXPECT_EQ(libOpenCOR::FileManager::instance().file(fileName), nullptr);
+}
+
 TEST(BasicFileTest, encodedRemoteFile)
 {
     auto file {libOpenCOR::File::create("https://models.physiomeproject.org/workspace/aed/@@rawfile/d4accf8429dbf5bdd5dfa1719790f361f5baddbe/FAIRDO%20BG%20example%203.1.cellml")};
@@ -252,6 +292,46 @@ TEST(BasicFileTest, concurrentFileCreations)
     EXPECT_EQ(std::filesystem::current_path(), origDir);
 }
 
+TEST(BasicFileTest, concurrentFileDownloads)
+{
+    // Remote files that are downloaded at the same time must each have their own local copy, so that deleting the local
+    // copy of one of them doesn't delete the local copy of another one.
+
+    static constexpr size_t THREAD_COUNT {8};
+
+    std::vector<libOpenCOR::FilePtr> files(THREAD_COUNT);
+    std::vector<std::thread> threads;
+
+    threads.reserve(THREAD_COUNT);
+
+    for (size_t i {0}; i < THREAD_COUNT; ++i) {
+        threads.emplace_back([&files, i] {
+            files[i] = libOpenCOR::File::create(std::string(libOpenCOR::REMOTE_FILE) + "?thread=" + std::to_string(i));
+        });
+    }
+
+    for (auto &thread : threads) {
+        thread.join();
+    }
+
+    std::set<std::string> fileNames;
+
+    for (const auto &file : files) {
+        EXPECT_EQ(file->type(), libOpenCOR::File::Type::CELLML_FILE);
+        EXPECT_EQ(file->contents(), files[0]->contents());
+
+        fileNames.insert(file->fileName());
+    }
+
+    EXPECT_EQ(fileNames.size(), THREAD_COUNT);
+
+    files.clear();
+
+    for (const auto &fileName : fileNames) {
+        EXPECT_FALSE(std::filesystem::exists(fileName));
+    }
+}
+
 TEST(BasicFileTest, localVirtualFile)
 {
     auto filePath {libOpenCOR::resourcePath("unknown_file.txt")};
@@ -278,11 +358,7 @@ TEST(BasicFileTest, remoteVirtualFile)
     auto file {libOpenCOR::File::create("https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt", false)};
 
     EXPECT_EQ(file->type(), libOpenCOR::File::Type::UNKNOWN_FILE);
-#ifdef BUILDING_USING_MSVC
-    EXPECT_EQ(file->fileName(), "\\some\\path\\file");
-#else
-    EXPECT_EQ(file->fileName(), "/some/path/file");
-#endif
+    EXPECT_EQ(file->fileName(), "");
     EXPECT_EQ(file->url(), "https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt");
     EXPECT_EQ(file->path(), "https://raw.githubusercontent.com/opencor/libopencor/master/tests/res/unknown_file.txt");
     EXPECT_TRUE(file->contents().empty());
@@ -298,6 +374,22 @@ TEST(BasicFileTest, remoteVirtualFile)
     EXPECT_EQ_ISSUES(file, expectedUnknownFileIssues());
 }
 
+TEST(BasicFileTest, remoteVirtualFileMatchingLocalFile)
+{
+    // A remote file that has no local copy has an empty file name, so it must never be confused with a local file that
+    // has an empty file name.
+
+    auto remoteFile {libOpenCOR::File::create(libOpenCOR::REMOTE_FILE, false)};
+    auto localFile {libOpenCOR::File::create("", false)};
+
+    EXPECT_NE(localFile, remoteFile);
+    EXPECT_EQ(remoteFile->fileName(), "");
+    EXPECT_EQ(localFile->fileName(), "");
+    EXPECT_EQ(localFile->url(), "");
+    EXPECT_EQ(libOpenCOR::FileManager::instance().file(""), localFile);
+    EXPECT_EQ(libOpenCOR::FileManager::instance().file(libOpenCOR::REMOTE_FILE), remoteFile);
+}
+
 TEST(BasicFileTest, fileManager)
 {
     auto &fileManager {libOpenCOR::FileManager::instance()};