diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h index 64923d5085440..ae6cd015584ef 100644 --- a/include/envoy/filesystem/filesystem.h +++ b/include/envoy/filesystem/filesystem.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -13,6 +14,8 @@ namespace Envoy { namespace Filesystem { +using FlagSet = std::bitset<4>; + /** * Abstraction for a basic file on disk. */ @@ -20,13 +23,20 @@ class File { public: virtual ~File() = default; + enum Operation { + Read, + Write, + Create, + Append, + }; + /** - * Open the file with O_RDWR | O_APPEND | O_CREAT + * Open the file with Flag * The file will be closed when this object is destructed * * @return bool whether the open succeeded */ - virtual Api::IoCallBoolResult open() PURE; + virtual Api::IoCallBoolResult open(FlagSet flags) PURE; /** * Write the buffer to the file. The file must be explicitly opened before writing. diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index cbf5f8caa4a6a..dd2615421db13 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -41,8 +41,16 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch open(); } +Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { + static constexpr Filesystem::FlagSet default_flags{ + 1 << Filesystem::File::Operation::Read | 1 << Filesystem::File::Operation::Write | + 1 << Filesystem::File::Operation::Create | 1 << Filesystem::File::Operation::Append}; + + return default_flags; +} + void AccessLogFileImpl::open() { - const Api::IoCallBoolResult result = file_->open(); + const Api::IoCallBoolResult result = file_->open(defaultFlags()); if (!result.rc_) { throw EnvoyException( fmt::format("unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails())); diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index 4a8dedcdf367e..03efbf2d030ba 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -83,6 +83,9 @@ class AccessLogFileImpl : public AccessLogFile { void open(); void createFlushStructures(); + // return default flags set which used by open + static Filesystem::FlagSet defaultFlags(); + // Minimum size before the flush thread will be told to flush. static const uint64_t MIN_FLUSH_SIZE = 1024 * 64; diff --git a/source/common/filesystem/file_shared_impl.cc b/source/common/filesystem/file_shared_impl.cc index 5e235c534e639..dc0e8bfcdc328 100644 --- a/source/common/filesystem/file_shared_impl.cc +++ b/source/common/filesystem/file_shared_impl.cc @@ -9,12 +9,12 @@ Api::IoError::IoErrorCode IoFileError::getErrorCode() const { return IoErrorCode std::string IoFileError::getErrorDetails() const { return ::strerror(errno_); } -Api::IoCallBoolResult FileSharedImpl::open() { +Api::IoCallBoolResult FileSharedImpl::open(FlagSet in) { if (isOpen()) { return resultSuccess(true); } - openFile(); + openFile(in); return fd_ != -1 ? resultSuccess(true) : resultFailure(false, errno); } @@ -36,4 +36,4 @@ bool FileSharedImpl::isOpen() const { return fd_ != -1; }; std::string FileSharedImpl::path() const { return path_; }; } // namespace Filesystem -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/common/filesystem/file_shared_impl.h b/source/common/filesystem/file_shared_impl.h index 5e05b607cfa8d..06e166661287e 100644 --- a/source/common/filesystem/file_shared_impl.h +++ b/source/common/filesystem/file_shared_impl.h @@ -42,14 +42,14 @@ class FileSharedImpl : public File { ~FileSharedImpl() override = default; // Filesystem::File - Api::IoCallBoolResult open() override; + 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() PURE; + virtual void openFile(FlagSet in) PURE; virtual ssize_t writeFile(absl::string_view buffer) PURE; virtual bool closeFile() PURE; diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index 5d1b3b03eb8da..0ea5a45efc21b 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -28,17 +28,38 @@ FileImplPosix::~FileImplPosix() { } } -void FileImplPosix::openFile() { - const int flags = O_RDWR | O_APPEND | O_CREAT; - const int mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; - - fd_ = ::open(path_.c_str(), flags, mode); +void FileImplPosix::openFile(FlagSet in) { + const auto flags_and_mode = translateFlag(in); + fd_ = ::open(path_.c_str(), flags_and_mode.flags_, flags_and_mode.mode_); } ssize_t FileImplPosix::writeFile(absl::string_view buffer) { return ::write(fd_, buffer.data(), buffer.size()); } +FileImplPosix::FlagsAndMode FileImplPosix::translateFlag(FlagSet in) { + int out = 0; + mode_t mode = 0; + if (in.test(File::Operation::Create)) { + out |= O_CREAT; + mode |= S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; + } + + if (in.test(File::Operation::Append)) { + out |= O_APPEND; + } + + 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; + } + + return {out, mode}; +} + bool FileImplPosix::closeFile() { return ::close(fd_) != -1; } FilePtr InstanceImplPosix::createFile(const std::string& path) { diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index b0c6c6388321f..5777c85ef4be7 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -16,8 +16,14 @@ class FileImplPosix : public FileSharedImpl { ~FileImplPosix() override; protected: + struct FlagsAndMode { + int flags_ = 0; + mode_t mode_ = 0; + }; + // Filesystem::FileSharedImpl - void openFile() override; + FlagsAndMode translateFlag(FlagSet in); + void openFile(FlagSet flags) override; ssize_t writeFile(absl::string_view buffer) override; bool closeFile() override; diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 41a15235ef3cd..cc08abbc924ce 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -33,17 +33,38 @@ FileImplWin32::~FileImplWin32() { } } -void FileImplWin32::openFile() { - const int flags = _O_RDWR | _O_APPEND | _O_CREAT; - const int mode = _S_IREAD | _S_IWRITE; - - fd_ = ::_open(path_.c_str(), flags, mode); +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_); } ssize_t FileImplWin32::writeFile(absl::string_view buffer) { return ::_write(fd_, buffer.data(), buffer.size()); } +FileImplWin32::FlagsAndMode FileImplWin32::translateFlag(FlagSet in) { + int out = 0; + int pmode = 0; + if (in.test(File::Operation::Create)) { + out |= _O_CREAT; + pmode |= _S_IREAD | _S_IWRITE; + } + + if (in.test(File::Operation::Append)) { + out |= _O_APPEND; + } + + 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; + } + + return {out, pmode}; +} + bool FileImplWin32::closeFile() { return ::_close(fd_) != -1; } FilePtr InstanceImplWin32::createFile(const std::string& path) { diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index 7c7add205c87d..8d0a2d8ba13d8 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -14,8 +14,14 @@ class FileImplWin32 : public FileSharedImpl { ~FileImplWin32(); protected: + struct FlagsAndMode { + int flags_ = 0; + int pmode_ = 0; + }; + // Filesystem::FileSharedImpl - void openFile() override; + FlagsAndMode translateFlag(FlagSet in); + void openFile(FlagSet in) override; ssize_t writeFile(absl::string_view buffer) override; bool closeFile() override; diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index f5576e822b651..c8cfdc56ae3a7 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -12,6 +12,10 @@ namespace Envoy { namespace Filesystem { +static constexpr FlagSet DefaultFlags{ + 1 << Filesystem::File::Operation::Read | 1 << Filesystem::File::Operation::Write | + 1 << Filesystem::File::Operation::Create | 1 << Filesystem::File::Operation::Append}; + class FileSystemImplTest : public testing::Test { protected: int getFd(File* file) { @@ -154,7 +158,7 @@ TEST_F(FileSystemImplTest, Open) { ::unlink(new_file_path.c_str()); FilePtr file = file_system_.createFile(new_file_path); - const Api::IoCallBoolResult result = file->open(); + const Api::IoCallBoolResult result = file->open(DefaultFlags); EXPECT_TRUE(result.rc_); EXPECT_TRUE(file->isOpen()); } @@ -166,13 +170,13 @@ TEST_F(FileSystemImplTest, OpenTwice) { FilePtr file = file_system_.createFile(new_file_path); EXPECT_EQ(getFd(file.get()), -1); - const Api::IoCallBoolResult result1 = file->open(); + const Api::IoCallBoolResult result1 = file->open(DefaultFlags); const int initial_fd = getFd(file.get()); EXPECT_TRUE(result1.rc_); EXPECT_TRUE(file->isOpen()); // check that we don't leak a file descriptor - const Api::IoCallBoolResult result2 = file->open(); + const Api::IoCallBoolResult result2 = file->open(DefaultFlags); EXPECT_EQ(initial_fd, getFd(file.get())); EXPECT_TRUE(result2.rc_); EXPECT_TRUE(file->isOpen()); @@ -180,7 +184,7 @@ TEST_F(FileSystemImplTest, OpenTwice) { TEST_F(FileSystemImplTest, OpenBadFilePath) { FilePtr file = file_system_.createFile(""); - const Api::IoCallBoolResult result = file->open(); + const Api::IoCallBoolResult result = file->open(DefaultFlags); EXPECT_FALSE(result.rc_); } @@ -190,7 +194,7 @@ TEST_F(FileSystemImplTest, ExistingFile) { { FilePtr file = file_system_.createFile(file_path); - const Api::IoCallBoolResult open_result = file->open(); + const Api::IoCallBoolResult open_result = file->open(DefaultFlags); EXPECT_TRUE(open_result.rc_); std::string data(" new data"); const Api::IoCallSizeResult result = file->write(data); @@ -207,7 +211,7 @@ TEST_F(FileSystemImplTest, NonExistingFile) { { FilePtr file = file_system_.createFile(new_file_path); - const Api::IoCallBoolResult open_result = file->open(); + const Api::IoCallBoolResult open_result = file->open(DefaultFlags); EXPECT_TRUE(open_result.rc_); std::string data(" new data"); const Api::IoCallSizeResult result = file->write(data); @@ -223,7 +227,7 @@ TEST_F(FileSystemImplTest, Close) { ::unlink(new_file_path.c_str()); FilePtr file = file_system_.createFile(new_file_path); - const Api::IoCallBoolResult result1 = file->open(); + const Api::IoCallBoolResult result1 = file->open(DefaultFlags); EXPECT_TRUE(result1.rc_); EXPECT_TRUE(file->isOpen()); @@ -237,7 +241,7 @@ TEST_F(FileSystemImplTest, WriteAfterClose) { ::unlink(new_file_path.c_str()); FilePtr file = file_system_.createFile(new_file_path); - const Api::IoCallBoolResult bool_result1 = file->open(); + const Api::IoCallBoolResult bool_result1 = file->open(DefaultFlags); EXPECT_TRUE(bool_result1.rc_); const Api::IoCallBoolResult bool_result2 = file->close(); EXPECT_TRUE(bool_result2.rc_); @@ -247,5 +251,34 @@ TEST_F(FileSystemImplTest, WriteAfterClose) { EXPECT_EQ("Bad file descriptor", size_result.err_->getErrorDetails()); } +TEST_F(FileSystemImplTest, NonExistingFileAndReadOnly) { + const std::string new_file_path = TestEnvironment::temporaryPath("envoy_this_not_exist"); + ::unlink(new_file_path.c_str()); + + static constexpr FlagSet flag(static_cast(Filesystem::File::Operation::Read)); + FilePtr file = file_system_.createFile(new_file_path); + const Api::IoCallBoolResult open_result = file->open(flag); + EXPECT_FALSE(open_result.rc_); +} + +TEST_F(FileSystemImplTest, ExistingReadOnlyFileAndWrite) { + const std::string file_path = + TestEnvironment::writeStringToFileForTest("test_envoy", "existing file"); + + { + static constexpr FlagSet flag(static_cast(Filesystem::File::Operation::Read)); + FilePtr file = file_system_.createFile(file_path); + const Api::IoCallBoolResult open_result = file->open(flag); + EXPECT_TRUE(open_result.rc_); + 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"); + } + + auto contents = TestEnvironment::readFileToStringForTest(file_path); + EXPECT_EQ("existing file", contents); +} + } // namespace Filesystem } // namespace Envoy diff --git a/test/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc b/test/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc index 0e711a4846307..fb4fb3c19d8f8 100644 --- a/test/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc +++ b/test/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc @@ -42,9 +42,15 @@ void QuicRecordTestOutputToFile(const std::string& filename, QuicStringPiece dat return; } + static constexpr Envoy::Filesystem::FlagSet DefaultFlags{ + 1 << Envoy::Filesystem::File::Operation::Read | + 1 << Envoy::Filesystem::File::Operation::Write | + 1 << Envoy::Filesystem::File::Operation::Create | + 1 << Envoy::Filesystem::File::Operation::Append}; + const std::string output_path = output_dir + filename; Envoy::Filesystem::FilePtr file = file_system.createFile(output_path); - if (!file->open().rc_) { + if (!file->open(DefaultFlags).rc_) { QUIC_LOG(ERROR) << "Failed to open test output file: " << output_path; return; } diff --git a/test/mocks/filesystem/mocks.cc b/test/mocks/filesystem/mocks.cc index 5aa5806a26a00..7cf7333150308 100644 --- a/test/mocks/filesystem/mocks.cc +++ b/test/mocks/filesystem/mocks.cc @@ -9,7 +9,7 @@ namespace Filesystem { MockFile::MockFile() : num_opens_(0), num_writes_(0), is_open_(false) {} MockFile::~MockFile() = default; -Api::IoCallBoolResult MockFile::open() { +Api::IoCallBoolResult MockFile::open(FlagSet) { Thread::LockGuard lock(open_mutex_); Api::IoCallBoolResult result = open_(); diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index b62af89b68a4e..459cdfd8ce65b 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -19,7 +19,7 @@ class MockFile : public File { ~MockFile() override; // Filesystem::File - Api::IoCallBoolResult open() override; + Api::IoCallBoolResult open(FlagSet flag) override; Api::IoCallSizeResult write(absl::string_view buffer) override; Api::IoCallBoolResult close() override; bool isOpen() const override { return is_open_; };