From 2548dce98b5d309064597d64b285de5faf2acc53 Mon Sep 17 00:00:00 2001 From: Mark Raasveldt Date: Mon, 20 Sep 2021 13:33:19 +0200 Subject: [PATCH 1/3] Fix #2296: Avoid getting O_RDWR permissions when we only need O_WRONLY permissions so we can correctly write to fifo streams --- src/common/local_file_system.cpp | 100 +++++++++++++--------- src/storage/single_file_block_manager.cpp | 2 +- test/common/test_file_system.cpp | 2 +- tools/shell/shell-test.py | 12 +++ 4 files changed, 75 insertions(+), 41 deletions(-) diff --git a/src/common/local_file_system.cpp b/src/common/local_file_system.cpp index 321c9df3036..eede23bd5e3 100644 --- a/src/common/local_file_system.cpp +++ b/src/common/local_file_system.cpp @@ -33,14 +33,18 @@ extern "C" WINBASEAPI BOOL WINAPI GetPhysicallyInstalledSystemMemory(PULONGLONG) namespace duckdb { static void AssertValidFileFlags(uint8_t flags) { - // cannot combine Read and Write flags - D_ASSERT(!(flags & FileFlags::FILE_FLAGS_READ && flags & FileFlags::FILE_FLAGS_WRITE)); - // cannot combine Read and CREATE/Append flags - D_ASSERT(!(flags & FileFlags::FILE_FLAGS_READ && flags & FileFlags::FILE_FLAGS_APPEND)); - D_ASSERT(!(flags & FileFlags::FILE_FLAGS_READ && flags & FileFlags::FILE_FLAGS_FILE_CREATE)); - D_ASSERT(!(flags & FileFlags::FILE_FLAGS_READ && flags & FileFlags::FILE_FLAGS_FILE_CREATE_NEW)); +#ifdef DEBUG + bool is_read = flags & FileFlags::FILE_FLAGS_READ; + bool is_write = flags & FileFlags::FILE_FLAGS_WRITE; + // require either READ or WRITE (or both) + D_ASSERT(is_read || is_write); + // CREATE/Append flags require writing + D_ASSERT(is_write || !(flags & FileFlags::FILE_FLAGS_APPEND)); + D_ASSERT(is_write || !(flags & FileFlags::FILE_FLAGS_FILE_CREATE)); + D_ASSERT(is_write || !(flags & FileFlags::FILE_FLAGS_FILE_CREATE_NEW)); // cannot combine CREATE and CREATE_NEW flags D_ASSERT(!(flags & FileFlags::FILE_FLAGS_FILE_CREATE && flags & FileFlags::FILE_FLAGS_FILE_CREATE_NEW)); +#endif } #ifndef _WIN32 @@ -73,6 +77,32 @@ struct UnixFileHandle : public FileHandle { int fd; }; + +static FileType GetFileTypeInternal(int fd) { + struct stat s; + if (fstat(fd, &s) == -1) { + return FileType::FILE_TYPE_INVALID; + } + switch (s.st_mode & S_IFMT) { + case S_IFBLK: + return FileType::FILE_TYPE_BLOCKDEV; + case S_IFCHR: + return FileType::FILE_TYPE_CHARDEV; + case S_IFIFO: + return FileType::FILE_TYPE_FIFO; + case S_IFDIR: + return FileType::FILE_TYPE_DIR; + case S_IFLNK: + return FileType::FILE_TYPE_LINK; + case S_IFREG: + return FileType::FILE_TYPE_REGULAR; + case S_IFSOCK: + return FileType::FILE_TYPE_SOCKET; + default: + return FileType::FILE_TYPE_INVALID; + } +} + unique_ptr LocalFileSystem::OpenFile(const string &path, uint8_t flags, FileLockType lock_type, FileCompressionType compression, FileOpener *opener) { if (compression != FileCompressionType::UNCOMPRESSED) { @@ -83,12 +113,21 @@ unique_ptr LocalFileSystem::OpenFile(const string &path, uint8_t fla int open_flags = 0; int rc; - if (flags & FileFlags::FILE_FLAGS_READ) { + bool open_read = flags & FileFlags::FILE_FLAGS_READ; + bool open_write = flags & FileFlags::FILE_FLAGS_WRITE; + if (open_read && open_write) { + open_flags = O_RDWR; + } else if (open_read) { open_flags = O_RDONLY; + } else if (open_write) { + open_flags = O_WRONLY; } else { + throw InternalException("READ, WRITE or both should be specified when opening a file"); + } + if (flags & FileFlags::FILE_FLAGS_WRITE) { // need Read or Write D_ASSERT(flags & FileFlags::FILE_FLAGS_WRITE); - open_flags = O_RDWR | O_CLOEXEC; + open_flags |= O_CLOEXEC; if (flags & FileFlags::FILE_FLAGS_FILE_CREATE) { open_flags |= O_CREAT; } else if (flags & FileFlags::FILE_FLAGS_FILE_CREATE_NEW) { @@ -124,15 +163,19 @@ unique_ptr LocalFileSystem::OpenFile(const string &path, uint8_t fla // #endif if (lock_type != FileLockType::NO_LOCK) { // set lock on file - struct flock fl; - memset(&fl, 0, sizeof fl); - fl.l_type = lock_type == FileLockType::READ_LOCK ? F_RDLCK : F_WRLCK; - fl.l_whence = SEEK_SET; - fl.l_start = 0; - fl.l_len = 0; - rc = fcntl(fd, F_SETLK, &fl); - if (rc == -1) { - throw IOException("Could not set lock on file \"%s\": %s", path, strerror(errno)); + // but only if it is not an input/output stream + auto file_type = GetFileTypeInternal(fd); + if (file_type != FileType::FILE_TYPE_FIFO && file_type != FileType::FILE_TYPE_SOCKET) { + struct flock fl; + memset(&fl, 0, sizeof fl); + fl.l_type = lock_type == FileLockType::READ_LOCK ? F_RDLCK : F_WRLCK; + fl.l_whence = SEEK_SET; + fl.l_start = 0; + fl.l_len = 0; + rc = fcntl(fd, F_SETLK, &fl); + if (rc == -1) { + throw IOException("Could not set lock on file \"%s\": %s", path, strerror(errno)); + } } } return make_unique(*this, path, fd); @@ -218,28 +261,7 @@ time_t LocalFileSystem::GetLastModifiedTime(FileHandle &handle) { FileType LocalFileSystem::GetFileType(FileHandle &handle) { int fd = ((UnixFileHandle &)handle).fd; - struct stat s; - if (fstat(fd, &s) == -1) { - return FileType::FILE_TYPE_INVALID; - } - switch (s.st_mode & S_IFMT) { - case S_IFBLK: - return FileType::FILE_TYPE_BLOCKDEV; - case S_IFCHR: - return FileType::FILE_TYPE_CHARDEV; - case S_IFIFO: - return FileType::FILE_TYPE_FIFO; - case S_IFDIR: - return FileType::FILE_TYPE_DIR; - case S_IFLNK: - return FileType::FILE_TYPE_LINK; - case S_IFREG: - return FileType::FILE_TYPE_REGULAR; - case S_IFSOCK: - return FileType::FILE_TYPE_SOCKET; - default: - return FileType::FILE_TYPE_INVALID; - } + return GetFileTypeInternal(fd); } void LocalFileSystem::Truncate(FileHandle &handle, int64_t new_size) { diff --git a/src/storage/single_file_block_manager.cpp b/src/storage/single_file_block_manager.cpp index 932922450e8..237c5b93520 100644 --- a/src/storage/single_file_block_manager.cpp +++ b/src/storage/single_file_block_manager.cpp @@ -79,7 +79,7 @@ SingleFileBlockManager::SingleFileBlockManager(DatabaseInstance &db, string path flags = FileFlags::FILE_FLAGS_READ; lock = FileLockType::READ_LOCK; } else { - flags = FileFlags::FILE_FLAGS_WRITE; + flags = FileFlags::FILE_FLAGS_WRITE | FileFlags::FILE_FLAGS_READ; lock = FileLockType::WRITE_LOCK; if (create_new) { flags |= FileFlags::FILE_FLAGS_FILE_CREATE; diff --git a/test/common/test_file_system.cpp b/test/common/test_file_system.cpp index 47ad8723fb6..2011b89f958 100644 --- a/test/common/test_file_system.cpp +++ b/test/common/test_file_system.cpp @@ -113,7 +113,7 @@ TEST_CASE("Test file buffers for reading/writing to file", "[file_system]") { // open file for writing REQUIRE_NOTHROW(handle = fs->OpenFile(fname, - FileFlags::FILE_FLAGS_WRITE | FileFlags::FILE_FLAGS_FILE_CREATE | + FileFlags::FILE_FLAGS_WRITE | FileFlags::FILE_FLAGS_READ | FileFlags::FILE_FLAGS_FILE_CREATE | FileFlags::FILE_FLAGS_DIRECT_IO, FileLockType::WRITE_LOCK)); // write the buffer diff --git a/tools/shell/shell-test.py b/tools/shell/shell-test.py index a9026d22918..4a957134f4e 100644 --- a/tools/shell/shell-test.py +++ b/tools/shell/shell-test.py @@ -539,3 +539,15 @@ def tf(): input_file='test/sql/copy/csv/data/test/test.csv', out='''foo,bar,baz 0,0," test"''') + + test(''' + COPY (SELECT 42) TO '/dev/stdout' WITH (FORMAT 'csv'); + ''', + extra_commands=['-csv', ':memory:'], + out='''42''') + + test(''' + COPY (SELECT 42) TO '/dev/stderr' WITH (FORMAT 'csv'); + ''', + extra_commands=['-csv', ':memory:'], + err='''42''') From 8d2805c1af29d29bc505b855e50a6e3dbc709c61 Mon Sep 17 00:00:00 2001 From: Mark Raasveldt Date: Mon, 20 Sep 2021 13:47:54 +0200 Subject: [PATCH 2/3] Format fix --- src/common/local_file_system.cpp | 1 - test/common/test_file_system.cpp | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/local_file_system.cpp b/src/common/local_file_system.cpp index eede23bd5e3..efc6a69e95d 100644 --- a/src/common/local_file_system.cpp +++ b/src/common/local_file_system.cpp @@ -77,7 +77,6 @@ struct UnixFileHandle : public FileHandle { int fd; }; - static FileType GetFileTypeInternal(int fd) { struct stat s; if (fstat(fd, &s) == -1) { diff --git a/test/common/test_file_system.cpp b/test/common/test_file_system.cpp index 2011b89f958..fcf06c87b41 100644 --- a/test/common/test_file_system.cpp +++ b/test/common/test_file_system.cpp @@ -113,8 +113,8 @@ TEST_CASE("Test file buffers for reading/writing to file", "[file_system]") { // open file for writing REQUIRE_NOTHROW(handle = fs->OpenFile(fname, - FileFlags::FILE_FLAGS_WRITE | FileFlags::FILE_FLAGS_READ | FileFlags::FILE_FLAGS_FILE_CREATE | - FileFlags::FILE_FLAGS_DIRECT_IO, + FileFlags::FILE_FLAGS_WRITE | FileFlags::FILE_FLAGS_READ | + FileFlags::FILE_FLAGS_FILE_CREATE | FileFlags::FILE_FLAGS_DIRECT_IO, FileLockType::WRITE_LOCK)); // write the buffer REQUIRE_NOTHROW(buf->Write(*handle, 0)); From 8e80101f5fbabbbf7249d80e5ab9685486ed64f4 Mon Sep 17 00:00:00 2001 From: Mark Raasveldt Date: Mon, 20 Sep 2021 14:50:01 +0200 Subject: [PATCH 3/3] Correctly handle READ/WRITE/READ+WRITE flags on Windows --- src/common/local_file_system.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/common/local_file_system.cpp b/src/common/local_file_system.cpp index efc6a69e95d..4cbd237dd0b 100644 --- a/src/common/local_file_system.cpp +++ b/src/common/local_file_system.cpp @@ -123,7 +123,7 @@ unique_ptr LocalFileSystem::OpenFile(const string &path, uint8_t fla } else { throw InternalException("READ, WRITE or both should be specified when opening a file"); } - if (flags & FileFlags::FILE_FLAGS_WRITE) { + if (open_write) { // need Read or Write D_ASSERT(flags & FileFlags::FILE_FLAGS_WRITE); open_flags |= O_CLOEXEC; @@ -463,14 +463,21 @@ unique_ptr LocalFileSystem::OpenFile(const string &path, uint8_t fla DWORD share_mode; DWORD creation_disposition = OPEN_EXISTING; DWORD flags_and_attributes = FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED; - if (flags & FileFlags::FILE_FLAGS_READ) { + bool open_read = flags & FileFlags::FILE_FLAGS_READ; + bool open_write = flags & FileFlags::FILE_FLAGS_WRITE; + if (open_read && open_write) { + desired_access = GENERIC_READ | GENERIC_WRITE; + share_mode = 0; + } else if (open_read) { desired_access = GENERIC_READ; share_mode = FILE_SHARE_READ; - } else { - // need Read or Write - D_ASSERT(flags & FileFlags::FILE_FLAGS_WRITE); - desired_access = GENERIC_READ | GENERIC_WRITE; + } else if (open_write) { + desired_access = GENERIC_WRITE; share_mode = 0; + } else { + throw InternalException("READ, WRITE or both should be specified when opening a file"); + } + if (open_write) { if (flags & FileFlags::FILE_FLAGS_FILE_CREATE) { creation_disposition = OPEN_ALWAYS; } else if (flags & FileFlags::FILE_FLAGS_FILE_CREATE_NEW) {