diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h index 3bc541286362c..d654026e41266 100644 --- a/include/envoy/common/platform.h +++ b/include/envoy/common/platform.h @@ -61,6 +61,7 @@ typedef ptrdiff_t ssize_t; typedef uint32_t mode_t; typedef SOCKET os_fd_t; +typedef HANDLE filesystem_os_id_t; // NOLINT(modernize-use-using) typedef unsigned int sa_family_t; @@ -116,6 +117,7 @@ struct msghdr { #define IPV6_RECVPKTINFO IPV6_PKTINFO #endif +#define INVALID_HANDLE INVALID_HANDLE_VALUE #define SOCKET_VALID(sock) ((sock) != INVALID_SOCKET) #define SOCKET_INVALID(sock) ((sock) == INVALID_SOCKET) #define SOCKET_FAILURE(rc) ((rc) == SOCKET_ERROR) @@ -143,6 +145,9 @@ struct msghdr { #define SOCKET_ERROR_INVAL WSAEINVAL #define SOCKET_ERROR_ADDR_IN_USE WSAEADDRINUSE +#define HANDLE_ERROR_PERM ERROR_ACCESS_DENIED +#define HANDLE_ERROR_INVALID ERROR_INVALID_HANDLE + namespace Platform { constexpr absl::string_view null_device_path{"NUL"}; } @@ -206,7 +211,9 @@ constexpr absl::string_view null_device_path{"NUL"}; #endif typedef int os_fd_t; +typedef int filesystem_os_id_t; // NOLINT(modernize-use-using) +#define INVALID_HANDLE -1 #define INVALID_SOCKET -1 #define SOCKET_VALID(sock) ((sock) >= 0) #define SOCKET_INVALID(sock) ((sock) == -1) @@ -231,6 +238,10 @@ typedef int os_fd_t; #define SOCKET_ERROR_INVAL EINVAL #define SOCKET_ERROR_ADDR_IN_USE EADDRINUSE +// Mapping POSIX file errors to common error names +#define HANDLE_ERROR_PERM EACCES +#define HANDLE_ERROR_INVALID EBADF + namespace Platform { constexpr absl::string_view null_device_path{"/dev/null"}; } diff --git a/source/common/filesystem/file_shared_impl.cc b/source/common/filesystem/file_shared_impl.cc index 56601badb01c9..5c76ea6c29de2 100644 --- a/source/common/filesystem/file_shared_impl.cc +++ b/source/common/filesystem/file_shared_impl.cc @@ -1,43 +1,25 @@ #include "common/filesystem/file_shared_impl.h" -#include +#include "common/common/utility.h" namespace Envoy { namespace Filesystem { -Api::IoError::IoErrorCode IoFileError::getErrorCode() const { return IoErrorCode::UnknownError; } - -std::string IoFileError::getErrorDetails() const { - // TODO(sunjayBhatia, wrowe): Disable clang-format until win32 implementation no longer uses POSIX - // subsystem, see https://github.com/envoyproxy/envoy/issues/11655 - // clang-format off - return ::strerror(errno_); - // clang-format on -} - -Api::IoCallBoolResult FileSharedImpl::open(FlagSet in) { - if (isOpen()) { - return resultSuccess(true); +Api::IoError::IoErrorCode IoFileError::getErrorCode() const { + switch (errno_) { + case HANDLE_ERROR_PERM: + return IoErrorCode::Permission; + case HANDLE_ERROR_INVALID: + return IoErrorCode::BadFd; + default: + ENVOY_LOG_MISC(debug, "Unknown error code {} details {}", errno_, getErrorDetails()); + return IoErrorCode::UnknownError; } - - openFile(in); - return fd_ != -1 ? resultSuccess(true) : resultFailure(false, errno); } -Api::IoCallSizeResult FileSharedImpl::write(absl::string_view buffer) { - const ssize_t rc = writeFile(buffer); - return rc != -1 ? resultSuccess(rc) : resultFailure(rc, errno); -}; - -Api::IoCallBoolResult FileSharedImpl::close() { - ASSERT(isOpen()); - - bool success = closeFile(); - fd_ = -1; - return success ? resultSuccess(true) : resultFailure(false, errno); -} +std::string IoFileError::getErrorDetails() const { return errorDetails(errno_); } -bool FileSharedImpl::isOpen() const { return fd_ != -1; }; +bool FileSharedImpl::isOpen() const { return fd_ != INVALID_HANDLE; }; std::string FileSharedImpl::path() const { return path_; }; diff --git a/source/common/filesystem/file_shared_impl.h b/source/common/filesystem/file_shared_impl.h index 06e166661287e..4b8b623f7fdbb 100644 --- a/source/common/filesystem/file_shared_impl.h +++ b/source/common/filesystem/file_shared_impl.h @@ -37,23 +37,15 @@ template Api::IoCallResult resultSuccess(T result) { class FileSharedImpl : public File { public: - FileSharedImpl(std::string path) : fd_(-1), path_(std::move(path)) {} + FileSharedImpl(std::string path) : fd_(INVALID_HANDLE), path_(std::move(path)) {} ~FileSharedImpl() override = default; - // Filesystem::File - Api::IoCallBoolResult open(FlagSet flag) override; - Api::IoCallSizeResult write(absl::string_view buffer) override; - Api::IoCallBoolResult close() override; bool isOpen() const override; std::string path() const override; protected: - virtual void openFile(FlagSet in) PURE; - virtual ssize_t writeFile(absl::string_view buffer) PURE; - virtual bool closeFile() PURE; - - int fd_; + filesystem_os_id_t fd_; const std::string path_; }; diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index e24814d0ca700..5f69e98e764bf 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -30,13 +30,26 @@ FileImplPosix::~FileImplPosix() { } } -void FileImplPosix::openFile(FlagSet in) { +Api::IoCallBoolResult FileImplPosix::open(FlagSet in) { + if (isOpen()) { + return resultSuccess(true); + } + const auto flags_and_mode = translateFlag(in); fd_ = ::open(path_.c_str(), flags_and_mode.flags_, flags_and_mode.mode_); + return fd_ != -1 ? resultSuccess(true) : resultFailure(false, errno); } -ssize_t FileImplPosix::writeFile(absl::string_view buffer) { - return ::write(fd_, buffer.data(), buffer.size()); +Api::IoCallSizeResult FileImplPosix::write(absl::string_view buffer) { + const ssize_t rc = ::write(fd_, buffer.data(), buffer.size()); + return rc != -1 ? resultSuccess(rc) : resultFailure(rc, errno); +}; + +Api::IoCallBoolResult FileImplPosix::close() { + ASSERT(isOpen()); + int rc = ::close(fd_); + fd_ = -1; + return (rc != -1) ? resultSuccess(true) : resultFailure(false, errno); } FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) { @@ -62,8 +75,6 @@ FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) { return {out, mode}; } -bool FileImplPosix::closeFile() { return ::close(fd_) != -1; } - FilePtr InstanceImplPosix::createFile(const std::string& path) { return std::make_unique(path); } diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index 173be8918d339..4f81255be2225 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -21,13 +21,12 @@ class FileImplPosix : public FileSharedImpl { mode_t mode_ = 0; }; - // Filesystem::FileSharedImpl - FlagsAndMode translateFlag(FlagSet in); - void openFile(FlagSet flags) override; - ssize_t writeFile(absl::string_view buffer) override; - bool closeFile() override; + Api::IoCallBoolResult open(FlagSet flag) override; + Api::IoCallSizeResult write(absl::string_view buffer) override; + Api::IoCallBoolResult close() override; private: + FlagsAndMode translateFlag(FlagSet in); friend class FileSystemImplTest; }; diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 268408e2dd670..cfdb3098fe1e2 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -26,40 +26,67 @@ FileImplWin32::~FileImplWin32() { } } -void FileImplWin32::openFile(FlagSet in) { - const auto flags_and_mode = translateFlag(in); - fd_ = ::open(path_.c_str(), flags_and_mode.flags_, flags_and_mode.pmode_); +Api::IoCallBoolResult FileImplWin32::open(FlagSet in) { + if (isOpen()) { + return resultSuccess(true); + } + + auto flags = translateFlag(in); + fd_ = CreateFileA(path_.c_str(), flags.access_, FILE_SHARE_READ | FILE_SHARE_WRITE, 0, + flags.creation_, 0, NULL); + if (fd_ == INVALID_HANDLE) { + return resultFailure(false, ::GetLastError()); + } + return resultSuccess(true); } -ssize_t FileImplWin32::writeFile(absl::string_view buffer) { - return ::_write(fd_, buffer.data(), buffer.size()); +Api::IoCallSizeResult FileImplWin32::write(absl::string_view buffer) { + DWORD bytes_written; + BOOL result = WriteFile(fd_, buffer.data(), buffer.length(), &bytes_written, NULL); + if (result == 0) { + return resultFailure(-1, ::GetLastError()); + } + return resultSuccess(bytes_written); +}; + +Api::IoCallBoolResult FileImplWin32::close() { + ASSERT(isOpen()); + + BOOL result = CloseHandle(fd_); + fd_ = INVALID_HANDLE; + if (result == 0) { + return resultFailure(false, ::GetLastError()); + } + return resultSuccess(true); } FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) { - int out = 0; - int pmode = 0; + DWORD access = 0; + DWORD creation = OPEN_EXISTING; + if (in.test(File::Operation::Create)) { - out |= _O_CREAT; - pmode |= _S_IREAD | _S_IWRITE; + creation = OPEN_ALWAYS; + } + + if (in.test(File::Operation::Write)) { + access = GENERIC_WRITE; } + // Order of tests matter here. There reason for that + // is that `FILE_APPEND_DATA` should not be used together + // with `GENERIC_WRITE`. If both of them are used the file + // is not opened in append mode. if (in.test(File::Operation::Append)) { - out |= _O_APPEND; + access = FILE_APPEND_DATA; } - if (in.test(File::Operation::Read) && in.test(File::Operation::Write)) { - out |= _O_RDWR; - } else if (in.test(File::Operation::Read)) { - out |= _O_RDONLY; - } else if (in.test(File::Operation::Write)) { - out |= _O_WRONLY; + if (in.test(File::Operation::Read)) { + access |= GENERIC_READ; } - return {out, pmode}; + return {access, creation}; } -bool FileImplWin32::closeFile() { return ::_close(fd_) != -1; } - FilePtr InstanceImplWin32::createFile(const std::string& path) { return std::make_unique(path); } @@ -78,11 +105,19 @@ bool InstanceImplWin32::directoryExists(const std::string& path) { } ssize_t InstanceImplWin32::fileSize(const std::string& path) { - struct _stat info; - if (::_stat(path.c_str(), &info) != 0) { + auto fd = CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING, 0, NULL); + if (fd == INVALID_HANDLE) { + return -1; + } + ssize_t result = 0; + LARGE_INTEGER lFileSize; + BOOL bGetSize = GetFileSizeEx(fd, &lFileSize); + CloseHandle(fd); + if (!bGetSize) { return -1; } - return info.st_size; + result += lFileSize.QuadPart; + return result; } std::string InstanceImplWin32::fileReadToEnd(const std::string& path) { diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index f39b40378d64b..95989ceb9e0f6 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -14,18 +14,17 @@ class FileImplWin32 : public FileSharedImpl { ~FileImplWin32(); protected: + Api::IoCallBoolResult open(FlagSet flag) override; + Api::IoCallSizeResult write(absl::string_view buffer) override; + Api::IoCallBoolResult close() override; + +private: struct FlagsAndMode { - int flags_ = 0; - int pmode_ = 0; + DWORD access_ = 0; + DWORD creation_ = 0; }; - // Filesystem::FileSharedImpl FlagsAndMode translateFlag(FlagSet in); - void openFile(FlagSet in) override; - ssize_t writeFile(absl::string_view buffer) override; - bool closeFile() override; - -private: friend class FileSystemImplTest; }; diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index 7870c285e19d0..127451d3929e8 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -19,7 +19,7 @@ static constexpr FlagSet DefaultFlags{ class FileSystemImplTest : public testing::Test { protected: - int getFd(File* file) { + filesystem_os_id_t getFd(File* file) { #ifdef WIN32 auto file_impl = dynamic_cast(file); #else @@ -63,11 +63,7 @@ TEST_F(FileSystemImplTest, DirectoryExists) { } TEST_F(FileSystemImplTest, FileSize) { -#ifdef WIN32 - EXPECT_EQ(0, file_system_.fileSize("NUL")); -#else - EXPECT_EQ(0, file_system_.fileSize("/dev/null")); -#endif + EXPECT_EQ(0, file_system_.fileSize(std::string(Platform::null_device_path))); EXPECT_EQ(-1, file_system_.fileSize("/dev/blahblahblah")); const std::string data = "test string\ntest"; const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", data); @@ -235,15 +231,27 @@ TEST_F(FileSystemImplTest, Open) { EXPECT_TRUE(file->isOpen()); } +TEST_F(FileSystemImplTest, OpenReadOnly) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); + static constexpr FlagSet ReadOnlyFlags{1 << Filesystem::File::Operation::Read | + 1 << Filesystem::File::Operation::Create | + 1 << Filesystem::File::Operation::Append}; + FilePtr file = file_system_.createFile(new_file_path); + const Api::IoCallBoolResult result = file->open(ReadOnlyFlags); + EXPECT_TRUE(result.rc_); + EXPECT_TRUE(file->isOpen()); +} + TEST_F(FileSystemImplTest, OpenTwice) { const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); ::unlink(new_file_path.c_str()); FilePtr file = file_system_.createFile(new_file_path); - EXPECT_EQ(getFd(file.get()), -1); + EXPECT_EQ(getFd(file.get()), INVALID_HANDLE); const Api::IoCallBoolResult result1 = file->open(DefaultFlags); - const int initial_fd = getFd(file.get()); + const filesystem_os_id_t initial_fd = getFd(file.get()); EXPECT_TRUE(result1.rc_); EXPECT_TRUE(file->isOpen()); @@ -319,8 +327,7 @@ TEST_F(FileSystemImplTest, WriteAfterClose) { EXPECT_TRUE(bool_result2.rc_); const Api::IoCallSizeResult size_result = file->write(" new data"); EXPECT_EQ(-1, size_result.rc_); - EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, size_result.err_->getErrorCode()); - EXPECT_EQ("Bad file descriptor", size_result.err_->getErrorDetails()); + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, size_result.err_->getErrorCode()); } TEST_F(FileSystemImplTest, NonExistingFileAndReadOnly) { @@ -345,12 +352,30 @@ TEST_F(FileSystemImplTest, ExistingReadOnlyFileAndWrite) { std::string data(" new data"); const Api::IoCallSizeResult result = file->write(data); EXPECT_TRUE(result.rc_ < 0); - EXPECT_EQ(result.err_->getErrorDetails(), "Bad file descriptor"); +#ifdef WIN32 + EXPECT_EQ(IoFileError::IoErrorCode::Permission, result.err_->getErrorCode()); +#else + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, result.err_->getErrorCode()); +#endif } auto contents = TestEnvironment::readFileToStringForTest(file_path); EXPECT_EQ("existing file", contents); } +TEST_F(FileSystemImplTest, TestIoFileError) { + IoFileError error1(HANDLE_ERROR_PERM); + EXPECT_EQ(IoFileError::IoErrorCode::Permission, error1.getErrorCode()); + EXPECT_EQ(errorDetails(HANDLE_ERROR_PERM), error1.getErrorDetails()); + + IoFileError error2(HANDLE_ERROR_INVALID); + EXPECT_EQ(IoFileError::IoErrorCode::BadFd, error2.getErrorCode()); + EXPECT_EQ(errorDetails(HANDLE_ERROR_INVALID), error2.getErrorDetails()); + + int not_known_error = 42; + IoFileError error3(not_known_error); + EXPECT_EQ(IoFileError::IoErrorCode::UnknownError, error3.getErrorCode()); +} + } // namespace Filesystem } // namespace Envoy