From 3668eda2b27bbf9e4f66b049f4e22b16e8482489 Mon Sep 17 00:00:00 2001 From: Florian Reimold <11774314+FlorianReimold@users.noreply.github.com> Date: Fri, 1 Mar 2024 10:37:33 +0100 Subject: [PATCH] Fixed empty-file issue (#66) - Fixed a bug that caused empty files to be not downloadable. - Added GTest that tests uploading and downloading empty files. Before, files with no content reported an error when trying to download them. Now, they are properly "downloaded", i.e. the data socket is opened and immediately a finished data transfer is reported. --- fineftp-server/src/ftp_session.cpp | 6 + fineftp-server/src/unix/file_man.cpp | 108 +++++++++--------- fineftp-server/src/unix/file_man.h | 8 +- fineftp-server/src/win32/file_man.cpp | 105 ++++++++++------- fineftp-server/src/win32/file_man.h | 8 +- fineftp-server/version.cmake | 2 +- tests/fineftp_test/src/fineftp_stresstest.cpp | 79 +++++++++++++ 7 files changed, 214 insertions(+), 102 deletions(-) diff --git a/fineftp-server/src/ftp_session.cpp b/fineftp-server/src/ftp_session.cpp index eda76dc..3be83a9 100755 --- a/fineftp-server/src/ftp_session.cpp +++ b/fineftp-server/src/ftp_session.cpp @@ -1289,6 +1289,12 @@ namespace fineftp { me->sendFtpMessage(FtpReplyCode::CLOSING_DATA_CONNECTION, "Done"); } + else if (file->data() == nullptr) + { + // Error that should never happen. If it does, it's a bug in the server. + // Usually, if the data is null, the file size should be 0. + me->sendFtpMessage(FtpReplyCode::TRANSFER_ABORTED, "Data transfer aborted: File data is null"); + } else { me->data_socket_weakptr_ = data_socket; diff --git a/fineftp-server/src/unix/file_man.cpp b/fineftp-server/src/unix/file_man.cpp index 4ae6d4c..08079e8 100644 --- a/fineftp-server/src/unix/file_man.cpp +++ b/fineftp-server/src/unix/file_man.cpp @@ -15,69 +15,73 @@ namespace fineftp { -namespace { - -std::mutex guard; -std::map> files; - -} // namespace - -ReadableFile::~ReadableFile() -{ - if (nullptr != data_) + namespace { - ::munmap(data_, size_); - } + std::mutex guard; + std::map> files; + } // namespace - const std::lock_guard lock{guard}; - if (!pth_.empty()) + ReadableFile::~ReadableFile() { - (void)files.erase(pth_); - } -} + if (nullptr != data_) + { + ::munmap(data_, size_); + } -std::shared_ptr ReadableFile::get(const std::string& pth) -{ - // See if we already have this file mapped - const std::lock_guard lock{guard}; - auto fit = files.find(pth); - if (files.end() != fit) - { - auto p = fit->second.lock(); - if (p) + const std::lock_guard lock{guard}; + if (!path_.empty()) { - return p; + (void)files.erase(path_); } } - - auto handle = ::open(pth.c_str(), O_RDONLY); - if (-1 == handle) - { - return {}; - } - struct stat st {}; - if (-1 == ::fstat(handle, &st)) + std::shared_ptr ReadableFile::get(const std::string& file_path) { - ::close(handle); - return {}; - } + // See if we already have this file mapped + const std::lock_guard lock{guard}; + auto existing_files_it = files.find(file_path); + if (files.end() != existing_files_it) + { + auto readable_file_ptr = existing_files_it->second.lock(); + if (readable_file_ptr) + { + return readable_file_ptr; + } + } - auto* map_start = ::mmap(nullptr, st.st_size, PROT_READ, MAP_SHARED, handle, 0); - if (MAP_FAILED == map_start) - { - ::close(handle); - return {}; - } + auto handle = ::open(file_path.c_str(), O_RDONLY); + if (-1 == handle) + { + return {}; + } - ::close(handle); + struct stat file_status {}; + if (-1 == ::fstat(handle, &file_status)) + { + ::close(handle); + return {}; + } - std::shared_ptr p{new ReadableFile{}}; - p->pth_ = pth; - p->size_ = st.st_size; - p->data_ = static_cast(map_start); - files[p->pth_] = p; - return p; -} + void* map_start = nullptr; + + if (file_status.st_size > 0) + { + // Only mmap file with a size > 0 + map_start = ::mmap(nullptr, file_status.st_size, PROT_READ, MAP_SHARED, handle, 0); + if (MAP_FAILED == map_start) + { + ::close(handle); + return {}; + } + } + ::close(handle); + + std::shared_ptr readable_file_ptr{new ReadableFile{}}; + readable_file_ptr->path_ = file_path; + readable_file_ptr->size_ = file_status.st_size; + readable_file_ptr->data_ = static_cast(map_start); + files[readable_file_ptr->path_] = readable_file_ptr; + return readable_file_ptr; + } } diff --git a/fineftp-server/src/unix/file_man.h b/fineftp-server/src/unix/file_man.h index f7bf93b..01a595f 100644 --- a/fineftp-server/src/unix/file_man.h +++ b/fineftp-server/src/unix/file_man.h @@ -28,10 +28,10 @@ class ReadableFile /// Retrieves the file at the specified path. /// - /// @param pth The path of the file. + /// @param file_path The path of the file. /// /// @param The requested file or nullptr if the file could not be retrieved. - static std::shared_ptr get(const std::string& pth); + static std::shared_ptr get(const std::string& file_path); /// Returns the size of the file. /// @@ -51,7 +51,7 @@ class ReadableFile private: ReadableFile() = default; - std::string pth_ = {}; + std::string path_ = {}; std::size_t size_ = {}; std::uint8_t* data_ = {}; }; @@ -118,7 +118,7 @@ inline const std::uint8_t* ReadableFile::data() const inline const std::string& ReadableFile::path() const { - return pth_; + return path_; } } diff --git a/fineftp-server/src/win32/file_man.cpp b/fineftp-server/src/win32/file_man.cpp index cded3b1..ce53c29 100644 --- a/fineftp-server/src/win32/file_man.cpp +++ b/fineftp-server/src/win32/file_man.cpp @@ -21,24 +21,26 @@ std::map> files; ReadableFile::~ReadableFile() { - if (INVALID_HANDLE_VALUE != handle_) - { + if (data_ != nullptr) ::UnmapViewOfFile(data_); + + if (map_handle_ != INVALID_HANDLE_VALUE) ::CloseHandle(map_handle_); + + if (handle_ != INVALID_HANDLE_VALUE) ::CloseHandle(handle_); - } const std::lock_guard lock{guard}; - if (!pth_.empty()) + if (!path_.empty()) { - (void)files.erase(pth_); + (void)files.erase(path_); } } -std::shared_ptr ReadableFile::get(const Str& pth) +std::shared_ptr ReadableFile::get(const Str& file_path) { std::basic_ostringstream os; - for (auto c : pth) + for (auto c : file_path) { if (c == '/') { @@ -50,62 +52,83 @@ std::shared_ptr ReadableFile::get(const Str& pth) } } - auto&& s = os.str(); + auto&& file_path_fixed_separators = os.str(); // See if we already have this file mapped const std::lock_guard lock{guard}; - auto fit = files.find(s); - if (files.end() != fit) + auto existing_files_it = files.find(file_path_fixed_separators); + if (files.end() != existing_files_it) { - auto p = fit->second.lock(); - if (p) + auto readable_file_ptr = existing_files_it->second.lock(); + if (readable_file_ptr) { - return p; + return readable_file_ptr; } } #if !defined(__GNUG__) - HANDLE handle = - ::CreateFileW(s.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + HANDLE file_handle = + ::CreateFileW(file_path_fixed_separators.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); #else - auto handle = - ::CreateFileA(s.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + auto file_handle = + ::CreateFileA(file_path_fixed_separators.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); #endif - if (INVALID_HANDLE_VALUE == handle) + if (INVALID_HANDLE_VALUE == file_handle) { return {}; } - LARGE_INTEGER sz; - if (0 == ::GetFileSizeEx(handle, &sz)) + // Get the file size by Using GetFileInformationByHandle + BY_HANDLE_FILE_INFORMATION file_info; + if (0 == ::GetFileInformationByHandle(file_handle, &file_info)) { - ::CloseHandle(handle); + ::CloseHandle(file_handle); return {}; } - - auto* map_handle = ::CreateFileMapping(handle, nullptr, PAGE_READONLY, sz.HighPart, sz.LowPart, nullptr); - if ((map_handle == INVALID_HANDLE_VALUE) || (map_handle == nullptr)) - { - ::CloseHandle(handle); - return {}; + LARGE_INTEGER file_size; + file_size.LowPart = file_info.nFileSizeLow; + file_size.HighPart = file_info.nFileSizeHigh; + + // Create new ReadableFile ptr + std::shared_ptr readable_file_ptr(new ReadableFile{}); + + if (file_size.QuadPart == 0) + { + // Handle zero-size files + readable_file_ptr->path_ = std::move(file_path_fixed_separators); + readable_file_ptr->size_ = file_size.QuadPart; + readable_file_ptr->data_ = static_cast(nullptr); + readable_file_ptr->handle_ = file_handle; + readable_file_ptr->map_handle_ = INVALID_HANDLE_VALUE; } - - auto* map_start = ::MapViewOfFile(map_handle, FILE_MAP_READ, 0, 0, sz.QuadPart); - if (nullptr == map_start) + else { - ::CloseHandle(map_handle); - ::CloseHandle(handle); - return {}; + // Handle non-zero-size files + auto* map_handle = ::CreateFileMapping(file_handle, nullptr, PAGE_READONLY, file_size.HighPart, file_size.LowPart, nullptr); + if ((map_handle == INVALID_HANDLE_VALUE) || (map_handle == nullptr)) + { + ::CloseHandle(file_handle); + return {}; + } + + auto* map_start = ::MapViewOfFile(map_handle, FILE_MAP_READ, 0, 0, file_size.QuadPart); + if (nullptr == map_start) + { + ::CloseHandle(map_handle); + ::CloseHandle(file_handle); + return {}; + } + + readable_file_ptr->path_ = std::move(file_path_fixed_separators); + readable_file_ptr->size_ = file_size.QuadPart; + readable_file_ptr->data_ = static_cast(map_start); + readable_file_ptr->handle_ = file_handle; + readable_file_ptr->map_handle_ = map_handle; } - std::shared_ptr p{new ReadableFile{}}; - p->pth_ = std::move(s); - p->size_ = sz.QuadPart; - p->data_ = static_cast(map_start); - p->handle_ = handle; - p->map_handle_ = map_handle; - files[p->pth_] = p; - return p; + // Add readable_file_ptr to the map and return it to the user + files[readable_file_ptr->path_] = readable_file_ptr; + return readable_file_ptr; } WriteableFile::WriteableFile(const std::string& filename, std::ios::openmode mode) diff --git a/fineftp-server/src/win32/file_man.h b/fineftp-server/src/win32/file_man.h index 7b8ad12..8fc12f3 100644 --- a/fineftp-server/src/win32/file_man.h +++ b/fineftp-server/src/win32/file_man.h @@ -33,10 +33,10 @@ class ReadableFile /// Retrieves the file at the specified path. /// - /// @param pth The path of the file. + /// @param file_path The path of the file. /// /// @param The requested file or nullptr if the file could not be retrieved. - static std::shared_ptr get(const Str& pth); + static std::shared_ptr get(const Str& file_path); /// Returns the size of the file. /// @@ -56,7 +56,7 @@ class ReadableFile private: ReadableFile() = default; - Str pth_ = {}; + Str path_ = {}; std::size_t size_ = {}; std::uint8_t* data_ = {}; HANDLE handle_ = INVALID_HANDLE_VALUE; @@ -105,7 +105,7 @@ inline const std::uint8_t* ReadableFile::data() const inline const ReadableFile::Str& ReadableFile::path() const { - return pth_; + return path_; } inline bool WriteableFile::good() const diff --git a/fineftp-server/version.cmake b/fineftp-server/version.cmake index 7c441ad..730153e 100644 --- a/fineftp-server/version.cmake +++ b/fineftp-server/version.cmake @@ -1,3 +1,3 @@ set(FINEFTP_SERVER_VERSION_MAJOR 1) set(FINEFTP_SERVER_VERSION_MINOR 4) -set(FINEFTP_SERVER_VERSION_PATCH 1) +set(FINEFTP_SERVER_VERSION_PATCH 2) diff --git a/tests/fineftp_test/src/fineftp_stresstest.cpp b/tests/fineftp_test/src/fineftp_stresstest.cpp index 471f105..75087a9 100644 --- a/tests/fineftp_test/src/fineftp_stresstest.cpp +++ b/tests/fineftp_test/src/fineftp_stresstest.cpp @@ -101,6 +101,85 @@ TEST(FineFTPTest, SimpleUploadDownload) { } #endif +#if 1 +TEST(FineFTPTest, EmptyFile) +{ + const auto test_working_dir = std::filesystem::current_path(); + const auto ftp_root_dir = test_working_dir / "ftp_root"; + const auto local_root_dir = test_working_dir / "local_root"; + + if (std::filesystem::exists(ftp_root_dir)) + std::filesystem::remove_all(ftp_root_dir); + + if (std::filesystem::exists(local_root_dir)) + std::filesystem::remove_all(local_root_dir); + + // Make sure that we start clean, so no old dir exists + ASSERT_FALSE(std::filesystem::exists(ftp_root_dir)); + ASSERT_FALSE(std::filesystem::exists(local_root_dir)); + + std::filesystem::create_directory(ftp_root_dir); + std::filesystem::create_directory(local_root_dir); + + // Make sure that we were able to create the dir + ASSERT_TRUE(std::filesystem::is_directory(ftp_root_dir)); + ASSERT_TRUE(std::filesystem::is_directory(local_root_dir)); + + fineftp::FtpServer server(2121); + server.start(1); + + server.addUserAnonymous(ftp_root_dir.string(), fineftp::Permission::All); + + // Create an empty file in the local root dir + auto local_file = local_root_dir / "empty_file.txt"; + std::ofstream ofs(local_file.string()); + ofs.close(); + + // Make sure that the file exists + ASSERT_TRUE(std::filesystem::exists(local_file)); + ASSERT_TRUE(std::filesystem::is_regular_file(local_file)); + + // Upload the file to the FTP server using curl + { + const std::string curl_command = "curl -S -s -T \"" + local_file.string() + "\" \"ftp://localhost:2121/\""; + const auto curl_result = std::system(curl_command.c_str()); + + // Make sure that the upload was successful + ASSERT_EQ(curl_result, 0); + + // Make sure that the file exists in the FTP root dir + auto ftp_file = ftp_root_dir / "empty_file.txt"; + ASSERT_TRUE(std::filesystem::exists(ftp_file)); + ASSERT_TRUE(std::filesystem::is_regular_file(ftp_file)); + + // Make sure that the file is empty + std::ifstream ifs(ftp_file.string()); + const std::string content((std::istreambuf_iterator(ifs)), (std::istreambuf_iterator())); + ASSERT_TRUE(content.empty()); + } + + // Download the file again + { + const std::string curl_command_download = "curl -S -s -o \"" + local_root_dir.string() + "/empty_file_download.txt\" \"ftp://localhost:2121/empty_file.txt\""; + const auto curl_result = std::system(curl_command_download.c_str()); + ASSERT_EQ(curl_result, 0); + + // Make sure that the files are identical + auto local_file_download = local_root_dir / "empty_file_download.txt"; + + ASSERT_TRUE(std::filesystem::exists(local_file_download)); + ASSERT_TRUE(std::filesystem::is_regular_file(local_file_download)); + + std::ifstream ifs(local_file_download.string()); + const std::string content((std::istreambuf_iterator(ifs)), (std::istreambuf_iterator())); + ASSERT_TRUE(content.empty()); + } + + + // Stop the server +} +#endif + #if 1 TEST(FineFTPTest, BigFilesMultipleClients) {