Skip to content

Commit

Permalink
Fixed empty-file issue (#66)
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@FlorianReimold
FlorianReimold committed Mar 1, 2024
1 parent eea7bcd commit 3668eda
Show file tree
Hide file tree
Showing 7 changed files with 214 additions and 102 deletions.
6 changes: 6 additions & 0 deletions fineftp-server/src/ftp_session.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
108 changes: 56 additions & 52 deletions fineftp-server/src/unix/file_man.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,69 +15,73 @@
namespace fineftp
{

namespace {

std::mutex guard;
std::map<std::string, std::weak_ptr<ReadableFile>> files;

} // namespace

ReadableFile::~ReadableFile()
{
if (nullptr != data_)
namespace
{
::munmap(data_, size_);
}
std::mutex guard;
std::map<std::string, std::weak_ptr<ReadableFile>> files;
} // namespace

const std::lock_guard<std::mutex> lock{guard};
if (!pth_.empty())
ReadableFile::~ReadableFile()
{
(void)files.erase(pth_);
}
}
if (nullptr != data_)
{
::munmap(data_, size_);
}

std::shared_ptr<ReadableFile> ReadableFile::get(const std::string& pth)
{
// See if we already have this file mapped
const std::lock_guard<std::mutex> lock{guard};
auto fit = files.find(pth);
if (files.end() != fit)
{
auto p = fit->second.lock();
if (p)
const std::lock_guard<std::mutex> 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> ReadableFile::get(const std::string& file_path)
{
::close(handle);
return {};
}
// See if we already have this file mapped
const std::lock_guard<std::mutex> 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<ReadableFile> p{new ReadableFile{}};
p->pth_ = pth;
p->size_ = st.st_size;
p->data_ = static_cast<uint8_t*>(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<ReadableFile> 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<uint8_t*>(map_start);
files[readable_file_ptr->path_] = readable_file_ptr;
return readable_file_ptr;
}
}
8 changes: 4 additions & 4 deletions fineftp-server/src/unix/file_man.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ReadableFile> get(const std::string& pth);
static std::shared_ptr<ReadableFile> get(const std::string& file_path);

/// Returns the size of the file.
///
Expand All @@ -51,7 +51,7 @@ class ReadableFile
private:
ReadableFile() = default;

std::string pth_ = {};
std::string path_ = {};
std::size_t size_ = {};
std::uint8_t* data_ = {};
};
Expand Down Expand Up @@ -118,7 +118,7 @@ inline const std::uint8_t* ReadableFile::data() const

inline const std::string& ReadableFile::path() const
{
return pth_;
return path_;
}

}
Expand Down
105 changes: 64 additions & 41 deletions fineftp-server/src/win32/file_man.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,26 @@ std::map<ReadableFile::Str, std::weak_ptr<ReadableFile>> 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<std::mutex> lock{guard};
if (!pth_.empty())
if (!path_.empty())
{
(void)files.erase(pth_);
(void)files.erase(path_);
}
}

std::shared_ptr<ReadableFile> ReadableFile::get(const Str& pth)
std::shared_ptr<ReadableFile> ReadableFile::get(const Str& file_path)
{
std::basic_ostringstream<Str::value_type> os;
for (auto c : pth)
for (auto c : file_path)
{
if (c == '/')
{
Expand All @@ -50,62 +52,83 @@ std::shared_ptr<ReadableFile> 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<std::mutex> 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<ReadableFile> 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<uint8_t*>(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<uint8_t*>(map_start);
readable_file_ptr->handle_ = file_handle;
readable_file_ptr->map_handle_ = map_handle;
}

std::shared_ptr<ReadableFile> p{new ReadableFile{}};
p->pth_ = std::move(s);
p->size_ = sz.QuadPart;
p->data_ = static_cast<uint8_t*>(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)
Expand Down
8 changes: 4 additions & 4 deletions fineftp-server/src/win32/file_man.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ReadableFile> get(const Str& pth);
static std::shared_ptr<ReadableFile> get(const Str& file_path);

/// Returns the size of the file.
///
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion fineftp-server/version.cmake
View file
Original file line number Diff line number Diff line change
@@ -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)
Loading

0 comments on commit 3668eda

Please sign in to comment.