From 6d27b5234973fdd8e91a953b1d977a2004878c12 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Thu, 20 Aug 2026 17:14:43 -0700 Subject: [PATCH 01/23] Add cross-platform file I/O utilities --- cpp/include/qdk/chemistry/utils/file_io.hpp | 65 +++ cpp/src/qdk/chemistry/utils/CMakeLists.txt | 1 + cpp/src/qdk/chemistry/utils/file_io.cpp | 586 ++++++++++++++++++++ cpp/tests/test_file_io.cpp | 322 +++++++++++ python/src/qdk_chemistry/utils/__init__.py | 10 + python/src/qdk_chemistry/utils/file_io.py | 212 +++++++ python/tests/test_utils_file_io.py | 231 ++++++++ 7 files changed, 1427 insertions(+) create mode 100644 cpp/include/qdk/chemistry/utils/file_io.hpp create mode 100644 cpp/src/qdk/chemistry/utils/file_io.cpp create mode 100644 cpp/tests/test_file_io.cpp create mode 100644 python/src/qdk_chemistry/utils/file_io.py create mode 100644 python/tests/test_utils_file_io.py diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp new file mode 100644 index 000000000..e7368f672 --- /dev/null +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -0,0 +1,65 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE.txt in the project root for +// license information. + +#pragma once + +#include +#include +#include +#include + +namespace qdk::chemistry::utils { + +using AtomicFileWriter = + std::function; + +/** + * @brief Create the parent directory of a path when it does not exist. + * + * A path without a parent component refers to the current directory and needs + * no action. + */ +void ensure_parent_directory(const std::filesystem::path& path); + +/** + * @brief Read an entire file as binary-safe text. + */ +std::string read_text_file(const std::filesystem::path& path); + +/** + * @brief Write a file through a temporary sibling and atomically replace the + * destination. + * + * The writer receives a unique temporary path in the destination directory. + * The temporary file is removed if the writer throws. Keeping the temporary + * file beside the destination allows replacement to remain atomic. The + * temporary path preserves the destination's suffixes for format-sensitive + * writers. + * + * On POSIX, replacing an existing file preserves its permission bits and new + * files are created with owner-only permissions. On Windows, replacement + * preserves the read-only attribute and new files use the filesystem's + * standard access controls. Other file-object metadata and hard-link identity + * are not preserved. Atomic replacement prevents partial visibility but does + * not guarantee durability after power loss. + * + * The destination's parent directory must not be writable by principals less + * privileged than the process performing the write. + * + * @param path Destination path. + * @param writer Function that writes the complete temporary file. + * @param create_parent_directories Create missing parent directories when true. + */ +void write_file_atomically(const std::filesystem::path& path, + const AtomicFileWriter& writer, + bool create_parent_directories = false); + +/** + * @brief Write binary-safe text through an atomic file replacement. + */ +void write_text_file_atomically(const std::filesystem::path& path, + std::string_view contents, + bool create_parent_directories = false); + +} // namespace qdk::chemistry::utils diff --git a/cpp/src/qdk/chemistry/utils/CMakeLists.txt b/cpp/src/qdk/chemistry/utils/CMakeLists.txt index c6ee4b7e2..841870a88 100644 --- a/cpp/src/qdk/chemistry/utils/CMakeLists.txt +++ b/cpp/src/qdk/chemistry/utils/CMakeLists.txt @@ -1,4 +1,5 @@ target_sources(chemistry PRIVATE + file_io.cpp hash_context.cpp valence_space.cpp orbital_rotation.cpp diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp new file mode 100644 index 000000000..7cf77ff16 --- /dev/null +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -0,0 +1,586 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE.txt in the project root for +// license information. + +#include +#include +#include +#include +#include +#include +#include + +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include +#else +#include +#include +#include +#endif + +namespace qdk::chemistry::utils { +namespace { + +class ScopedReadHandle { + public: +#ifdef _WIN32 + using NativeHandle = HANDLE; +#else + using NativeHandle = int; +#endif + + static NativeHandle invalid_handle() { +#ifdef _WIN32 + return INVALID_HANDLE_VALUE; +#else + return -1; +#endif + } + + explicit ScopedReadHandle(NativeHandle handle) : handle_(handle) {} + ScopedReadHandle(const ScopedReadHandle&) = delete; + ScopedReadHandle& operator=(const ScopedReadHandle&) = delete; + + ~ScopedReadHandle() { + if (handle_ == invalid_handle()) { + return; + } +#ifdef _WIN32 + CloseHandle(handle_); +#else + ::close(handle_); +#endif + } + + NativeHandle get() const { return handle_; } + + private: + NativeHandle handle_; +}; + +std::string display_path(const std::filesystem::path& path) { + const auto value = path.u8string(); + return {value.begin(), value.end()}; +} + +std::filesystem::path make_temporary_path( + const std::filesystem::path& destination) { + static std::atomic counter{0}; + const auto timestamp = + std::chrono::steady_clock::now().time_since_epoch().count(); + + std::filesystem::path temporary_name = ".qdk-tmp-"; + temporary_name += std::to_string(timestamp); + temporary_name += "-"; + temporary_name += std::to_string(counter.fetch_add(1)); + + const auto filename = destination.filename().native(); + const auto dot = filename.find( + static_cast('.'), + !filename.empty() && + filename.front() == + static_cast('.') + ? 1 + : 0); + if (dot != decltype(filename)::npos) { + temporary_name += filename.substr(dot); + } + return destination.parent_path() / temporary_name; +} + +std::filesystem::path make_compact_temporary_path( + const std::filesystem::path& destination, int attempt, int stem_length) { + constexpr std::string_view alphabet = + "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-"; + std::filesystem::path temporary_name = + std::string(static_cast(stem_length - 1), 'q') + + alphabet[static_cast(attempt)]; + + const auto filename = destination.filename().native(); + const auto dot = filename.find( + static_cast('.'), + !filename.empty() && + filename.front() == + static_cast('.') + ? 1 + : 0); + if (dot != decltype(filename)::npos) { + temporary_name += filename.substr(dot); + } + return destination.parent_path() / temporary_name; +} + +#ifdef _WIN32 +DWORD settable_file_attributes(DWORD attributes) { + constexpr DWORD supported_attributes = + FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_HIDDEN | + FILE_ATTRIBUTE_NOT_CONTENT_INDEXED | FILE_ATTRIBUTE_OFFLINE | + FILE_ATTRIBUTE_READONLY | FILE_ATTRIBUTE_SYSTEM | + FILE_ATTRIBUTE_TEMPORARY; + const DWORD result = attributes & supported_attributes; + return result == 0 ? FILE_ATTRIBUTE_NORMAL : result; +} +#endif + +class ReservedTemporaryFile { + public: +#ifdef _WIN32 + using NativeHandle = HANDLE; +#else + using NativeHandle = int; +#endif + + static NativeHandle invalid_handle() { +#ifdef _WIN32 + return INVALID_HANDLE_VALUE; +#else + return -1; +#endif + } + + ReservedTemporaryFile(std::filesystem::path path, NativeHandle handle) + : path_(std::move(path)), + handle_(handle), + cleanup_(handle != invalid_handle()) {} + + ReservedTemporaryFile(const ReservedTemporaryFile&) = delete; + ReservedTemporaryFile& operator=(const ReservedTemporaryFile&) = delete; + + ReservedTemporaryFile(ReservedTemporaryFile&& other) noexcept + : path_(std::move(other.path_)), + handle_(other.handle_), + cleanup_(other.cleanup_) { + other.handle_ = invalid_handle(); + other.cleanup_ = false; + } + + ~ReservedTemporaryFile() { + close(); + if (cleanup_) { +#ifdef _WIN32 + const DWORD attributes = GetFileAttributesW(path_.c_str()); + if (attributes != INVALID_FILE_ATTRIBUTES && + (attributes & FILE_ATTRIBUTE_READONLY) != 0) { + SetFileAttributesW( + path_.c_str(), + settable_file_attributes(attributes & ~FILE_ATTRIBUTE_READONLY)); + } +#endif + std::error_code ignored; + std::filesystem::remove(path_, ignored); + } + } + + const std::filesystem::path& path() const { return path_; } + + void verify_identity() const { +#ifdef _WIN32 + BY_HANDLE_FILE_INFORMATION reserved_info; + if (!GetFileInformationByHandle(handle_, &reserved_info)) { + throw_last_error("Could not inspect reserved temporary file"); + } + + HANDLE current_handle = CreateFileW( + path_.c_str(), FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (current_handle == INVALID_HANDLE_VALUE) { + throw_last_error("Could not inspect temporary path"); + } + + BY_HANDLE_FILE_INFORMATION current_info; + const bool inspected = + GetFileInformationByHandle(current_handle, ¤t_info); + const DWORD inspection_error = inspected ? ERROR_SUCCESS : GetLastError(); + CloseHandle(current_handle); + if (!inspected) { + throw_windows_error("Could not inspect temporary path", inspection_error); + } + + const bool same_file = + reserved_info.dwVolumeSerialNumber == + current_info.dwVolumeSerialNumber && + reserved_info.nFileIndexHigh == current_info.nFileIndexHigh && + reserved_info.nFileIndexLow == current_info.nFileIndexLow; + if (!same_file || + (current_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0 || + current_info.nNumberOfLinks != 1) { + throw std::runtime_error("Temporary file identity changed: '" + + display_path(path_) + "'"); + } +#else + struct stat reserved_status{}; + struct stat current_status{}; + if (::fstat(handle_, &reserved_status) != 0 || + ::lstat(path_.c_str(), ¤t_status) != 0) { + throw std::runtime_error("Could not inspect temporary file: '" + + display_path(path_) + "'"); + } + if (reserved_status.st_dev != current_status.st_dev || + reserved_status.st_ino != current_status.st_ino || + !S_ISREG(current_status.st_mode) || current_status.st_nlink != 1) { + throw std::runtime_error("Temporary file identity changed: '" + + display_path(path_) + "'"); + } +#endif + } + + void set_permissions(std::filesystem::perms permissions) { +#ifdef _WIN32 + static_cast(permissions); +#else + if (::fchmod(handle_, static_cast(permissions)) != 0) { + throw std::runtime_error("Could not set temporary file permissions: '" + + display_path(path_) + "'"); + } +#endif + } + + void release() { + close(); + cleanup_ = false; + } + + private: +#ifdef _WIN32 + [[noreturn]] static void throw_windows_error(const std::string& message, + DWORD windows_error) { + const std::error_code error(static_cast(windows_error), + std::system_category()); + throw std::runtime_error(message + ": " + error.message()); + } + + [[noreturn]] static void throw_last_error(const std::string& message) { + throw_windows_error(message, GetLastError()); + } +#endif + + void close() { + if (handle_ == invalid_handle()) { + return; + } +#ifdef _WIN32 + CloseHandle(handle_); +#else + ::close(handle_); +#endif + handle_ = invalid_handle(); + } + + std::filesystem::path path_; + NativeHandle handle_ = invalid_handle(); + bool cleanup_ = true; +}; + +ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, + std::error_code& error) { +#ifdef _WIN32 + HANDLE handle = + CreateFileW(path.c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); + if (handle == INVALID_HANDLE_VALUE) { + error = std::error_code(static_cast(GetLastError()), + std::system_category()); + return {path, ReservedTemporaryFile::invalid_handle()}; + } +#else + const int descriptor = + ::open(path.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600); + if (descriptor == -1) { + error = std::error_code(errno, std::generic_category()); + return {path, ReservedTemporaryFile::invalid_handle()}; + } +#endif + return {path, +#ifdef _WIN32 + handle +#else + descriptor +#endif + }; +} + +ReservedTemporaryFile reserve_temporary_file( + const std::filesystem::path& destination) { + constexpr int max_attempts = 64; + for (int attempt = 0; attempt < max_attempts; ++attempt) { + const auto temporary_path = make_temporary_path(destination); + std::error_code error; + auto temporary_file = create_exclusive_file(temporary_path, error); + if (!error) { + return temporary_file; + } + if (error != std::errc::file_exists) { + if (error == std::errc::filename_too_long) { + break; + } + throw std::runtime_error("Could not create temporary file beside '" + + display_path(destination) + + "': " + error.message()); + } + } + + for (int stem_length = 16; stem_length > 0; --stem_length) { + for (int attempt = 0; attempt < max_attempts; ++attempt) { + const auto temporary_path = + make_compact_temporary_path(destination, attempt, stem_length); + std::error_code error; + auto temporary_file = create_exclusive_file(temporary_path, error); + if (!error) { + return temporary_file; + } + if (error == std::errc::file_exists) { + continue; + } + if (error == std::errc::filename_too_long) { + break; + } + throw std::runtime_error("Could not create temporary file beside '" + + display_path(destination) + + "': " + error.message()); + } + } + + throw std::runtime_error("Could not create a unique temporary file beside '" + + display_path(destination) + "'"); +} + +void replace_file(const std::filesystem::path& source, + const std::filesystem::path& destination) { +#ifdef _WIN32 + const DWORD original_attributes = GetFileAttributesW(destination.c_str()); + const bool destination_exists = + original_attributes != INVALID_FILE_ATTRIBUTES; + + auto move = [&]() { + return MoveFileExW(source.c_str(), destination.c_str(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) != 0; + }; + if (move()) { + return; + } + + const DWORD first_error = GetLastError(); + const bool read_only = destination_exists && + (original_attributes & FILE_ATTRIBUTE_READONLY) != 0; + if (first_error != ERROR_ACCESS_DENIED || !read_only || + !SetFileAttributesW(destination.c_str(), + settable_file_attributes(original_attributes & + ~FILE_ATTRIBUTE_READONLY))) { + const std::error_code error(static_cast(first_error), + std::system_category()); + throw std::runtime_error("Could not replace file '" + + display_path(destination) + + "': " + error.message()); + } + + if (!move()) { + const DWORD retry_error = GetLastError(); + SetFileAttributesW(destination.c_str(), + settable_file_attributes(original_attributes)); + const std::error_code error(static_cast(retry_error), + std::system_category()); + throw std::runtime_error("Could not replace file '" + + display_path(destination) + + "': " + error.message()); + } + if (!SetFileAttributesW(destination.c_str(), + settable_file_attributes(original_attributes))) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + throw std::runtime_error("Could not restore file attributes for '" + + display_path(destination) + + "': " + error.message()); + } +#else + std::error_code error; + std::filesystem::rename(source, destination, error); + if (error) { + throw std::runtime_error("Could not replace file '" + + display_path(destination) + + "': " + error.message()); + } +#endif +} + +void preserve_permissions(ReservedTemporaryFile& temporary_file, + const std::filesystem::path& destination) { + std::error_code status_error; + const auto status = std::filesystem::status(destination, status_error); + if (status_error) { + if (status_error == std::errc::no_such_file_or_directory) { +#ifndef _WIN32 + temporary_file.set_permissions(std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write); +#endif + return; + } + throw std::runtime_error("Could not inspect permissions for '" + + display_path(destination) + + "': " + status_error.message()); + } + if (!std::filesystem::exists(status)) { +#ifndef _WIN32 + temporary_file.set_permissions(std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write); +#endif + return; + } + + temporary_file.set_permissions(status.permissions()); +} + +} // namespace + +void ensure_parent_directory(const std::filesystem::path& path) { + const auto parent = path.parent_path(); + if (parent.empty()) { + return; + } + + std::error_code error; + std::filesystem::create_directories(parent, error); + if (error) { + throw std::runtime_error("Could not create parent directory for '" + + display_path(path) + "': " + error.message()); + } +} + +std::string read_text_file(const std::filesystem::path& path) { + std::string contents; +#ifdef _WIN32 + HANDLE handle = + CreateFileW(path.c_str(), GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + if (handle == INVALID_HANDLE_VALUE) { + throw std::runtime_error("Could not open file for reading: '" + + display_path(path) + "'"); + } + const ScopedReadHandle scoped_handle(handle); + + BY_HANDLE_FILE_INFORMATION info; + if (!GetFileInformationByHandle(handle, &info) || + (info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 || + GetFileType(handle) != FILE_TYPE_DISK) { + throw std::runtime_error("Path is not a regular file: '" + + display_path(path) + "'"); + } + + std::array buffer{}; + while (true) { + DWORD bytes_read = 0; + if (!ReadFile(handle, buffer.data(), static_cast(buffer.size()), + &bytes_read, nullptr)) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + throw std::runtime_error("Could not read file: '" + display_path(path) + + "': " + error.message()); + } + if (bytes_read == 0) { + break; + } + contents.append(buffer.data(), bytes_read); + } +#else + const int descriptor = + ::open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC); + if (descriptor == -1) { + throw std::runtime_error("Could not open file for reading: '" + + display_path(path) + "'"); + } + const ScopedReadHandle scoped_descriptor(descriptor); + + struct stat status{}; + if (::fstat(descriptor, &status) != 0 || !S_ISREG(status.st_mode)) { + throw std::runtime_error("Path is not a regular file: '" + + display_path(path) + "'"); + } + + std::array buffer{}; + while (true) { + const ssize_t bytes_read = ::read(descriptor, buffer.data(), buffer.size()); + if (bytes_read > 0) { + contents.append(buffer.data(), static_cast(bytes_read)); + continue; + } + if (bytes_read < 0 && errno == EINTR) { + continue; + } + if (bytes_read < 0) { + const int read_error = errno; + throw std::runtime_error( + "Could not read file: '" + display_path(path) + "': " + + std::error_code(read_error, std::generic_category()).message()); + } + break; + } +#endif + return contents; +} + +void write_file_atomically(const std::filesystem::path& path, + const AtomicFileWriter& writer, + bool create_parent_directories) { + std::error_code absolute_error; + const auto destination = std::filesystem::absolute(path, absolute_error); + if (absolute_error) { + throw std::runtime_error("Could not resolve absolute path for '" + + display_path(path) + + "': " + absolute_error.message()); + } + + if (create_parent_directories) { + ensure_parent_directory(destination); + } + + const auto parent = destination.parent_path(); + if (!parent.empty()) { + std::error_code error; + const bool parent_is_directory = + std::filesystem::is_directory(parent, error); + if (error || !parent_is_directory) { + throw std::runtime_error("Parent directory does not exist for '" + + display_path(path) + "'"); + } + } + + auto temporary_file = reserve_temporary_file(destination); + writer(temporary_file.path()); + temporary_file.verify_identity(); + preserve_permissions(temporary_file, destination); + temporary_file.verify_identity(); + replace_file(temporary_file.path(), destination); + temporary_file.release(); +} + +void write_text_file_atomically(const std::filesystem::path& path, + std::string_view contents, + bool create_parent_directories) { + write_file_atomically( + path, + [contents, &path](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path, + std::ios::binary | std::ios::trunc); + if (!output.is_open()) { + throw std::runtime_error( + "Could not open temporary file for writing " + "destination '" + + display_path(path) + "'"); + } + output.write(contents.data(), + static_cast(contents.size())); + output.close(); + if (!output) { + throw std::runtime_error("Could not write file: '" + + display_path(path) + "'"); + } + }, + create_parent_directories); +} + +} // namespace qdk::chemistry::utils diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp new file mode 100644 index 000000000..ec43ab149 --- /dev/null +++ b/cpp/tests/test_file_io.cpp @@ -0,0 +1,322 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE.txt in the project root for +// license information. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef _WIN32 +#include +#include +#include +#else +#ifndef NOMINMAX +#define NOMINMAX +#endif +#include +#endif + +namespace { + +class FileIoTest : public ::testing::Test { + protected: + void SetUp() override { + root_ = std::filesystem::temp_directory_path() / + ("qdk_file_io_test_" + + std::to_string( + std::chrono::steady_clock::now().time_since_epoch().count())); + std::filesystem::create_directories(root_); + } + + void TearDown() override { + std::error_code ignored; + std::filesystem::remove_all(root_, ignored); + } + + std::filesystem::path root_; +}; + +TEST_F(FileIoTest, WritesReadsAndReplacesText) { + const auto path = root_ / "data.txt"; + + qdk::chemistry::utils::write_text_file_atomically(path, "first"); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "first"); + + qdk::chemistry::utils::write_text_file_atomically(path, "second"); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "second"); +} + +TEST_F(FileIoTest, CreatesParentDirectoriesWhenRequested) { + const auto path = root_ / "nested" / "directory" / "data.txt"; + + qdk::chemistry::utils::write_text_file_atomically(path, "contents", true); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} + +TEST_F(FileIoTest, RejectsMissingParentDirectoryByDefault) { + const auto path = root_ / "missing" / "data.txt"; + + EXPECT_THROW( + qdk::chemistry::utils::write_text_file_atomically(path, "contents"), + std::runtime_error); + EXPECT_FALSE(std::filesystem::exists(path)); +} + +TEST_F(FileIoTest, PreservesDestinationWhenWriterFails) { + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + + EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( + path, + [](const std::filesystem::path& temporary_path) { + qdk::chemistry::utils::write_text_file_atomically( + temporary_path, "incomplete"); + throw std::runtime_error("writer failed"); + }), + std::runtime_error); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "original"); + EXPECT_EQ(std::distance(std::filesystem::directory_iterator(root_), + std::filesystem::directory_iterator()), + 1); +} + +TEST_F(FileIoTest, RejectsDirectoryReads) { + EXPECT_THROW(qdk::chemistry::utils::read_text_file(root_), + std::runtime_error); +} + +TEST_F(FileIoTest, PreservesDestinationPermissions) { +#ifndef _WIN32 + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + const auto private_permissions = std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write | + std::filesystem::perms::group_read; + std::filesystem::permissions(path, private_permissions, + std::filesystem::perm_options::replace); + + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"); + + EXPECT_EQ(std::filesystem::status(path).permissions(), private_permissions); +#endif +} + +TEST_F(FileIoTest, CreatesNewFilesWithOwnerOnlyPermissions) { +#ifndef _WIN32 + const auto path = root_ / "data.txt"; + + qdk::chemistry::utils::write_file_atomically( + path, [](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path); + output << "contents"; + output.close(); + std::filesystem::permissions(temporary_path, + std::filesystem::perms::all, + std::filesystem::perm_options::replace); + }); + + const auto owner_only_permissions = + std::filesystem::perms::owner_read | std::filesystem::perms::owner_write; + EXPECT_EQ(std::filesystem::status(path).permissions(), + owner_only_permissions); +#endif +} + +#ifndef _WIN32 +TEST_F(FileIoTest, DoesNotInheritTemporaryDescriptorAcrossExec) { + const auto path = root_ / "data.txt"; + + qdk::chemistry::utils::write_file_atomically( + path, [](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path); + output << "contents"; + output.close(); + + struct stat temporary_status{}; + ASSERT_EQ(::stat(temporary_path.c_str(), &temporary_status), 0); + const long system_limit = ::sysconf(_SC_OPEN_MAX); + const long descriptor_limit = + system_limit < 0 ? 1024 : std::min(system_limit, 1024); + for (int descriptor = 0; descriptor < descriptor_limit; ++descriptor) { + struct stat descriptor_status{}; + if (::fstat(descriptor, &descriptor_status) == 0 && + descriptor_status.st_dev == temporary_status.st_dev && + descriptor_status.st_ino == temporary_status.st_ino) { + EXPECT_NE(::fcntl(descriptor, F_GETFD) & FD_CLOEXEC, 0); + return; + } + } + FAIL() << "Reserved temporary descriptor not found"; + }); +} +#endif + +#ifdef _WIN32 +TEST_F(FileIoTest, ReplacesReadOnlyDestinationOnWindows) { + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + ASSERT_NE(SetFileAttributesW(path.c_str(), FILE_ATTRIBUTE_READONLY), 0); + + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "replacement"); + EXPECT_EQ(std::filesystem::status(path).permissions() & + std::filesystem::perms::owner_write, + std::filesystem::perms::none); +} + +TEST_F(FileIoTest, CleansUpReadOnlyTemporaryFileWhenWriterFails) { + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + + EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( + path, + [](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path); + output << "incomplete"; + output.close(); + ASSERT_NE(SetFileAttributesW(temporary_path.c_str(), + FILE_ATTRIBUTE_READONLY), + 0); + throw std::runtime_error("writer failed"); + }), + std::runtime_error); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "original"); + EXPECT_EQ(std::distance(std::filesystem::directory_iterator(root_), + std::filesystem::directory_iterator()), + 1); +} +#endif + +TEST_F(FileIoTest, PreservesDestinationSuffixesForWriter) { + const auto path = root_ / "data.structure.json"; + std::filesystem::path observed_temporary_path; + + qdk::chemistry::utils::write_file_atomically( + path, + [&observed_temporary_path](const std::filesystem::path& temporary_path) { + observed_temporary_path = temporary_path; + std::ofstream output(temporary_path, + std::ios::binary | std::ios::trunc); + output << "contents"; + }); + + EXPECT_EQ(observed_temporary_path.extension(), ".json"); + EXPECT_EQ(observed_temporary_path.stem().extension(), ".structure"); +} + +#ifndef _WIN32 +TEST_F(FileIoTest, PreservesLongDestinationSuffix) { + const auto path = root_ / ("x." + std::string(249, 'a')); + std::filesystem::path observed_temporary_path; + + qdk::chemistry::utils::write_file_atomically( + path, + [&observed_temporary_path](const std::filesystem::path& temporary_path) { + observed_temporary_path = temporary_path; + std::ofstream output(temporary_path); + output << "contents"; + }); + + EXPECT_EQ(observed_temporary_path.extension(), path.extension()); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); + EXPECT_EQ(std::distance(std::filesystem::directory_iterator(root_), + std::filesystem::directory_iterator()), + 1); +} +#endif + +TEST_F(FileIoTest, RejectsReplacedTemporaryFile) { + const auto path = root_ / "data.txt"; + + EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( + path, + [](const std::filesystem::path& temporary_path) { + std::filesystem::remove(temporary_path); + std::ofstream output(temporary_path); + output << "replacement"; + }), + std::runtime_error); + EXPECT_FALSE(std::filesystem::exists(path)); +} + +TEST_F(FileIoTest, FreezesRelativeDestinationBeforeWriterRuns) { + const auto original_directory = std::filesystem::current_path(); + const auto first_directory = root_ / "first"; + const auto second_directory = root_ / "second"; + std::filesystem::create_directories(first_directory); + std::filesystem::create_directories(second_directory); + std::filesystem::current_path(first_directory); + + try { + qdk::chemistry::utils::write_file_atomically( + "data.txt", + [&second_directory](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path); + output << "contents"; + output.close(); + std::filesystem::current_path(second_directory); + }); + } catch (...) { + std::filesystem::current_path(original_directory); + throw; + } + std::filesystem::current_path(original_directory); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(first_directory / "data.txt"), + "contents"); + EXPECT_FALSE(std::filesystem::exists(second_directory / "data.txt")); +} + +#ifndef _WIN32 +TEST_F(FileIoTest, PreservesSymlinkParentTraversalWhenFreezingPath) { + const auto original_directory = std::filesystem::current_path(); + const auto working_directory = root_ / "working"; + const auto target_parent = root_ / "target-parent"; + const auto target_directory = target_parent / "target"; + std::filesystem::create_directories(working_directory); + std::filesystem::create_directories(target_directory); + std::filesystem::create_directory_symlink(target_directory, + working_directory / "link"); + std::filesystem::current_path(working_directory); + + try { + qdk::chemistry::utils::write_text_file_atomically("link/../data.txt", + "contents"); + } catch (...) { + std::filesystem::current_path(original_directory); + throw; + } + std::filesystem::current_path(original_directory); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(target_parent / "data.txt"), + "contents"); + EXPECT_FALSE(std::filesystem::exists(working_directory / "data.txt")); +} +#endif + +TEST_F(FileIoTest, SupportsUnicodePaths) { +#ifdef _WIN32 + const std::filesystem::path filename = L"\u6570\u636e.txt"; +#else + const std::filesystem::path filename = "\xE6\x95\xB0\xE6\x8D\xAE.txt"; +#endif + const auto path = root_ / filename; + + qdk::chemistry::utils::write_text_file_atomically(path, "contents"); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} + +} // namespace diff --git a/python/src/qdk_chemistry/utils/__init__.py b/python/src/qdk_chemistry/utils/__init__.py index 29eb6f9c8..a4d1827bb 100644 --- a/python/src/qdk_chemistry/utils/__init__.py +++ b/python/src/qdk_chemistry/utils/__init__.py @@ -7,6 +7,12 @@ # Import C++ utilities from the compiled extension from qdk_chemistry._core.utils import Logger, compute_valence_space_parameters, rotate_orbitals from qdk_chemistry.utils.enum import CaseInsensitiveStrEnum +from qdk_chemistry.utils.file_io import ( + ensure_parent_directory, + read_text_file, + write_file_atomically, + write_text_file_atomically, +) from . import model_hamiltonians @@ -14,6 +20,10 @@ "CaseInsensitiveStrEnum", "Logger", "compute_valence_space_parameters", + "ensure_parent_directory", "model_hamiltonians", + "read_text_file", "rotate_orbitals", + "write_file_atomically", + "write_text_file_atomically", ] diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py new file mode 100644 index 000000000..d124e5118 --- /dev/null +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -0,0 +1,212 @@ +"""Cross-platform file and path helpers.""" + +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See LICENSE.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +from __future__ import annotations + +import errno +import os +import stat +from collections.abc import Callable +from pathlib import Path +from typing import TypeAlias + +PathLike: TypeAlias = str | os.PathLike[str] +AtomicFileWriter: TypeAlias = Callable[[Path], None] + +__all__ = [ + "AtomicFileWriter", + "PathLike", + "ensure_parent_directory", + "read_text_file", + "write_file_atomically", + "write_text_file_atomically", +] + + +def ensure_parent_directory(path: PathLike) -> None: + """Create the parent directory of *path* when it does not exist.""" + parent = Path(path).parent + if parent != Path("."): + parent.mkdir(parents=True, exist_ok=True) + + +def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: + """Read an entire text file without changing its line endings.""" + descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) + try: + if not stat.S_ISREG(os.fstat(descriptor).st_mode): + raise OSError(f"Path is not a regular file: '{path}'") + with os.fdopen(descriptor, "r", encoding=encoding, newline="", closefd=False) as stream: + return stream.read() + finally: + os.close(descriptor) + + +def write_file_atomically( + path: PathLike, + writer: AtomicFileWriter, + *, + create_parent_directories: bool = False, +) -> None: + """Write through a temporary sibling and atomically replace *path*. + + The writer receives a unique temporary path in the destination directory. + The path preserves the destination's suffixes for format-sensitive writers. + The temporary file is removed if the writer raises an exception. + + On POSIX, replacing an existing file preserves its permission bits and new + files are created with owner-only permissions. On Windows, replacement + preserves the read-only attribute and new files use the filesystem's + standard access controls. Other file-object metadata and hard-link identity + are not preserved. Atomic replacement prevents partial visibility but does + not guarantee durability after power loss. + + The destination's parent directory must not be writable by principals less + privileged than the process performing the write. + """ + destination = Path(path) + if not destination.is_absolute(): + destination = Path(os.path.abspath(destination)) if os.name == "nt" else Path.cwd() / destination + if create_parent_directories: + ensure_parent_directory(destination) + + parent = destination.parent + if not parent.is_dir(): + raise FileNotFoundError(f"Parent directory does not exist for '{destination}'") + + descriptor, temporary_name = _reserve_temporary_file(destination) + temporary_path = Path(temporary_name) + + try: + writer(temporary_path) + reserved_status = os.fstat(descriptor) + current_status = temporary_path.lstat() + if ( + reserved_status.st_dev != current_status.st_dev + or reserved_status.st_ino != current_status.st_ino + or not stat.S_ISREG(current_status.st_mode) + or current_status.st_nlink != 1 + ): + raise RuntimeError(f"Temporary file identity changed: '{temporary_path}'") + + try: + existing_mode = stat.S_IMODE(destination.stat().st_mode) + except FileNotFoundError: + destination_mode = None + if os.name != "nt": + os.fchmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) + else: + destination_mode = existing_mode + if os.name != "nt" and hasattr(os, "fchmod"): + os.fchmod(descriptor, existing_mode) + elif os.name != "nt": + temporary_path.chmod(existing_mode) + + if os.name == "nt": + os.close(descriptor) + descriptor = -1 + _replace_file(temporary_path, destination, destination_mode) + except BaseException as error: + if descriptor >= 0: + os.close(descriptor) + descriptor = -1 + try: + _remove_temporary_file(temporary_path) + except OSError as cleanup_error: + raise error from cleanup_error + raise + finally: + if descriptor >= 0: + os.close(descriptor) + + +def _reserve_temporary_file(destination: Path) -> tuple[int, str]: + """Reserve a private temporary sibling and keep its descriptor open.""" + suffix = "".join(destination.suffixes) + for _ in range(64): + temporary_path = destination.parent / f".qdk-tmp-{os.urandom(8).hex()}{suffix}" + try: + descriptor = os.open(temporary_path, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o600) + except FileExistsError: + continue + except OSError as error: + if error.errno != errno.ENAMETOOLONG: + raise + break + return descriptor, str(temporary_path) + + alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-" + for stem_length in range(16, 0, -1): + for attempt in range(64): + stem = "q" * (stem_length - 1) + alphabet[attempt] + temporary_path = destination.parent / f"{stem}{suffix}" + try: + descriptor = os.open(temporary_path, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o600) + except FileExistsError: + continue + except OSError as error: + if error.errno == errno.ENAMETOOLONG: + break + raise + return descriptor, str(temporary_path) + + raise FileExistsError(f"Could not create a unique temporary file beside '{destination}'") + + +def _remove_temporary_file(temporary_path: Path) -> None: + """Remove a temporary file, including a Windows read-only file.""" + try: + temporary_path.unlink(missing_ok=True) + except PermissionError: + if os.name != "nt": + raise + try: + mode = temporary_path.stat().st_mode + except FileNotFoundError: + return + temporary_path.chmod(mode | stat.S_IWRITE) + temporary_path.unlink(missing_ok=True) + + +def _replace_file(temporary_path: Path, destination: Path, destination_mode: int | None) -> None: + """Replace a destination, handling Windows read-only files.""" + try: + os.replace(temporary_path, destination) + return + except PermissionError: + read_only = os.name == "nt" and destination_mode is not None and destination_mode & stat.S_IWRITE == 0 + if not read_only: + raise + + assert destination_mode is not None + destination.chmod(destination_mode | stat.S_IWRITE) + try: + os.replace(temporary_path, destination) + except BaseException: + destination.chmod(destination_mode) + raise + destination.chmod(destination_mode) + + +def write_text_file_atomically( + path: PathLike, + contents: str, + *, + encoding: str = "utf-8", + create_parent_directories: bool = False, +) -> None: + """Write text through an atomic file replacement.""" + + def write_temporary_file(temporary_path: Path) -> None: + with temporary_path.open("w", encoding=encoding, newline="") as stream: + stream.write(contents) + + write_file_atomically( + path, + write_temporary_file, + create_parent_directories=create_parent_directories, + ) diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py new file mode 100644 index 000000000..0e86003bd --- /dev/null +++ b/python/tests/test_utils_file_io.py @@ -0,0 +1,231 @@ +"""Tests for cross-platform file helpers.""" + +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See LICENSE.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +import os +import stat +from pathlib import Path + +import pytest + +from qdk_chemistry.utils import ( + read_text_file, + write_file_atomically, + write_text_file_atomically, +) + + +def test_write_read_and_replace_text(tmp_path: Path): + path = tmp_path / "data.txt" + + write_text_file_atomically(path, "first") + assert read_text_file(path) == "first" + + write_text_file_atomically(path, "second") + assert read_text_file(path) == "second" + + +def test_create_parent_directories_when_requested(tmp_path: Path): + path = tmp_path / "nested" / "directory" / "data.txt" + + write_text_file_atomically(path, "contents", create_parent_directories=True) + + assert read_text_file(path) == "contents" + + +def test_reject_missing_parent_directory_by_default(tmp_path: Path): + path = tmp_path / "missing" / "data.txt" + + with pytest.raises(FileNotFoundError, match="Parent directory does not exist"): + write_text_file_atomically(path, "contents") + + assert not path.exists() + + +def test_preserve_destination_when_writer_fails(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + + def fail_after_write(temporary_path: Path) -> None: + temporary_path.write_text("incomplete", encoding="utf-8") + raise RuntimeError("writer failed") + + with pytest.raises(RuntimeError, match="writer failed"): + write_file_atomically(path, fail_after_write) + + assert read_text_file(path) == "original" + assert list(tmp_path.iterdir()) == [path] + + +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") +def test_clean_up_read_only_temporary_file_when_writer_fails(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + + def fail_after_making_temporary_file_read_only(temporary_path: Path) -> None: + temporary_path.write_text("incomplete", encoding="utf-8") + temporary_path.chmod(stat.S_IREAD) + raise RuntimeError("writer failed") + + with pytest.raises(RuntimeError, match="writer failed"): + write_file_atomically(path, fail_after_making_temporary_file_read_only) + + assert read_text_file(path) == "original" + assert list(tmp_path.iterdir()) == [path] + + +def test_preserve_line_endings(tmp_path: Path): + path = tmp_path / "data.txt" + contents = "a\r\nb\rc\nd" + + write_text_file_atomically(path, contents) + + assert read_text_file(path) == contents + + +def test_preserve_encoding_error(tmp_path: Path): + path = tmp_path / "data.txt" + path.write_text("contents", encoding="utf-8") + + with pytest.raises(LookupError): + read_text_file(path, encoding="not-a-real-codec") + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX special files") +def test_reject_non_regular_file(tmp_path: Path): + fifo = tmp_path / "data.fifo" + os.mkfifo(fifo) + + with pytest.raises(OSError, match="Path is not a regular file"): + read_text_file(fifo) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are not portable to Windows") +def test_preserve_destination_permissions(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + path.chmod(0o640) + + write_text_file_atomically(path, "replacement") + + assert stat.S_IMODE(path.stat().st_mode) == 0o640 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are not portable to Windows") +def test_create_new_file_with_owner_only_permissions(tmp_path: Path): + path = tmp_path / "data.txt" + + def write_and_relax_permissions(temporary_path: Path) -> None: + temporary_path.write_text("contents", encoding="utf-8") + temporary_path.chmod(0o666) + + write_file_atomically(path, write_and_relax_permissions) + + assert stat.S_IMODE(path.stat().st_mode) == 0o600 + + +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") +def test_replace_read_only_destination_on_windows(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + path.chmod(stat.S_IREAD) + + write_text_file_atomically(path, "replacement") + + assert read_text_file(path) == "replacement" + assert path.stat().st_mode & stat.S_IWRITE == 0 + + +def test_preserve_destination_suffixes_for_writer(tmp_path: Path): + path = tmp_path / "data.structure.json" + observed_temporary_path: Path | None = None + + def write_temporary_file(temporary_path: Path) -> None: + nonlocal observed_temporary_path + observed_temporary_path = temporary_path + temporary_path.write_text("contents", encoding="utf-8") + + write_file_atomically(path, write_temporary_file) + + assert observed_temporary_path is not None + assert observed_temporary_path.suffixes[-2:] == [".structure", ".json"] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX component length semantics") +def test_preserve_long_destination_suffix(tmp_path: Path): + path = tmp_path / f"x.{'a' * 249}" + observed_temporary_path: Path | None = None + + def write_temporary_file(temporary_path: Path) -> None: + nonlocal observed_temporary_path + observed_temporary_path = temporary_path + temporary_path.write_text("contents", encoding="utf-8") + + write_file_atomically(path, write_temporary_file) + + assert observed_temporary_path is not None + assert observed_temporary_path.suffix == path.suffix + assert read_text_file(path) == "contents" + assert list(tmp_path.iterdir()) == [path] + + +def test_reject_replaced_temporary_file(tmp_path: Path): + path = tmp_path / "data.txt" + + def replace_temporary_file(temporary_path: Path) -> None: + temporary_path.unlink() + temporary_path.write_text("replacement", encoding="utf-8") + + if os.name == "nt": + with pytest.raises(PermissionError): + write_file_atomically(path, replace_temporary_file) + else: + with pytest.raises(RuntimeError, match="Temporary file identity changed"): + write_file_atomically(path, replace_temporary_file) + + assert not path.exists() + assert list(tmp_path.iterdir()) == [] + + +def test_freeze_relative_destination_before_writer_runs(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + first_directory = tmp_path / "first" + second_directory = tmp_path / "second" + first_directory.mkdir() + second_directory.mkdir() + monkeypatch.chdir(first_directory) + + def change_directory(temporary_path: Path) -> None: + temporary_path.write_text("contents", encoding="utf-8") + os.chdir(second_directory) + + write_file_atomically("data.txt", change_directory) + + assert read_text_file(first_directory / "data.txt") == "contents" + assert not (second_directory / "data.txt").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink traversal semantics") +def test_preserve_symlink_parent_traversal_when_freezing_path(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + working_directory = tmp_path / "working" + target_parent = tmp_path / "target-parent" + target_directory = target_parent / "target" + working_directory.mkdir() + target_directory.mkdir(parents=True) + (working_directory / "link").symlink_to(target_directory, target_is_directory=True) + monkeypatch.chdir(working_directory) + + write_text_file_atomically("link/../data.txt", "contents") + + assert read_text_file(target_parent / "data.txt") == "contents" + assert not (working_directory / "data.txt").exists() + + +def test_support_unicode_paths(tmp_path: Path): + path = tmp_path / "data-\u6570\u636e.txt" + + write_text_file_atomically(path, "contents") + + assert read_text_file(path) == "contents" From 4bdeb7d91ddd41ee23f64f3e5bd34b3dbb162873 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Fri, 21 Aug 2026 19:14:29 -0700 Subject: [PATCH 02/23] Harden cross-platform file I/O --- cpp/include/qdk/chemistry/utils/file_io.hpp | 4 +- cpp/src/qdk/chemistry/utils/file_io.cpp | 113 +++++++++++++--- cpp/tests/test_file_io.cpp | 121 ++++++++++++++++-- python/src/qdk_chemistry/utils/file_io.py | 135 +++++++++++++++++--- python/tests/test_utils_file_io.py | 112 +++++++++++++++- 5 files changed, 436 insertions(+), 49 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index e7368f672..4a8552baf 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -44,8 +44,8 @@ std::string read_text_file(const std::filesystem::path& path); * are not preserved. Atomic replacement prevents partial visibility but does * not guarantee durability after power loss. * - * The destination's parent directory must not be writable by principals less - * privileged than the process performing the write. + * The destination's parent directory must not be readable or writable by + * principals less privileged than the process performing the write. * * @param path Destination path. * @param writer Function that writes the complete temporary file. diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 7cf77ff16..6d7c92df6 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -66,6 +66,14 @@ std::string display_path(const std::filesystem::path& path) { return {value.begin(), value.end()}; } +void validate_path(const std::filesystem::path& path) { + const auto& native_path = path.native(); + if (native_path.find(static_cast('\0')) != + std::filesystem::path::string_type::npos) { + throw std::invalid_argument("Path contains an embedded NUL character"); + } +} + std::filesystem::path make_temporary_path( const std::filesystem::path& destination) { static std::atomic counter{0}; @@ -158,8 +166,7 @@ class ReservedTemporaryFile { } ~ReservedTemporaryFile() { - close(); - if (cleanup_) { + if (cleanup_ && path_matches_identity()) { #ifdef _WIN32 const DWORD attributes = GetFileAttributesW(path_.c_str()); if (attributes != INVALID_FILE_ATTRIBUTES && @@ -172,6 +179,7 @@ class ReservedTemporaryFile { std::error_code ignored; std::filesystem::remove(path_, ignored); } + close(); } const std::filesystem::path& path() const { return path_; } @@ -258,6 +266,45 @@ class ReservedTemporaryFile { } #endif + bool path_matches_identity() const noexcept { + if (handle_ == invalid_handle()) { + return false; + } +#ifdef _WIN32 + BY_HANDLE_FILE_INFORMATION reserved_info{}; + if (!GetFileInformationByHandle(handle_, &reserved_info)) { + return false; + } + HANDLE current_handle = CreateFileW( + path_.c_str(), FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (current_handle == INVALID_HANDLE_VALUE) { + return false; + } + BY_HANDLE_FILE_INFORMATION current_info{}; + const bool inspected = + GetFileInformationByHandle(current_handle, ¤t_info) != 0; + CloseHandle(current_handle); + return inspected && + reserved_info.dwVolumeSerialNumber == + current_info.dwVolumeSerialNumber && + reserved_info.nFileIndexHigh == current_info.nFileIndexHigh && + reserved_info.nFileIndexLow == current_info.nFileIndexLow && + (current_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == + 0 && + current_info.nNumberOfLinks == 1; +#else + struct stat reserved_status{}; + struct stat current_status{}; + return ::fstat(handle_, &reserved_status) == 0 && + ::lstat(path_.c_str(), ¤t_status) == 0 && + reserved_status.st_dev == current_status.st_dev && + reserved_status.st_ino == current_status.st_ino && + S_ISREG(current_status.st_mode) && current_status.st_nlink == 1; +#endif + } + void close() { if (handle_ == invalid_handle()) { return; @@ -278,10 +325,9 @@ class ReservedTemporaryFile { ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, std::error_code& error) { #ifdef _WIN32 - HANDLE handle = - CreateFileW(path.c_str(), GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); + HANDLE handle = CreateFileW( + path.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); if (handle == INVALID_HANDLE_VALUE) { error = std::error_code(static_cast(GetLastError()), std::system_category()); @@ -309,6 +355,11 @@ ReservedTemporaryFile reserve_temporary_file( constexpr int max_attempts = 64; for (int attempt = 0; attempt < max_attempts; ++attempt) { const auto temporary_path = make_temporary_path(destination); +#ifdef _WIN32 + if (temporary_path.filename().native().size() > 255) { + break; + } +#endif std::error_code error; auto temporary_file = create_exclusive_file(temporary_path, error); if (!error) { @@ -328,6 +379,14 @@ ReservedTemporaryFile reserve_temporary_file( for (int attempt = 0; attempt < max_attempts; ++attempt) { const auto temporary_path = make_compact_temporary_path(destination, attempt, stem_length); + if (temporary_path == destination) { + continue; + } +#ifdef _WIN32 + if (temporary_path.filename().native().size() > 255) { + break; + } +#endif std::error_code error; auto temporary_file = create_exclusive_file(temporary_path, error); if (!error) { @@ -355,6 +414,15 @@ void replace_file(const std::filesystem::path& source, const DWORD original_attributes = GetFileAttributesW(destination.c_str()); const bool destination_exists = original_attributes != INVALID_FILE_ATTRIBUTES; + if (destination_exists && + !SetFileAttributesW(source.c_str(), + settable_file_attributes(original_attributes))) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + throw std::runtime_error("Could not prepare file attributes for '" + + display_path(destination) + + "': " + error.message()); + } auto move = [&]() { return MoveFileExW(source.c_str(), destination.c_str(), @@ -380,22 +448,25 @@ void replace_file(const std::filesystem::path& source, if (!move()) { const DWORD retry_error = GetLastError(); - SetFileAttributesW(destination.c_str(), - settable_file_attributes(original_attributes)); + const bool restored = + SetFileAttributesW(destination.c_str(), + settable_file_attributes(original_attributes)) != 0; + const DWORD restore_error = restored ? ERROR_SUCCESS : GetLastError(); const std::error_code error(static_cast(retry_error), std::system_category()); + if (!restored) { + const std::error_code rollback_error(static_cast(restore_error), + std::system_category()); + throw std::runtime_error("Could not replace file '" + + display_path(destination) + + "': " + error.message() + + "; could not restore original attributes: " + + rollback_error.message()); + } throw std::runtime_error("Could not replace file '" + display_path(destination) + "': " + error.message()); } - if (!SetFileAttributesW(destination.c_str(), - settable_file_attributes(original_attributes))) { - const std::error_code error(static_cast(GetLastError()), - std::system_category()); - throw std::runtime_error("Could not restore file attributes for '" + - display_path(destination) + - "': " + error.message()); - } #else std::error_code error; std::filesystem::rename(source, destination, error); @@ -410,7 +481,8 @@ void replace_file(const std::filesystem::path& source, void preserve_permissions(ReservedTemporaryFile& temporary_file, const std::filesystem::path& destination) { std::error_code status_error; - const auto status = std::filesystem::status(destination, status_error); + const auto status = + std::filesystem::symlink_status(destination, status_error); if (status_error) { if (status_error == std::errc::no_such_file_or_directory) { #ifndef _WIN32 @@ -430,6 +502,10 @@ void preserve_permissions(ReservedTemporaryFile& temporary_file, #endif return; } + if (std::filesystem::is_symlink(status)) { + throw std::runtime_error("Symlink destinations are not supported: '" + + display_path(destination) + "'"); + } temporary_file.set_permissions(status.permissions()); } @@ -437,6 +513,7 @@ void preserve_permissions(ReservedTemporaryFile& temporary_file, } // namespace void ensure_parent_directory(const std::filesystem::path& path) { + validate_path(path); const auto parent = path.parent_path(); if (parent.empty()) { return; @@ -451,6 +528,7 @@ void ensure_parent_directory(const std::filesystem::path& path) { } std::string read_text_file(const std::filesystem::path& path) { + validate_path(path); std::string contents; #ifdef _WIN32 HANDLE handle = @@ -526,6 +604,7 @@ std::string read_text_file(const std::filesystem::path& path) { void write_file_atomically(const std::filesystem::path& path, const AtomicFileWriter& writer, bool create_parent_directories) { + validate_path(path); std::error_code absolute_error; const auto destination = std::filesystem::absolute(path, absolute_error); if (absolute_error) { diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index ec43ab149..21cf589e3 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -71,6 +71,22 @@ TEST_F(FileIoTest, RejectsMissingParentDirectoryByDefault) { EXPECT_FALSE(std::filesystem::exists(path)); } +TEST_F(FileIoTest, RejectsEmbeddedNulPaths) { + const auto prefix = root_ / "data.txt"; + std::string path = prefix.string(); + path.append("\0ignored", 8); + const std::filesystem::path nul_path(path); + + EXPECT_THROW(qdk::chemistry::utils::ensure_parent_directory(nul_path), + std::invalid_argument); + EXPECT_THROW(qdk::chemistry::utils::read_text_file(nul_path), + std::invalid_argument); + EXPECT_THROW( + qdk::chemistry::utils::write_text_file_atomically(nul_path, "contents"), + std::invalid_argument); + EXPECT_FALSE(std::filesystem::exists(prefix)); +} + TEST_F(FileIoTest, PreservesDestinationWhenWriterFails) { const auto path = root_ / "data.txt"; qdk::chemistry::utils::write_text_file_atomically(path, "original"); @@ -78,8 +94,9 @@ TEST_F(FileIoTest, PreservesDestinationWhenWriterFails) { EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( path, [](const std::filesystem::path& temporary_path) { - qdk::chemistry::utils::write_text_file_atomically( - temporary_path, "incomplete"); + std::ofstream output(temporary_path); + output << "incomplete"; + output.close(); throw std::runtime_error("writer failed"); }), std::runtime_error); @@ -111,6 +128,27 @@ TEST_F(FileIoTest, PreservesDestinationPermissions) { #endif } +#ifndef _WIN32 +TEST_F(FileIoTest, RejectsSymlinkDestinationsWithoutCopyingReferentMode) { + const auto target = root_ / "target.txt"; + const auto link = root_ / "link.txt"; + qdk::chemistry::utils::write_text_file_atomically(target, "target"); + std::filesystem::permissions( + target, + std::filesystem::perms::owner_all | std::filesystem::perms::group_read | + std::filesystem::perms::set_uid | std::filesystem::perms::set_gid, + std::filesystem::perm_options::replace); + std::filesystem::create_symlink(target, link); + + EXPECT_THROW( + qdk::chemistry::utils::write_text_file_atomically(link, "replacement"), + std::runtime_error); + + EXPECT_TRUE(std::filesystem::is_symlink(link)); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(target), "target"); +} +#endif + TEST_F(FileIoTest, CreatesNewFilesWithOwnerOnlyPermissions) { #ifndef _WIN32 const auto path = root_ / "data.txt"; @@ -197,6 +235,47 @@ TEST_F(FileIoTest, CleansUpReadOnlyTemporaryFileWhenWriterFails) { std::filesystem::directory_iterator()), 1); } + +TEST_F(FileIoTest, PreservesWritableDestinationOnWindows) { + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + + qdk::chemistry::utils::write_file_atomically( + path, [](const std::filesystem::path& temporary_path) { + std::ofstream output(temporary_path); + output << "replacement"; + output.close(); + ASSERT_NE( + SetFileAttributesW(temporary_path.c_str(), FILE_ATTRIBUTE_READONLY), + 0); + }); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "replacement"); + EXPECT_NE(std::filesystem::status(path).permissions() & + std::filesystem::perms::owner_write, + std::filesystem::perms::none); +} + +TEST_F(FileIoTest, AllowsExclusiveWriterOnWindows) { + const auto path = root_ / "data.txt"; + + qdk::chemistry::utils::write_file_atomically( + path, [](const std::filesystem::path& temporary_path) { + HANDLE handle = + CreateFileW(temporary_path.c_str(), GENERIC_WRITE, 0, nullptr, + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + ASSERT_NE(handle, INVALID_HANDLE_VALUE); + constexpr char contents[] = "contents"; + DWORD written = 0; + EXPECT_NE(WriteFile(handle, contents, sizeof(contents) - 1, &written, + nullptr), + 0); + EXPECT_EQ(written, sizeof(contents) - 1); + CloseHandle(handle); + }); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} #endif TEST_F(FileIoTest, PreservesDestinationSuffixesForWriter) { @@ -235,20 +314,42 @@ TEST_F(FileIoTest, PreservesLongDestinationSuffix) { std::filesystem::directory_iterator()), 1); } + +TEST_F(FileIoTest, CompactTemporaryPathNeverAliasesDestination) { + const auto path = root_ / ("qqqq0." + std::string(249, 'a')); + std::filesystem::path observed_temporary_path; + bool destination_visible = false; + + qdk::chemistry::utils::write_file_atomically( + path, [&](const std::filesystem::path& temporary_path) { + observed_temporary_path = temporary_path; + destination_visible = std::filesystem::exists(path); + std::ofstream output(temporary_path); + output << "contents"; + }); + + EXPECT_NE(observed_temporary_path, path); + EXPECT_FALSE(destination_visible); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} #endif TEST_F(FileIoTest, RejectsReplacedTemporaryFile) { const auto path = root_ / "data.txt"; + std::filesystem::path replacement_path; - EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( - path, - [](const std::filesystem::path& temporary_path) { - std::filesystem::remove(temporary_path); - std::ofstream output(temporary_path); - output << "replacement"; - }), - std::runtime_error); + EXPECT_THROW( + qdk::chemistry::utils::write_file_atomically( + path, + [&replacement_path](const std::filesystem::path& temporary_path) { + replacement_path = temporary_path; + std::filesystem::remove(temporary_path); + std::ofstream output(temporary_path); + output << "replacement"; + }), + std::runtime_error); EXPECT_FALSE(std::filesystem::exists(path)); + EXPECT_TRUE(std::filesystem::exists(replacement_path)); } TEST_F(FileIoTest, FreezesRelativeDestinationBeforeWriterRuns) { diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index d124e5118..862550997 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -7,12 +7,16 @@ from __future__ import annotations +import ctypes import errno +import importlib import os import stat +import sys from collections.abc import Callable +from ctypes import wintypes from pathlib import Path -from typing import TypeAlias +from typing import TypeAlias, cast PathLike: TypeAlias = str | os.PathLike[str] AtomicFileWriter: TypeAlias = Callable[[Path], None] @@ -34,6 +38,13 @@ def ensure_parent_directory(path: PathLike) -> None: parent.mkdir(parents=True, exist_ok=True) +def _validate_destination_path(path: PathLike) -> None: + value = os.fspath(path) + separators = tuple(separator for separator in (os.sep, os.altsep) if separator) + if value.endswith(separators): + raise ValueError(f"Destination path must name a file: '{value}'") + + def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: """Read an entire text file without changing its line endings.""" descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) @@ -65,9 +76,10 @@ def write_file_atomically( are not preserved. Atomic replacement prevents partial visibility but does not guarantee durability after power loss. - The destination's parent directory must not be writable by principals less - privileged than the process performing the write. + The destination's parent directory must not be readable or writable by + principals less privileged than the process performing the write. """ + _validate_destination_path(path) destination = Path(path) if not destination.is_absolute(): destination = Path(os.path.abspath(destination)) if os.name == "nt" else Path.cwd() / destination @@ -80,26 +92,25 @@ def write_file_atomically( descriptor, temporary_name = _reserve_temporary_file(destination) temporary_path = Path(temporary_name) + reserved_status: os.stat_result | None = None try: writer(temporary_path) reserved_status = os.fstat(descriptor) current_status = temporary_path.lstat() - if ( - reserved_status.st_dev != current_status.st_dev - or reserved_status.st_ino != current_status.st_ino - or not stat.S_ISREG(current_status.st_mode) - or current_status.st_nlink != 1 - ): + if not _same_file_identity(reserved_status, current_status): raise RuntimeError(f"Temporary file identity changed: '{temporary_path}'") try: - existing_mode = stat.S_IMODE(destination.stat().st_mode) + destination_status = destination.lstat() except FileNotFoundError: destination_mode = None if os.name != "nt": os.fchmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) else: + if stat.S_ISLNK(destination_status.st_mode): + raise ValueError(f"Symlink destinations are not supported: '{destination}'") + existing_mode = stat.S_IMODE(destination_status.st_mode) destination_mode = existing_mode if os.name != "nt" and hasattr(os, "fchmod"): os.fchmod(descriptor, existing_mode) @@ -111,13 +122,19 @@ def write_file_atomically( descriptor = -1 _replace_file(temporary_path, destination, destination_mode) except BaseException as error: + if reserved_status is None and descriptor >= 0: + try: + reserved_status = os.fstat(descriptor) + except OSError: + reserved_status = None if descriptor >= 0: os.close(descriptor) descriptor = -1 - try: - _remove_temporary_file(temporary_path) - except OSError as cleanup_error: - raise error from cleanup_error + if reserved_status is not None and _temporary_path_matches(temporary_path, reserved_status): + try: + _remove_temporary_file(temporary_path) + except OSError as cleanup_error: + raise error from cleanup_error raise finally: if descriptor >= 0: @@ -129,8 +146,10 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: suffix = "".join(destination.suffixes) for _ in range(64): temporary_path = destination.parent / f".qdk-tmp-{os.urandom(8).hex()}{suffix}" + if _component_is_too_long(temporary_path): + break try: - descriptor = os.open(temporary_path, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o600) + descriptor = _create_exclusive_file(temporary_path) except FileExistsError: continue except OSError as error: @@ -144,8 +163,12 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: for attempt in range(64): stem = "q" * (stem_length - 1) + alphabet[attempt] temporary_path = destination.parent / f"{stem}{suffix}" + if temporary_path == destination: + continue + if _component_is_too_long(temporary_path): + break try: - descriptor = os.open(temporary_path, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o600) + descriptor = _create_exclusive_file(temporary_path) except FileExistsError: continue except OSError as error: @@ -157,6 +180,76 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: raise FileExistsError(f"Could not create a unique temporary file beside '{destination}'") +def _component_is_too_long(path: Path) -> bool: + if os.name != "nt": + return False + return len(path.name.encode("utf-16-le")) // 2 > 255 + + +def _create_exclusive_file(path: Path) -> int: + if sys.platform != "win32": + return os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY | os.O_CLOEXEC, 0o600) + + create_file = ctypes.WinDLL("kernel32", use_last_error=True).CreateFileW + create_file.argtypes = ( + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ) + create_file.restype = wintypes.HANDLE + handle = create_file( + str(path), + 0, + 0x00000001 | 0x00000002 | 0x00000004, + None, + 1, + 0x00000080, + None, + ) + if handle == wintypes.HANDLE(-1).value: + error = ctypes.get_last_error() + message = ctypes.FormatError(error) + if error in (80, 183): + raise FileExistsError(error, message, str(path)) + raise OSError(error, message, str(path)) + close_handle = ctypes.WinDLL("kernel32", use_last_error=True).CloseHandle + close_handle.argtypes = (wintypes.HANDLE,) + close_handle.restype = wintypes.BOOL + open_osfhandle = cast( + "Callable[[int, int], int]", + importlib.import_module("msvcrt").open_osfhandle, + ) + try: + return open_osfhandle( + cast("int", handle), + os.O_RDONLY | getattr(os, "O_NOINHERIT", 0), + ) + except BaseException: + close_handle(handle) + raise + + +def _same_file_identity(reserved_status: os.stat_result, current_status: os.stat_result) -> bool: + return ( + reserved_status.st_dev == current_status.st_dev + and reserved_status.st_ino == current_status.st_ino + and stat.S_ISREG(current_status.st_mode) + and current_status.st_nlink == 1 + ) + + +def _temporary_path_matches(temporary_path: Path, reserved_status: os.stat_result) -> bool: + try: + current_status = temporary_path.lstat() + except OSError: + return False + return _same_file_identity(reserved_status, current_status) + + def _remove_temporary_file(temporary_path: Path) -> None: """Remove a temporary file, including a Windows read-only file.""" try: @@ -174,6 +267,8 @@ def _remove_temporary_file(temporary_path: Path) -> None: def _replace_file(temporary_path: Path, destination: Path, destination_mode: int | None) -> None: """Replace a destination, handling Windows read-only files.""" + if os.name == "nt" and destination_mode is not None: + temporary_path.chmod(destination_mode) try: os.replace(temporary_path, destination) return @@ -186,10 +281,12 @@ def _replace_file(temporary_path: Path, destination: Path, destination_mode: int destination.chmod(destination_mode | stat.S_IWRITE) try: os.replace(temporary_path, destination) - except BaseException: - destination.chmod(destination_mode) + except BaseException as replace_error: + try: + destination.chmod(destination_mode) + except OSError as rollback_error: + raise replace_error from rollback_error raise - destination.chmod(destination_mode) def write_text_file_atomically( diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 0e86003bd..6be2f7e2a 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -5,8 +5,10 @@ # Licensed under the MIT License. See LICENSE.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import ctypes import os import stat +from ctypes import wintypes from pathlib import Path import pytest @@ -45,6 +47,15 @@ def test_reject_missing_parent_directory_by_default(tmp_path: Path): assert not path.exists() +def test_reject_trailing_separator_destination(tmp_path: Path): + path = tmp_path / "data" + + with pytest.raises(ValueError, match="must name a file"): + write_text_file_atomically(f"{path}{os.sep}", "contents") + + assert not path.exists() + + def test_preserve_destination_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" write_text_file_atomically(path, "original") @@ -114,6 +125,21 @@ def test_preserve_destination_permissions(tmp_path: Path): assert stat.S_IMODE(path.stat().st_mode) == 0o640 +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink semantics") +def test_reject_symlink_destination_without_copying_referent_mode(tmp_path: Path): + target = tmp_path / "target.txt" + link = tmp_path / "link.txt" + target.write_text("target", encoding="utf-8") + target.chmod(0o6755) + link.symlink_to(target) + + with pytest.raises(ValueError, match="Symlink destinations are not supported"): + write_text_file_atomically(link, "replacement") + + assert link.is_symlink() + assert target.read_text(encoding="utf-8") == "target" + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are not portable to Windows") def test_create_new_file_with_owner_only_permissions(tmp_path: Path): path = tmp_path / "data.txt" @@ -139,6 +165,66 @@ def test_replace_read_only_destination_on_windows(tmp_path: Path): assert path.stat().st_mode & stat.S_IWRITE == 0 +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") +def test_preserve_writable_destination_on_windows(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + + def write_read_only_temporary_file(temporary_path: Path) -> None: + temporary_path.write_text("replacement", encoding="utf-8") + temporary_path.chmod(stat.S_IREAD) + + write_file_atomically(path, write_read_only_temporary_file) + + assert read_text_file(path) == "replacement" + assert path.stat().st_mode & stat.S_IWRITE != 0 + + +@pytest.mark.skipif(os.name != "nt", reason="Windows file-sharing behavior") +def test_allow_exclusive_writer_on_windows(tmp_path: Path): + path = tmp_path / "data.txt" + + def write_with_exclusive_handle(temporary_path: Path) -> None: + create_file = ctypes.WinDLL("kernel32", use_last_error=True).CreateFileW + create_file.argtypes = ( + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ) + create_file.restype = wintypes.HANDLE + handle = create_file( + str(temporary_path), + 0x40000000, + 0, + None, + 3, + 0x00000080, + None, + ) + assert handle != wintypes.HANDLE(-1).value + try: + written = wintypes.DWORD() + contents = ctypes.create_string_buffer(b"contents") + assert ctypes.windll.kernel32.WriteFile( + handle, + contents, + len(contents.value), + ctypes.byref(written), + None, + ) + assert written.value == len(contents.value) + finally: + ctypes.windll.kernel32.CloseHandle(handle) + + write_file_atomically(path, write_with_exclusive_handle) + + assert read_text_file(path) == "contents" + + def test_preserve_destination_suffixes_for_writer(tmp_path: Path): path = tmp_path / "data.structure.json" observed_temporary_path: Path | None = None @@ -172,10 +258,32 @@ def write_temporary_file(temporary_path: Path) -> None: assert list(tmp_path.iterdir()) == [path] +@pytest.mark.skipif(os.name == "nt", reason="POSIX component length semantics") +def test_compact_temporary_path_never_aliases_destination(tmp_path: Path): + path = tmp_path / f"qqqq0.{'a' * 249}" + observed_temporary_path: Path | None = None + destination_visible = False + + def write_temporary_file(temporary_path: Path) -> None: + nonlocal destination_visible, observed_temporary_path + observed_temporary_path = temporary_path + destination_visible = path.exists() + temporary_path.write_text("contents", encoding="utf-8") + + write_file_atomically(path, write_temporary_file) + + assert observed_temporary_path != path + assert not destination_visible + assert read_text_file(path) == "contents" + + def test_reject_replaced_temporary_file(tmp_path: Path): path = tmp_path / "data.txt" + replacement_path: Path | None = None def replace_temporary_file(temporary_path: Path) -> None: + nonlocal replacement_path + replacement_path = temporary_path temporary_path.unlink() temporary_path.write_text("replacement", encoding="utf-8") @@ -187,7 +295,9 @@ def replace_temporary_file(temporary_path: Path) -> None: write_file_atomically(path, replace_temporary_file) assert not path.exists() - assert list(tmp_path.iterdir()) == [] + if os.name != "nt": + assert replacement_path is not None + assert replacement_path.read_text(encoding="utf-8") == "replacement" def test_freeze_relative_destination_before_writer_runs(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): From 2694a44880cf27cc60b7e02e1d7196f6958c7c24 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Fri, 21 Aug 2026 20:03:23 -0700 Subject: [PATCH 03/23] Address FileIO review findings --- cpp/include/qdk/chemistry/utils/file_io.hpp | 13 +- cpp/src/qdk/chemistry/utils/file_io.cpp | 63 ++++++++-- cpp/tests/test_file_io.cpp | 118 ++++++++++++++++++ python/src/qdk_chemistry/utils/file_io.py | 113 +++++++++++++---- python/tests/test_utils_file_io.py | 130 ++++++++++++++++++++ 5 files changed, 402 insertions(+), 35 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index 4a8552baf..1099e8896 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -37,12 +37,13 @@ std::string read_text_file(const std::filesystem::path& path); * temporary path preserves the destination's suffixes for format-sensitive * writers. * - * On POSIX, replacing an existing file preserves its permission bits and new - * files are created with owner-only permissions. On Windows, replacement - * preserves the read-only attribute and new files use the filesystem's - * standard access controls. Other file-object metadata and hard-link identity - * are not preserved. Atomic replacement prevents partial visibility but does - * not guarantee durability after power loss. + * On POSIX, replacing an existing file preserves its ordinary read, write, and + * execute permission bits. New files are created with owner-only permissions. + * On Windows, replacement preserves the read-only attribute and new files use + * the filesystem's standard access controls. Other file-object metadata and + * hard-link identity are not preserved. Atomic replacement prevents partial + * visibility but does not guarantee durability after power loss. + * Windows alternate data streams are not supported. * * The destination's parent directory must not be readable or writable by * principals less privileged than the process performing the write. diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 6d7c92df6..d52a75ff9 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -72,6 +72,18 @@ void validate_path(const std::filesystem::path& path) { std::filesystem::path::string_type::npos) { throw std::invalid_argument("Path contains an embedded NUL character"); } + if (path.filename().empty()) { + throw std::invalid_argument("Path must name a file"); + } +#ifdef _WIN32 + const auto root_name_length = path.root_name().native().size(); + if (native_path.find(static_cast(':'), + root_name_length) != + std::filesystem::path::string_type::npos) { + throw std::invalid_argument( + "Windows alternate data streams are not supported"); + } +#endif } std::filesystem::path make_temporary_path( @@ -166,7 +178,7 @@ class ReservedTemporaryFile { } ~ReservedTemporaryFile() { - if (cleanup_ && path_matches_identity()) { + if (cleanup_ && has_same_identity(path_)) { #ifdef _WIN32 const DWORD attributes = GetFileAttributesW(path_.c_str()); if (attributes != INVALID_FILE_ATTRIBUTES && @@ -183,6 +195,9 @@ class ReservedTemporaryFile { } const std::filesystem::path& path() const { return path_; } + bool has_same_identity(const std::filesystem::path& path) const noexcept { + return path_matches_identity(path); + } void verify_identity() const { #ifdef _WIN32 @@ -266,7 +281,7 @@ class ReservedTemporaryFile { } #endif - bool path_matches_identity() const noexcept { + bool path_matches_identity(const std::filesystem::path& path) const noexcept { if (handle_ == invalid_handle()) { return false; } @@ -276,7 +291,7 @@ class ReservedTemporaryFile { return false; } HANDLE current_handle = CreateFileW( - path_.c_str(), FILE_READ_ATTRIBUTES, + path.c_str(), FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); if (current_handle == INVALID_HANDLE_VALUE) { @@ -298,7 +313,7 @@ class ReservedTemporaryFile { struct stat reserved_status{}; struct stat current_status{}; return ::fstat(handle_, &reserved_status) == 0 && - ::lstat(path_.c_str(), ¤t_status) == 0 && + ::lstat(path.c_str(), ¤t_status) == 0 && reserved_status.st_dev == current_status.st_dev && reserved_status.st_ino == current_status.st_ino && S_ISREG(current_status.st_mode) && current_status.st_nlink == 1; @@ -340,6 +355,11 @@ ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, error = std::error_code(errno, std::generic_category()); return {path, ReservedTemporaryFile::invalid_handle()}; } + if (::fchmod(descriptor, S_IRUSR | S_IWUSR) != 0) { + const int permission_error = errno; + error = std::error_code(permission_error, std::generic_category()); + return {path, descriptor}; + } #endif return {path, #ifdef _WIN32 @@ -350,6 +370,28 @@ ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, }; } +bool is_name_too_long(const std::error_code& error, + const std::filesystem::path& destination) { + if (error == std::errc::filename_too_long) { + return true; + } +#ifdef _WIN32 + if (error.value() == ERROR_FILENAME_EXCED_RANGE || + error.value() == ERROR_BUFFER_OVERFLOW) { + return true; + } + if (error.value() == ERROR_PATH_NOT_FOUND) { + std::error_code parent_error; + return std::filesystem::is_directory(destination.parent_path(), + parent_error) && + !parent_error; + } +#else + static_cast(destination); +#endif + return false; +} + ReservedTemporaryFile reserve_temporary_file( const std::filesystem::path& destination) { constexpr int max_attempts = 64; @@ -363,10 +405,13 @@ ReservedTemporaryFile reserve_temporary_file( std::error_code error; auto temporary_file = create_exclusive_file(temporary_path, error); if (!error) { + if (temporary_file.has_same_identity(destination)) { + continue; + } return temporary_file; } if (error != std::errc::file_exists) { - if (error == std::errc::filename_too_long) { + if (is_name_too_long(error, destination)) { break; } throw std::runtime_error("Could not create temporary file beside '" + @@ -390,12 +435,15 @@ ReservedTemporaryFile reserve_temporary_file( std::error_code error; auto temporary_file = create_exclusive_file(temporary_path, error); if (!error) { + if (temporary_file.has_same_identity(destination)) { + continue; + } return temporary_file; } if (error == std::errc::file_exists) { continue; } - if (error == std::errc::filename_too_long) { + if (is_name_too_long(error, destination)) { break; } throw std::runtime_error("Could not create temporary file beside '" + @@ -507,7 +555,8 @@ void preserve_permissions(ReservedTemporaryFile& temporary_file, display_path(destination) + "'"); } - temporary_file.set_permissions(status.permissions()); + temporary_file.set_permissions(status.permissions() & + std::filesystem::perms::all); } } // namespace diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index 21cf589e3..fb49fbc0c 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -71,6 +71,23 @@ TEST_F(FileIoTest, RejectsMissingParentDirectoryByDefault) { EXPECT_FALSE(std::filesystem::exists(path)); } +TEST_F(FileIoTest, RejectsTrailingSeparatorBeforeWriterRuns) { + const auto directory = root_ / "directory"; + const auto trailing_path = directory / ""; + std::filesystem::create_directory(directory); + bool writer_ran = false; + + EXPECT_THROW( + qdk::chemistry::utils::write_file_atomically( + trailing_path, + [&writer_ran](const std::filesystem::path&) { writer_ran = true; }), + std::invalid_argument); + EXPECT_THROW(qdk::chemistry::utils::ensure_parent_directory(trailing_path), + std::invalid_argument); + + EXPECT_FALSE(writer_ran); +} + TEST_F(FileIoTest, RejectsEmbeddedNulPaths) { const auto prefix = root_ / "data.txt"; std::string path = prefix.string(); @@ -128,6 +145,44 @@ TEST_F(FileIoTest, PreservesDestinationPermissions) { #endif } +TEST_F(FileIoTest, ClearsSpecialPermissionBitsOnReplacement) { +#ifndef _WIN32 + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + const auto permissions = + std::filesystem::perms::owner_all | std::filesystem::perms::group_read | + std::filesystem::perms::group_exec | std::filesystem::perms::others_read | + std::filesystem::perms::others_exec | std::filesystem::perms::set_uid | + std::filesystem::perms::set_gid | std::filesystem::perms::sticky_bit; + std::filesystem::permissions(path, permissions, + std::filesystem::perm_options::replace); + + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"); + + EXPECT_EQ(std::filesystem::status(path).permissions(), + permissions & std::filesystem::perms::all); +#endif +} + +TEST_F(FileIoTest, RestrictiveUmaskDoesNotPreventWriting) { +#ifndef _WIN32 + const auto path = root_ / "data.txt"; + const mode_t original_umask = ::umask(0777); + try { + qdk::chemistry::utils::write_text_file_atomically(path, "contents"); + } catch (...) { + ::umask(original_umask); + throw; + } + ::umask(original_umask); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); + EXPECT_EQ( + std::filesystem::status(path).permissions(), + std::filesystem::perms::owner_read | std::filesystem::perms::owner_write); +#endif +} + #ifndef _WIN32 TEST_F(FileIoTest, RejectsSymlinkDestinationsWithoutCopyingReferentMode) { const auto target = root_ / "target.txt"; @@ -276,6 +331,36 @@ TEST_F(FileIoTest, AllowsExclusiveWriterOnWindows) { EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); } + +TEST_F(FileIoTest, RejectsAlternateDataStreamsOnWindows) { + const auto path = root_ / "data.txt:stream"; + bool writer_ran = false; + + EXPECT_THROW( + qdk::chemistry::utils::write_file_atomically( + path, + [&writer_ran](const std::filesystem::path&) { writer_ran = true; }), + std::invalid_argument); + + EXPECT_FALSE(writer_ran); +} + +TEST_F(FileIoTest, FallsBackForNearMaxPathDestinationOnWindows) { + auto parent = root_; + while ((parent / "d.txt").native().size() < 220) { + parent /= "segment123"; + } + const auto current_length = (parent / "d.txt").native().size(); + if (current_length < 244) { + parent /= std::wstring(243 - current_length, L'p'); + } + std::filesystem::create_directories(parent); + const auto path = parent / "d.txt"; + + qdk::chemistry::utils::write_text_file_atomically(path, "contents"); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} #endif TEST_F(FileIoTest, PreservesDestinationSuffixesForWriter) { @@ -334,6 +419,39 @@ TEST_F(FileIoTest, CompactTemporaryPathNeverAliasesDestination) { } #endif +TEST_F(FileIoTest, CompactTemporaryPathUsesDistinctFilesystemIdentity) { + const auto case_probe = root_ / "QdkCaseProbe"; + { + std::ofstream output(case_probe); + output << "probe"; + } + if (!std::filesystem::exists(root_ / "qdkcaseprobe")) { + GTEST_SKIP() << "Filesystem is case-sensitive"; + } + std::filesystem::remove(case_probe); + + const auto path = + root_ / ("Q" + std::string(14, 'q') + "0." + std::string(230, 'a')); + { + std::ofstream output(path); + if (!output.is_open()) { + GTEST_SKIP() << "Filesystem does not support the long test path"; + } + } + std::filesystem::remove(path); + bool destination_visible = false; + + qdk::chemistry::utils::write_file_atomically( + path, [&](const std::filesystem::path& temporary_path) { + destination_visible = std::filesystem::exists(path); + std::ofstream output(temporary_path); + output << "contents"; + }); + + EXPECT_FALSE(destination_visible); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} + TEST_F(FileIoTest, RejectsReplacedTemporaryFile) { const auto path = root_ / "data.txt"; std::filesystem::path replacement_path; diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 862550997..0286e2327 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -10,6 +10,7 @@ import ctypes import errno import importlib +import ntpath import os import stat import sys @@ -33,6 +34,7 @@ def ensure_parent_directory(path: PathLike) -> None: """Create the parent directory of *path* when it does not exist.""" + _validate_destination_path(path) parent = Path(path).parent if parent != Path("."): parent.mkdir(parents=True, exist_ok=True) @@ -43,11 +45,21 @@ def _validate_destination_path(path: PathLike) -> None: separators = tuple(separator for separator in (os.sep, os.altsep) if separator) if value.endswith(separators): raise ValueError(f"Destination path must name a file: '{value}'") + if sys.platform == "win32" and ":" in ntpath.splitdrive(value)[1]: + raise ValueError(f"Windows alternate data streams are not supported: '{value}'") def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: """Read an entire text file without changing its line endings.""" - descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) + _validate_destination_path(path) + if sys.platform == "win32": + descriptor = _open_windows_file( + path, + desired_access=0x80000000, + creation_disposition=3, + ) + else: + descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) try: if not stat.S_ISREG(os.fstat(descriptor).st_mode): raise OSError(f"Path is not a regular file: '{path}'") @@ -69,12 +81,13 @@ def write_file_atomically( The path preserves the destination's suffixes for format-sensitive writers. The temporary file is removed if the writer raises an exception. - On POSIX, replacing an existing file preserves its permission bits and new - files are created with owner-only permissions. On Windows, replacement - preserves the read-only attribute and new files use the filesystem's - standard access controls. Other file-object metadata and hard-link identity - are not preserved. Atomic replacement prevents partial visibility but does - not guarantee durability after power loss. + On POSIX, replacing an existing file preserves its ordinary read, write, + and execute permission bits. New files are created with owner-only + permissions. On Windows, replacement preserves the read-only attribute and + new files use the filesystem's standard access controls. Other file-object + metadata and hard-link identity are not preserved. Atomic replacement + prevents partial visibility but does not guarantee durability after power + loss. The destination's parent directory must not be readable or writable by principals less privileged than the process performing the write. @@ -110,7 +123,7 @@ def write_file_atomically( else: if stat.S_ISLNK(destination_status.st_mode): raise ValueError(f"Symlink destinations are not supported: '{destination}'") - existing_mode = stat.S_IMODE(destination_status.st_mode) + existing_mode = stat.S_IMODE(destination_status.st_mode) & 0o777 destination_mode = existing_mode if os.name != "nt" and hasattr(os, "fchmod"): os.fchmod(descriptor, existing_mode) @@ -149,14 +162,15 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: if _component_is_too_long(temporary_path): break try: - descriptor = _create_exclusive_file(temporary_path) + descriptor = _reserve_distinct_temporary_file(destination, temporary_path) except FileExistsError: continue except OSError as error: - if error.errno != errno.ENAMETOOLONG: + if not _is_name_too_long(error, destination): raise break - return descriptor, str(temporary_path) + if descriptor is not None: + return descriptor, str(temporary_path) alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-" for stem_length in range(16, 0, -1): @@ -168,14 +182,15 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: if _component_is_too_long(temporary_path): break try: - descriptor = _create_exclusive_file(temporary_path) + descriptor = _reserve_distinct_temporary_file(destination, temporary_path) except FileExistsError: continue except OSError as error: - if error.errno == errno.ENAMETOOLONG: + if _is_name_too_long(error, destination): break raise - return descriptor, str(temporary_path) + if descriptor is not None: + return descriptor, str(temporary_path) raise FileExistsError(f"Could not create a unique temporary file beside '{destination}'") @@ -188,7 +203,33 @@ def _component_is_too_long(path: Path) -> bool: def _create_exclusive_file(path: Path) -> int: if sys.platform != "win32": - return os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY | os.O_CLOEXEC, 0o600) + descriptor = os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY | os.O_CLOEXEC, 0o600) + try: + os.fchmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) + except BaseException as error: + try: + reserved_status = os.fstat(descriptor) + except OSError: + reserved_status = None + finally: + os.close(descriptor) + if reserved_status is not None and _temporary_path_matches(path, reserved_status): + try: + path.unlink() + except OSError as cleanup_error: + raise error from cleanup_error + raise + return descriptor + + return _open_windows_file(path, desired_access=0, creation_disposition=1) + + +def _open_windows_file( + path: PathLike, + *, + desired_access: int, + creation_disposition: int, +) -> int: create_file = ctypes.WinDLL("kernel32", use_last_error=True).CreateFileW create_file.argtypes = ( @@ -202,20 +243,17 @@ def _create_exclusive_file(path: Path) -> int: ) create_file.restype = wintypes.HANDLE handle = create_file( - str(path), - 0, + os.fspath(path), + desired_access, 0x00000001 | 0x00000002 | 0x00000004, None, - 1, + creation_disposition, 0x00000080, None, ) if handle == wintypes.HANDLE(-1).value: error = ctypes.get_last_error() - message = ctypes.FormatError(error) - if error in (80, 183): - raise FileExistsError(error, message, str(path)) - raise OSError(error, message, str(path)) + raise _windows_error(path, error) close_handle = ctypes.WinDLL("kernel32", use_last_error=True).CloseHandle close_handle.argtypes = (wintypes.HANDLE,) close_handle.restype = wintypes.BOOL @@ -233,6 +271,37 @@ def _create_exclusive_file(path: Path) -> int: raise +def _windows_error(path: PathLike, error: int) -> OSError: + return OSError(0, ctypes.FormatError(error), os.fspath(path), error) + + +def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> int | None: + descriptor = _create_exclusive_file(temporary_path) + try: + reserved_status = os.fstat(descriptor) + except BaseException: + os.close(descriptor) + raise + if not _temporary_path_matches(destination, reserved_status): + return descriptor + + os.close(descriptor) + if _temporary_path_matches(temporary_path, reserved_status): + _remove_temporary_file(temporary_path) + return None + + +def _is_name_too_long(error: OSError, destination: Path) -> bool: + if error.errno == errno.ENAMETOOLONG: + return True + if sys.platform != "win32": + return False + winerror = getattr(error, "winerror", None) + if winerror in (111, 206): + return True + return winerror == 3 and destination.parent.is_dir() + + def _same_file_identity(reserved_status: os.stat_result, current_status: os.stat_result) -> bool: return ( reserved_status.st_dev == current_status.st_dev diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 6be2f7e2a..de8233a67 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -8,16 +8,19 @@ import ctypes import os import stat +import threading from ctypes import wintypes from pathlib import Path import pytest from qdk_chemistry.utils import ( + ensure_parent_directory, read_text_file, write_file_atomically, write_text_file_atomically, ) +from qdk_chemistry.utils import file_io as file_io_module def test_write_read_and_replace_text(tmp_path: Path): @@ -56,6 +59,16 @@ def test_reject_trailing_separator_destination(tmp_path: Path): assert not path.exists() +def test_reject_trailing_separator_in_all_path_helpers(tmp_path: Path): + path = tmp_path / "data" + trailing_path = f"{path}{os.sep}" + + with pytest.raises(ValueError, match="must name a file"): + ensure_parent_directory(trailing_path) + with pytest.raises(ValueError, match="must name a file"): + read_text_file(trailing_path) + + def test_preserve_destination_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" write_text_file_atomically(path, "original") @@ -125,6 +138,30 @@ def test_preserve_destination_permissions(tmp_path: Path): assert stat.S_IMODE(path.stat().st_mode) == 0o640 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are not portable to Windows") +def test_clear_special_permission_bits_on_replacement(tmp_path: Path): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + path.chmod(0o7755) + + write_text_file_atomically(path, "replacement") + + assert stat.S_IMODE(path.stat().st_mode) == 0o755 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX umask semantics") +def test_restrictive_umask_does_not_prevent_writing(tmp_path: Path): + path = tmp_path / "data.txt" + original_umask = os.umask(0o777) + try: + write_text_file_atomically(path, "contents") + finally: + os.umask(original_umask) + + assert read_text_file(path) == "contents" + assert stat.S_IMODE(path.stat().st_mode) == 0o600 + + @pytest.mark.skipif(os.name == "nt", reason="POSIX symlink semantics") def test_reject_symlink_destination_without_copying_referent_mode(tmp_path: Path): target = tmp_path / "target.txt" @@ -225,6 +262,74 @@ def write_with_exclusive_handle(temporary_path: Path) -> None: assert read_text_file(path) == "contents" +@pytest.mark.skipif(os.name != "nt", reason="Windows file-sharing behavior") +def test_reader_does_not_block_atomic_replacement_on_windows( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + reader_opened = threading.Event() + release_reader = threading.Event() + original_fdopen = file_io_module.os.fdopen + + def blocking_fdopen(*args, **kwargs): + stream = original_fdopen(*args, **kwargs) + reader_opened.set() + assert release_reader.wait(timeout=10) + return stream + + monkeypatch.setattr(file_io_module.os, "fdopen", blocking_fdopen) + result: list[str] = [] + reader = threading.Thread(target=lambda: result.append(read_text_file(path))) + reader.start() + assert reader_opened.wait(timeout=10) + try: + write_text_file_atomically(path, "replacement") + finally: + release_reader.set() + reader.join(timeout=10) + + assert result == ["original"] + assert read_text_file(path) == "replacement" + + +@pytest.mark.skipif(os.name != "nt", reason="Windows path semantics") +def test_reject_alternate_data_stream_destination_on_windows(tmp_path: Path): + path = tmp_path / "data.txt:stream" + + with pytest.raises(ValueError, match="alternate data streams"): + write_text_file_atomically(path, "contents") + + assert not (tmp_path / "data.txt").exists() + + +@pytest.mark.skipif(os.name != "nt", reason="Windows path semantics") +def test_fall_back_for_near_max_path_destination_on_windows(tmp_path: Path): + parent = tmp_path + while len(str(parent / "d.txt")) < 220: + parent /= "segment123" + current_length = len(str(parent / "d.txt")) + if current_length < 244: + parent /= "p" * (243 - current_length) + parent.mkdir(parents=True) + path = parent / "d.txt" + + write_text_file_atomically(path, "contents") + + assert read_text_file(path) == "contents" + + +@pytest.mark.skipif(os.name != "nt", reason="Windows error semantics") +def test_windows_errors_preserve_winerror_and_subclass(tmp_path: Path): + permission_error = file_io_module._windows_error(tmp_path / "data.txt", 5) + length_error = file_io_module._windows_error(tmp_path / "data.txt", 206) + + assert isinstance(permission_error, PermissionError) + assert permission_error.winerror == 5 + assert file_io_module._is_name_too_long(length_error, tmp_path / "data.txt") + + def test_preserve_destination_suffixes_for_writer(tmp_path: Path): path = tmp_path / "data.structure.json" observed_temporary_path: Path | None = None @@ -277,6 +382,31 @@ def write_temporary_file(temporary_path: Path) -> None: assert read_text_file(path) == "contents" +def test_compact_temporary_path_uses_distinct_filesystem_identity(tmp_path: Path): + case_probe = tmp_path / "QdkCaseProbe" + case_probe.write_text("probe", encoding="utf-8") + if not (tmp_path / "qdkcaseprobe").exists(): + pytest.skip("Filesystem is case-sensitive") + case_probe.unlink() + path = tmp_path / f"Q{'q' * 14}0.{'a' * 230}" + try: + path.touch() + except OSError: + pytest.skip("Filesystem does not support the long test path") + path.unlink() + destination_visible = False + + def write_temporary_file(temporary_path: Path) -> None: + nonlocal destination_visible + destination_visible = path.exists() + temporary_path.write_text("contents", encoding="utf-8") + + write_file_atomically(path, write_temporary_file) + + assert not destination_visible + assert read_text_file(path) == "contents" + + def test_reject_replaced_temporary_file(tmp_path: Path): path = tmp_path / "data.txt" replacement_path: Path | None = None From 79d53083572bfb356ba933945eb6b0d2910d9073 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Fri, 21 Aug 2026 21:29:21 -0700 Subject: [PATCH 04/23] Complete FileIO review remediation --- cpp/include/qdk/chemistry/utils/file_io.hpp | 21 ++- cpp/src/qdk/chemistry/utils/file_io.cpp | 196 ++++++++++++++++---- cpp/tests/test_file_io.cpp | 76 ++++++++ python/src/qdk_chemistry/utils/file_io.py | 188 ++++++++++++++++--- python/tests/test_utils_file_io.py | 93 ++++++++++ 5 files changed, 500 insertions(+), 74 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index 1099e8896..fafba7551 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -31,22 +31,29 @@ std::string read_text_file(const std::filesystem::path& path); * @brief Write a file through a temporary sibling and atomically replace the * destination. * - * The writer receives a unique temporary path in the destination directory. - * The temporary file is removed if the writer throws. Keeping the temporary - * file beside the destination allows replacement to remain atomic. The - * temporary path preserves the destination's suffixes for format-sensitive - * writers. + * The writer receives the path of an existing empty temporary file in the + * destination directory. It must open and write that file in place, close all + * writes before returning, and must not unlink, rename, replace, or hard-link + * the file. Cleanup is guaranteed only while the reserved file remains at the + * temporary path. Keeping the temporary file beside the destination allows + * replacement to remain atomic. The path preserves the destination's suffixes + * for format-sensitive writers. * * On POSIX, replacing an existing file preserves its ordinary read, write, and * execute permission bits. New files are created with owner-only permissions. + * The filesystem must enforce POSIX permission bits; the write fails rather + * than publishing a file with broader effective permissions. * On Windows, replacement preserves the read-only attribute and new files use * the filesystem's standard access controls. Other file-object metadata and * hard-link identity are not preserved. Atomic replacement prevents partial * visibility but does not guarantee durability after power loss. * Windows alternate data streams are not supported. * - * The destination's parent directory must not be readable or writable by - * principals less privileged than the process performing the write. + * The destination's parent directory and mutable ancestors must not be + * writable by principals less privileged than the process performing the + * write. Missing POSIX parent directories are created with owner-only + * permissions. Windows parent directories use inherited filesystem access + * controls. * * @param path Destination path. * @param writer Function that writes the complete temporary file. diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index d52a75ff9..34db80cf6 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #ifdef _WIN32 #ifndef NOMINMAX @@ -72,13 +73,13 @@ void validate_path(const std::filesystem::path& path) { std::filesystem::path::string_type::npos) { throw std::invalid_argument("Path contains an embedded NUL character"); } - if (path.filename().empty()) { + const auto filename = path.filename(); + if (filename.empty() || filename == "." || filename == "..") { throw std::invalid_argument("Path must name a file"); } #ifdef _WIN32 - const auto root_name_length = path.root_name().native().size(); - if (native_path.find(static_cast(':'), - root_name_length) != + if (path.filename().native().find( + static_cast(':')) != std::filesystem::path::string_type::npos) { throw std::invalid_argument( "Windows alternate data streams are not supported"); @@ -86,6 +87,43 @@ void validate_path(const std::filesystem::path& path) { #endif } +#ifdef _WIN32 +enum class IdentityMatch { match, different, unknown }; + +IdentityMatch compare_handle_to_path_identity( + HANDLE handle, const std::filesystem::path& path, + bool require_single_link) noexcept { + if (handle == INVALID_HANDLE_VALUE) { + return IdentityMatch::unknown; + } + BY_HANDLE_FILE_INFORMATION reserved_info{}; + if (!GetFileInformationByHandle(handle, &reserved_info)) { + return IdentityMatch::unknown; + } + HANDLE current_handle = CreateFileW( + path.c_str(), FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (current_handle == INVALID_HANDLE_VALUE) { + return IdentityMatch::unknown; + } + BY_HANDLE_FILE_INFORMATION current_info{}; + const bool inspected = + GetFileInformationByHandle(current_handle, ¤t_info) != 0; + CloseHandle(current_handle); + if (!inspected) { + return IdentityMatch::unknown; + } + const bool matches = + reserved_info.dwVolumeSerialNumber == current_info.dwVolumeSerialNumber && + reserved_info.nFileIndexHigh == current_info.nFileIndexHigh && + reserved_info.nFileIndexLow == current_info.nFileIndexLow && + (current_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0 && + (!require_single_link || current_info.nNumberOfLinks == 1); + return matches ? IdentityMatch::match : IdentityMatch::different; +} +#endif + std::filesystem::path make_temporary_path( const std::filesystem::path& destination) { static std::atomic counter{0}; @@ -178,7 +216,7 @@ class ReservedTemporaryFile { } ~ReservedTemporaryFile() { - if (cleanup_ && has_same_identity(path_)) { + if (cleanup_ && path_matches_identity(path_, false)) { #ifdef _WIN32 const DWORD attributes = GetFileAttributesW(path_.c_str()); if (attributes != INVALID_FILE_ATTRIBUTES && @@ -196,7 +234,7 @@ class ReservedTemporaryFile { const std::filesystem::path& path() const { return path_; } bool has_same_identity(const std::filesystem::path& path) const noexcept { - return path_matches_identity(path); + return path_matches_identity(path, true); } void verify_identity() const { @@ -255,7 +293,10 @@ class ReservedTemporaryFile { #ifdef _WIN32 static_cast(permissions); #else - if (::fchmod(handle_, static_cast(permissions)) != 0) { + const auto requested = static_cast(permissions) & 0777; + struct stat status{}; + if (::fchmod(handle_, requested) != 0 || ::fstat(handle_, &status) != 0 || + (status.st_mode & 0777) != requested) { throw std::runtime_error("Could not set temporary file permissions: '" + display_path(path_) + "'"); } @@ -281,34 +322,14 @@ class ReservedTemporaryFile { } #endif - bool path_matches_identity(const std::filesystem::path& path) const noexcept { + bool path_matches_identity(const std::filesystem::path& path, + bool require_single_link) const noexcept { if (handle_ == invalid_handle()) { return false; } #ifdef _WIN32 - BY_HANDLE_FILE_INFORMATION reserved_info{}; - if (!GetFileInformationByHandle(handle_, &reserved_info)) { - return false; - } - HANDLE current_handle = CreateFileW( - path.c_str(), FILE_READ_ATTRIBUTES, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, - OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); - if (current_handle == INVALID_HANDLE_VALUE) { - return false; - } - BY_HANDLE_FILE_INFORMATION current_info{}; - const bool inspected = - GetFileInformationByHandle(current_handle, ¤t_info) != 0; - CloseHandle(current_handle); - return inspected && - reserved_info.dwVolumeSerialNumber == - current_info.dwVolumeSerialNumber && - reserved_info.nFileIndexHigh == current_info.nFileIndexHigh && - reserved_info.nFileIndexLow == current_info.nFileIndexLow && - (current_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == - 0 && - current_info.nNumberOfLinks == 1; + return compare_handle_to_path_identity( + handle_, path, require_single_link) == IdentityMatch::match; #else struct stat reserved_status{}; struct stat current_status{}; @@ -316,7 +337,8 @@ class ReservedTemporaryFile { ::lstat(path.c_str(), ¤t_status) == 0 && reserved_status.st_dev == current_status.st_dev && reserved_status.st_ino == current_status.st_ino && - S_ISREG(current_status.st_mode) && current_status.st_nlink == 1; + S_ISREG(current_status.st_mode) && + (!require_single_link || current_status.st_nlink == 1); #endif } @@ -360,6 +382,12 @@ ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, error = std::error_code(permission_error, std::generic_category()); return {path, descriptor}; } + struct stat status{}; + if (::fstat(descriptor, &status) != 0 || + (status.st_mode & 0777) != (S_IRUSR | S_IWUSR)) { + error = std::make_error_code(std::errc::permission_denied); + return {path, descriptor}; + } #endif return {path, #ifdef _WIN32 @@ -462,6 +490,14 @@ void replace_file(const std::filesystem::path& source, const DWORD original_attributes = GetFileAttributesW(destination.c_str()); const bool destination_exists = original_attributes != INVALID_FILE_ATTRIBUTES; + HANDLE original_handle_value = INVALID_HANDLE_VALUE; + if (destination_exists) { + original_handle_value = CreateFileW( + destination.c_str(), FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + } + const ScopedReadHandle original_handle(original_handle_value); if (destination_exists && !SetFileAttributesW(source.c_str(), settable_file_attributes(original_attributes))) { @@ -496,7 +532,19 @@ void replace_file(const std::filesystem::path& source, if (!move()) { const DWORD retry_error = GetLastError(); + const auto identity = compare_handle_to_path_identity(original_handle.get(), + destination, false); + if (identity == IdentityMatch::unknown) { + const std::error_code error(static_cast(retry_error), + std::system_category()); + throw std::runtime_error( + "Could not replace file '" + display_path(destination) + + "': " + error.message() + + "; original attributes were not restored because the destination " + "identity could not be verified"); + } const bool restored = + identity == IdentityMatch::different || SetFileAttributesW(destination.c_str(), settable_file_attributes(original_attributes)) != 0; const DWORD restore_error = restored ? ERROR_SUCCESS : GetLastError(); @@ -559,6 +607,83 @@ void preserve_permissions(ReservedTemporaryFile& temporary_file, std::filesystem::perms::all); } +void create_private_directories(const std::filesystem::path& directory) { +#ifdef _WIN32 + std::error_code error; + std::filesystem::create_directories(directory, error); + if (error) { + throw std::runtime_error("Could not create directory '" + + display_path(directory) + "': " + error.message()); + } +#else + std::vector missing; + auto current = directory; + std::error_code status_error; + while (!current.empty() && + !std::filesystem::is_directory(current, status_error)) { + if (status_error && status_error != std::errc::no_such_file_or_directory) { + throw std::runtime_error("Could not inspect directory '" + + display_path(current) + + "': " + status_error.message()); + } + status_error.clear(); + missing.push_back(current); + const auto parent = current.parent_path(); + if (parent == current) { + break; + } + current = parent; + } + + for (auto iterator = missing.rbegin(); iterator != missing.rend(); + ++iterator) { + if (::mkdir(iterator->c_str(), S_IRWXU) != 0) { + const int mkdir_error = errno; + if (mkdir_error == EEXIST && std::filesystem::is_directory(*iterator)) { + continue; + } + const std::error_code error(mkdir_error, std::generic_category()); + throw std::runtime_error("Could not create directory '" + + display_path(*iterator) + + "': " + error.message()); + } + if (::chmod(iterator->c_str(), S_IRWXU) != 0) { + const std::error_code error(errno, std::generic_category()); + std::error_code ignored; + std::filesystem::remove(*iterator, ignored); + throw std::runtime_error("Could not secure directory '" + + display_path(*iterator) + + "': " + error.message()); + } + const int descriptor = ::open( + iterator->c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + struct stat status{}; + int permission_error = 0; + if (descriptor == -1) { + permission_error = errno; + } else if (::fchmod(descriptor, S_IRWXU) != 0) { + permission_error = errno; + } else if (::fstat(descriptor, &status) != 0) { + permission_error = errno; + } else if ((status.st_mode & 0777) != S_IRWXU) { + permission_error = EPERM; + } + if (permission_error != 0) { + const std::error_code error(permission_error, std::generic_category()); + if (descriptor != -1) { + ::close(descriptor); + } + std::error_code ignored; + std::filesystem::remove(*iterator, ignored); + throw std::runtime_error("Could not secure directory '" + + display_path(*iterator) + + "': " + error.message()); + } + ::close(descriptor); + } +#endif +} + } // namespace void ensure_parent_directory(const std::filesystem::path& path) { @@ -568,12 +693,7 @@ void ensure_parent_directory(const std::filesystem::path& path) { return; } - std::error_code error; - std::filesystem::create_directories(parent, error); - if (error) { - throw std::runtime_error("Could not create parent directory for '" + - display_path(path) + "': " + error.message()); - } + create_private_directories(parent); } std::string read_text_file(const std::filesystem::path& path) { diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index fb49fbc0c..931bcd6b5 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -60,8 +60,33 @@ TEST_F(FileIoTest, CreatesParentDirectoriesWhenRequested) { qdk::chemistry::utils::write_text_file_atomically(path, "contents", true); EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +#ifndef _WIN32 + EXPECT_EQ(std::filesystem::status(path.parent_path()).permissions(), + std::filesystem::perms::owner_all); + EXPECT_EQ( + std::filesystem::status(path.parent_path().parent_path()).permissions(), + std::filesystem::perms::owner_all); +#endif } +#ifndef _WIN32 +TEST_F(FileIoTest, CreatesPrivateParentDirectoriesUnderRestrictiveUmask) { + const auto path = root_ / "private" / "nested" / "data.txt"; + const mode_t original_umask = ::umask(0777); + try { + qdk::chemistry::utils::write_text_file_atomically(path, "contents", true); + } catch (...) { + ::umask(original_umask); + throw; + } + ::umask(original_umask); + + EXPECT_EQ(std::filesystem::status(path.parent_path()).permissions(), + std::filesystem::perms::owner_all); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} +#endif + TEST_F(FileIoTest, RejectsMissingParentDirectoryByDefault) { const auto path = root_ / "missing" / "data.txt"; @@ -88,6 +113,23 @@ TEST_F(FileIoTest, RejectsTrailingSeparatorBeforeWriterRuns) { EXPECT_FALSE(writer_ran); } +TEST_F(FileIoTest, RejectsDotComponentsBeforeWriterRuns) { + bool writer_ran = false; + + EXPECT_THROW( + qdk::chemistry::utils::write_file_atomically( + root_ / ".", + [&writer_ran](const std::filesystem::path&) { writer_ran = true; }), + std::invalid_argument); + EXPECT_THROW( + qdk::chemistry::utils::write_file_atomically( + root_ / "..", + [&writer_ran](const std::filesystem::path&) { writer_ran = true; }), + std::invalid_argument); + + EXPECT_FALSE(writer_ran); +} + TEST_F(FileIoTest, RejectsEmbeddedNulPaths) { const auto prefix = root_ / "data.txt"; std::string path = prefix.string(); @@ -361,6 +403,15 @@ TEST_F(FileIoTest, FallsBackForNearMaxPathDestinationOnWindows) { EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); } + +TEST_F(FileIoTest, SupportsExtendedLengthPathsOnWindows) { + const std::filesystem::path path = + L"\\\\?\\" + root_.native() + L"\\extended.txt"; + + qdk::chemistry::utils::write_text_file_atomically(path, "contents"); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "contents"); +} #endif TEST_F(FileIoTest, PreservesDestinationSuffixesForWriter) { @@ -470,6 +521,31 @@ TEST_F(FileIoTest, RejectsReplacedTemporaryFile) { EXPECT_TRUE(std::filesystem::exists(replacement_path)); } +#ifndef _WIN32 +TEST_F(FileIoTest, CleansReservedPathAfterWriterAddsHardLink) { + const auto path = root_ / "data.txt"; + const auto extra_link = root_ / "extra.txt"; + std::filesystem::path temporary_path; + + EXPECT_THROW(qdk::chemistry::utils::write_file_atomically( + path, + [&](const std::filesystem::path& reserved_path) { + temporary_path = reserved_path; + std::ofstream output(reserved_path); + output << "sensitive"; + output.close(); + std::filesystem::create_hard_link(reserved_path, + extra_link); + throw std::runtime_error("writer failed"); + }), + std::runtime_error); + + EXPECT_FALSE(std::filesystem::exists(temporary_path)); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(extra_link), "sensitive"); + EXPECT_FALSE(std::filesystem::exists(path)); +} +#endif + TEST_F(FileIoTest, FreezesRelativeDestinationBeforeWriterRuns) { const auto original_directory = std::filesystem::current_path(); const auto first_directory = root_ / "first"; diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 0286e2327..f2dcb60b3 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -37,13 +37,20 @@ def ensure_parent_directory(path: PathLike) -> None: _validate_destination_path(path) parent = Path(path).parent if parent != Path("."): - parent.mkdir(parents=True, exist_ok=True) + if os.name == "nt": + parent.mkdir(parents=True, exist_ok=True) + else: + _create_private_directories(parent) def _validate_destination_path(path: PathLike) -> None: value = os.fspath(path) separators = tuple(separator for separator in (os.sep, os.altsep) if separator) - if value.endswith(separators): + path_module = ntpath if sys.platform == "win32" else os.path + final_component = path_module.basename(value) + if "\0" in value: + raise ValueError(f"Path contains an embedded NUL character: '{value}'") + if not value or value.endswith(separators) or final_component in ("", ".", ".."): raise ValueError(f"Destination path must name a file: '{value}'") if sys.platform == "win32" and ":" in ntpath.splitdrive(value)[1]: raise ValueError(f"Windows alternate data streams are not supported: '{value}'") @@ -77,20 +84,27 @@ def write_file_atomically( ) -> None: """Write through a temporary sibling and atomically replace *path*. - The writer receives a unique temporary path in the destination directory. - The path preserves the destination's suffixes for format-sensitive writers. - The temporary file is removed if the writer raises an exception. + The writer receives an existing empty temporary file in the destination + directory. It must write that file in place, close all writes before + returning, and must not unlink, rename, replace, or hard-link the file. + Cleanup is guaranteed only while the reserved file remains at the temporary + path. The path preserves the destination's suffixes for format-sensitive + writers. On POSIX, replacing an existing file preserves its ordinary read, write, and execute permission bits. New files are created with owner-only - permissions. On Windows, replacement preserves the read-only attribute and - new files use the filesystem's standard access controls. Other file-object - metadata and hard-link identity are not preserved. Atomic replacement - prevents partial visibility but does not guarantee durability after power - loss. - - The destination's parent directory must not be readable or writable by - principals less privileged than the process performing the write. + permissions. The filesystem must enforce POSIX permission bits; the write + fails rather than publishing a file with broader effective permissions. On + Windows, replacement preserves the read-only attribute and new files use + the filesystem's standard access controls. Other file-object metadata and + hard-link identity are not preserved. Atomic replacement prevents partial + visibility but does not guarantee durability after power loss. + + The destination's parent directory and mutable ancestors must not be + writable by principals less privileged than the process performing the + write. Missing POSIX parent directories are created with owner-only + permissions. Windows parent directories use inherited filesystem access + controls. """ _validate_destination_path(path) destination = Path(path) @@ -115,25 +129,27 @@ def write_file_atomically( raise RuntimeError(f"Temporary file identity changed: '{temporary_path}'") try: - destination_status = destination.lstat() + existing_status = destination.lstat() except FileNotFoundError: destination_mode = None + destination_status = None if os.name != "nt": - os.fchmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) + _set_permissions(descriptor, stat.S_IRUSR | stat.S_IWUSR, temporary_path) else: - if stat.S_ISLNK(destination_status.st_mode): + destination_status = existing_status + if stat.S_ISLNK(existing_status.st_mode): raise ValueError(f"Symlink destinations are not supported: '{destination}'") - existing_mode = stat.S_IMODE(destination_status.st_mode) & 0o777 + existing_mode = stat.S_IMODE(existing_status.st_mode) & 0o777 destination_mode = existing_mode if os.name != "nt" and hasattr(os, "fchmod"): - os.fchmod(descriptor, existing_mode) + _set_permissions(descriptor, existing_mode, temporary_path) elif os.name != "nt": temporary_path.chmod(existing_mode) if os.name == "nt": os.close(descriptor) descriptor = -1 - _replace_file(temporary_path, destination, destination_mode) + _replace_file(temporary_path, destination, destination_mode, destination_status) except BaseException as error: if reserved_status is None and descriptor >= 0: try: @@ -205,7 +221,7 @@ def _create_exclusive_file(path: Path) -> int: if sys.platform != "win32": descriptor = os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY | os.O_CLOEXEC, 0o600) try: - os.fchmod(descriptor, stat.S_IRUSR | stat.S_IWUSR) + _set_permissions(descriptor, stat.S_IRUSR | stat.S_IWUSR, path) except BaseException as error: try: reserved_status = os.fstat(descriptor) @@ -230,6 +246,8 @@ def _open_windows_file( desired_access: int, creation_disposition: int, ) -> int: + if sys.platform != "win32": + raise NotImplementedError("Windows file handles require Windows") create_file = ctypes.WinDLL("kernel32", use_last_error=True).CreateFileW create_file.argtypes = ( @@ -272,21 +290,38 @@ def _open_windows_file( def _windows_error(path: PathLike, error: int) -> OSError: + if sys.platform != "win32": + raise NotImplementedError("Windows errors require Windows") return OSError(0, ctypes.FormatError(error), os.fspath(path), error) def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> int | None: descriptor = _create_exclusive_file(temporary_path) + reserved_status: os.stat_result | None = None try: reserved_status = os.fstat(descriptor) - except BaseException: + destination_matches = _path_matches_identity( + destination, + reserved_status, + require_single_link=True, + ) + except BaseException as error: os.close(descriptor) + if reserved_status is not None and _path_matches_identity( + temporary_path, + reserved_status, + require_single_link=False, + ): + try: + _remove_temporary_file(temporary_path) + except OSError as cleanup_error: + raise error from cleanup_error raise - if not _temporary_path_matches(destination, reserved_status): + if not destination_matches: return descriptor os.close(descriptor) - if _temporary_path_matches(temporary_path, reserved_status): + if _path_matches_identity(temporary_path, reserved_status, require_single_link=False): _remove_temporary_file(temporary_path) return None @@ -311,12 +346,42 @@ def _same_file_identity(reserved_status: os.stat_result, current_status: os.stat ) -def _temporary_path_matches(temporary_path: Path, reserved_status: os.stat_result) -> bool: +def _path_matches_identity( + path: Path, + reserved_status: os.stat_result, + *, + require_single_link: bool, +) -> bool: + return ( + _path_identity_state( + path, + reserved_status, + require_single_link=require_single_link, + ) + is True + ) + + +def _path_identity_state( + path: Path, + reserved_status: os.stat_result, + *, + require_single_link: bool, +) -> bool | None: try: - current_status = temporary_path.lstat() - except OSError: - return False - return _same_file_identity(reserved_status, current_status) + current_status = path.lstat() + except (OSError, ValueError): + return None + return ( + reserved_status.st_dev == current_status.st_dev + and reserved_status.st_ino == current_status.st_ino + and stat.S_ISREG(current_status.st_mode) + and (not require_single_link or current_status.st_nlink == 1) + ) + + +def _temporary_path_matches(temporary_path: Path, reserved_status: os.stat_result) -> bool: + return _path_matches_identity(temporary_path, reserved_status, require_single_link=False) def _remove_temporary_file(temporary_path: Path) -> None: @@ -334,7 +399,12 @@ def _remove_temporary_file(temporary_path: Path) -> None: temporary_path.unlink(missing_ok=True) -def _replace_file(temporary_path: Path, destination: Path, destination_mode: int | None) -> None: +def _replace_file( + temporary_path: Path, + destination: Path, + destination_mode: int | None, + destination_status: os.stat_result | None, +) -> None: """Replace a destination, handling Windows read-only files.""" if os.name == "nt" and destination_mode is not None: temporary_path.chmod(destination_mode) @@ -351,6 +421,19 @@ def _replace_file(temporary_path: Path, destination: Path, destination_mode: int try: os.replace(temporary_path, destination) except BaseException as replace_error: + if destination_status is None: + raise + identity_matches = _path_identity_state( + destination, + destination_status, + require_single_link=False, + ) + if identity_matches is False: + raise + if identity_matches is None: + raise RuntimeError( + f"Could not safely restore attributes for '{destination}' because its identity could not be verified" + ) from replace_error try: destination.chmod(destination_mode) except OSError as rollback_error: @@ -358,6 +441,53 @@ def _replace_file(temporary_path: Path, destination: Path, destination_mode: int raise +def _set_permissions(descriptor: int, mode: int, path: Path) -> None: + os.fchmod(descriptor, mode) + actual_mode = stat.S_IMODE(os.fstat(descriptor).st_mode) + if actual_mode != mode: + raise PermissionError( + errno.EPERM, + f"Filesystem did not apply permissions {mode:#o} to '{path}'", + os.fspath(path), + ) + + +def _create_private_directories(directory: Path) -> None: + missing: list[Path] = [] + current = directory + while not current.is_dir(): + missing.append(current) + parent = current.parent + if parent == current: + break + current = parent + + for missing_directory in reversed(missing): + try: + os.mkdir(missing_directory, 0o700) + except FileExistsError: + if not missing_directory.is_dir(): + raise + continue + descriptor = -1 + try: + os.chmod(missing_directory, 0o700) + descriptor = os.open( + missing_directory, + os.O_RDONLY + | getattr(os, "O_DIRECTORY", 0) + | getattr(os, "O_NOFOLLOW", 0) + | getattr(os, "O_CLOEXEC", 0), + ) + _set_permissions(descriptor, 0o700, missing_directory) + except BaseException: + if descriptor >= 0: + os.close(descriptor) + missing_directory.rmdir() + raise + os.close(descriptor) + + def write_text_file_atomically( path: PathLike, contents: str, diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index de8233a67..cb2b11610 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -8,6 +8,7 @@ import ctypes import os import stat +import sys import threading from ctypes import wintypes from pathlib import Path @@ -39,6 +40,22 @@ def test_create_parent_directories_when_requested(tmp_path: Path): write_text_file_atomically(path, "contents", create_parent_directories=True) assert read_text_file(path) == "contents" + if os.name != "nt": + assert stat.S_IMODE(path.parent.stat().st_mode) == 0o700 + assert stat.S_IMODE(path.parent.parent.stat().st_mode) == 0o700 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX umask semantics") +def test_create_private_parent_directories_under_restrictive_umask(tmp_path: Path): + path = tmp_path / "private" / "nested" / "data.txt" + original_umask = os.umask(0o777) + try: + write_text_file_atomically(path, "contents", create_parent_directories=True) + finally: + os.umask(original_umask) + + assert stat.S_IMODE(path.parent.stat().st_mode) == 0o700 + assert read_text_file(path) == "contents" def test_reject_missing_parent_directory_by_default(tmp_path: Path): @@ -69,6 +86,35 @@ def test_reject_trailing_separator_in_all_path_helpers(tmp_path: Path): read_text_file(trailing_path) +@pytest.mark.parametrize( + "path", + [ + "", + ".", + "..", + f"data{os.sep}.", + "data\0ignored.txt", + ], +) +def test_reject_invalid_destination_before_writer_runs(tmp_path: Path, path: str): + writer_ran = False + destination = path if not path.startswith("data") else f"{tmp_path}{os.sep}{path}" + + def writer(temporary_path: Path) -> None: + nonlocal writer_ran + writer_ran = True + temporary_path.write_text("contents", encoding="utf-8") + + with pytest.raises(ValueError, match=r"must name a file|embedded NUL"): + write_file_atomically(destination, writer) + with pytest.raises(ValueError, match=r"must name a file|embedded NUL"): + ensure_parent_directory(destination) + with pytest.raises(ValueError, match=r"must name a file|embedded NUL"): + read_text_file(destination) + + assert not writer_ran + + def test_preserve_destination_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" write_text_file_atomically(path, "original") @@ -190,6 +236,25 @@ def write_and_relax_permissions(temporary_path: Path) -> None: assert stat.S_IMODE(path.stat().st_mode) == 0o600 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_reject_filesystem_that_ignores_permissions( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + original_fchmod = os.fchmod + + def apply_broader_permissions(descriptor: int, _mode: int) -> None: + original_fchmod(descriptor, 0o644) + + monkeypatch.setattr(file_io_module.os, "fchmod", apply_broader_permissions) + + with pytest.raises(PermissionError, match="did not apply permissions"): + write_text_file_atomically(path, "contents") + + assert list(tmp_path.iterdir()) == [] + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_replace_read_only_destination_on_windows(tmp_path: Path): path = tmp_path / "data.txt" @@ -219,6 +284,8 @@ def write_read_only_temporary_file(temporary_path: Path) -> None: @pytest.mark.skipif(os.name != "nt", reason="Windows file-sharing behavior") def test_allow_exclusive_writer_on_windows(tmp_path: Path): + if sys.platform != "win32": + raise AssertionError("Windows-only test ran on another platform") path = tmp_path / "data.txt" def write_with_exclusive_handle(temporary_path: Path) -> None: @@ -267,6 +334,8 @@ def test_reader_does_not_block_atomic_replacement_on_windows( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ): + if sys.platform != "win32": + raise AssertionError("Windows-only test ran on another platform") path = tmp_path / "data.txt" write_text_file_atomically(path, "original") reader_opened = threading.Event() @@ -322,6 +391,8 @@ def test_fall_back_for_near_max_path_destination_on_windows(tmp_path: Path): @pytest.mark.skipif(os.name != "nt", reason="Windows error semantics") def test_windows_errors_preserve_winerror_and_subclass(tmp_path: Path): + if sys.platform != "win32": + raise AssertionError("Windows-only test ran on another platform") permission_error = file_io_module._windows_error(tmp_path / "data.txt", 5) length_error = file_io_module._windows_error(tmp_path / "data.txt", 206) @@ -430,6 +501,28 @@ def replace_temporary_file(temporary_path: Path) -> None: assert replacement_path.read_text(encoding="utf-8") == "replacement" +@pytest.mark.skipif(os.name == "nt", reason="POSIX hard-link semantics") +def test_clean_up_reserved_path_after_writer_adds_hard_link(tmp_path: Path): + path = tmp_path / "data.txt" + extra_link = tmp_path / "extra.txt" + temporary_path: Path | None = None + + def fail_after_linking(reserved_path: Path) -> None: + nonlocal temporary_path + temporary_path = reserved_path + reserved_path.write_text("sensitive", encoding="utf-8") + os.link(reserved_path, extra_link) + raise RuntimeError("writer failed") + + with pytest.raises(RuntimeError, match="writer failed"): + write_file_atomically(path, fail_after_linking) + + assert temporary_path is not None + assert not temporary_path.exists() + assert extra_link.read_text(encoding="utf-8") == "sensitive" + assert not path.exists() + + def test_freeze_relative_destination_before_writer_runs(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): first_directory = tmp_path / "first" second_directory = tmp_path / "second" From 7501af3d5516530f1f65bbe95dedd003664669c8 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Fri, 21 Aug 2026 22:46:45 -0700 Subject: [PATCH 05/23] Resolve final FileIO council findings --- cpp/include/qdk/chemistry/utils/file_io.hpp | 19 ++- cpp/src/qdk/chemistry/utils/file_io.cpp | 153 ++++++++++++------- cpp/tests/test_file_io.cpp | 45 ++++++ python/src/qdk_chemistry/utils/file_io.py | 158 +++++++++++++++----- python/tests/test_utils_file_io.py | 34 ++++- 5 files changed, 316 insertions(+), 93 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index fafba7551..52c4e63ac 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -42,11 +42,18 @@ std::string read_text_file(const std::filesystem::path& path); * On POSIX, replacing an existing file preserves its ordinary read, write, and * execute permission bits. New files are created with owner-only permissions. * The filesystem must enforce POSIX permission bits; the write fails rather - * than publishing a file with broader effective permissions. + * than publishing a file with broader mode bits. Platform ACLs are not + * inspected and may grant access beyond those bits. * On Windows, replacement preserves the read-only attribute and new files use - * the filesystem's standard access controls. Other file-object metadata and - * hard-link identity are not preserved. Atomic replacement prevents partial - * visibility but does not guarantee durability after power loss. + * the filesystem's standard access controls. Existing Windows security + * descriptors and DACLs are not preserved; the replacement uses access + * controls inherited when its temporary file is created and may therefore + * grant broader access than the file it replaced. Callers that rely on + * explicit access-control entries must reapply them after the write. Read-only + * Windows destinations with multiple hard links are rejected. Other + * file-object metadata and hard-link identity are not preserved. Atomic + * replacement prevents partial visibility but does not guarantee durability + * after power loss. * Windows alternate data streams are not supported. * * The destination's parent directory and mutable ancestors must not be @@ -55,6 +62,10 @@ std::string read_text_file(const std::filesystem::path& path); * permissions. Windows parent directories use inherited filesystem access * controls. * + * On POSIX, relative destinations are frozen to an absolute path before the + * writer runs. A relative path may therefore be rejected when its expanded + * absolute form exceeds the platform pathname limit. + * * @param path Destination path. * @param writer Function that writes the complete temporary file. * @param create_parent_directories Create missing parent directories when true. diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 34db80cf6..ecbd37e0e 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -25,6 +26,17 @@ namespace qdk::chemistry::utils { namespace { +#ifndef _WIN32 +template +auto retry_on_eintr(Operation&& operation) -> decltype(operation()) { + decltype(operation()) result; + do { + result = operation(); + } while (result == -1 && errno == EINTR); + return result; +} +#endif + class ScopedReadHandle { public: #ifdef _WIN32 @@ -172,7 +184,7 @@ std::filesystem::path make_compact_temporary_path( } #ifdef _WIN32 -DWORD settable_file_attributes(DWORD attributes) { +DWORD normalized_file_attributes(DWORD attributes) { constexpr DWORD supported_attributes = FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_NOT_CONTENT_INDEXED | FILE_ATTRIBUTE_OFFLINE | @@ -181,6 +193,25 @@ DWORD settable_file_attributes(DWORD attributes) { const DWORD result = attributes & supported_attributes; return result == 0 ? FILE_ATTRIBUTE_NORMAL : result; } + +DWORD replacement_file_attributes(DWORD attributes) { + const DWORD result = attributes & FILE_ATTRIBUTE_READONLY; + return result == 0 ? FILE_ATTRIBUTE_NORMAL : result; +} + +bool set_handle_file_attributes(HANDLE handle, DWORD attributes) noexcept { + if (handle == INVALID_HANDLE_VALUE) { + return false; + } + FILE_BASIC_INFO info{}; + if (!GetFileInformationByHandleEx(handle, FileBasicInfo, &info, + sizeof(info))) { + return false; + } + info.FileAttributes = normalized_file_attributes(attributes); + return SetFileInformationByHandle(handle, FileBasicInfo, &info, + sizeof(info)) != 0; +} #endif class ReservedTemporaryFile { @@ -223,7 +254,7 @@ class ReservedTemporaryFile { (attributes & FILE_ATTRIBUTE_READONLY) != 0) { SetFileAttributesW( path_.c_str(), - settable_file_attributes(attributes & ~FILE_ATTRIBUTE_READONLY)); + normalized_file_attributes(attributes & ~FILE_ATTRIBUTE_READONLY)); } #endif std::error_code ignored; @@ -295,7 +326,8 @@ class ReservedTemporaryFile { #else const auto requested = static_cast(permissions) & 0777; struct stat status{}; - if (::fchmod(handle_, requested) != 0 || ::fstat(handle_, &status) != 0 || + if (retry_on_eintr([&] { return ::fchmod(handle_, requested); }) != 0 || + retry_on_eintr([&] { return ::fstat(handle_, &status); }) != 0 || (status.st_mode & 0777) != requested) { throw std::runtime_error("Could not set temporary file permissions: '" + display_path(path_) + "'"); @@ -371,19 +403,21 @@ ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, return {path, ReservedTemporaryFile::invalid_handle()}; } #else - const int descriptor = - ::open(path.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600); + const int descriptor = retry_on_eintr([&] { + return ::open(path.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600); + }); if (descriptor == -1) { error = std::error_code(errno, std::generic_category()); return {path, ReservedTemporaryFile::invalid_handle()}; } - if (::fchmod(descriptor, S_IRUSR | S_IWUSR) != 0) { + if (retry_on_eintr([&] { return ::fchmod(descriptor, S_IRUSR | S_IWUSR); }) != + 0) { const int permission_error = errno; error = std::error_code(permission_error, std::generic_category()); return {path, descriptor}; } struct stat status{}; - if (::fstat(descriptor, &status) != 0 || + if (retry_on_eintr([&] { return ::fstat(descriptor, &status); }) != 0 || (status.st_mode & 0777) != (S_IRUSR | S_IWUSR)) { error = std::make_error_code(std::errc::permission_denied); return {path, descriptor}; @@ -493,14 +527,14 @@ void replace_file(const std::filesystem::path& source, HANDLE original_handle_value = INVALID_HANDLE_VALUE; if (destination_exists) { original_handle_value = CreateFileW( - destination.c_str(), FILE_READ_ATTRIBUTES, + destination.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); } const ScopedReadHandle original_handle(original_handle_value); if (destination_exists && !SetFileAttributesW(source.c_str(), - settable_file_attributes(original_attributes))) { + replacement_file_attributes(original_attributes))) { const std::error_code error(static_cast(GetLastError()), std::system_category()); throw std::runtime_error("Could not prepare file attributes for '" + @@ -519,50 +553,57 @@ void replace_file(const std::filesystem::path& source, const DWORD first_error = GetLastError(); const bool read_only = destination_exists && (original_attributes & FILE_ATTRIBUTE_READONLY) != 0; - if (first_error != ERROR_ACCESS_DENIED || !read_only || - !SetFileAttributesW(destination.c_str(), - settable_file_attributes(original_attributes & - ~FILE_ATTRIBUTE_READONLY))) { + if (first_error != ERROR_ACCESS_DENIED || !read_only) { const std::error_code error(static_cast(first_error), std::system_category()); throw std::runtime_error("Could not replace file '" + display_path(destination) + "': " + error.message()); } - - if (!move()) { - const DWORD retry_error = GetLastError(); - const auto identity = compare_handle_to_path_identity(original_handle.get(), - destination, false); - if (identity == IdentityMatch::unknown) { - const std::error_code error(static_cast(retry_error), - std::system_category()); - throw std::runtime_error( - "Could not replace file '" + display_path(destination) + - "': " + error.message() + - "; original attributes were not restored because the destination " - "identity could not be verified"); - } - const bool restored = - identity == IdentityMatch::different || - SetFileAttributesW(destination.c_str(), - settable_file_attributes(original_attributes)) != 0; - const DWORD restore_error = restored ? ERROR_SUCCESS : GetLastError(); - const std::error_code error(static_cast(retry_error), + BY_HANDLE_FILE_INFORMATION original_info{}; + if (original_handle.get() == INVALID_HANDLE_VALUE || + !GetFileInformationByHandle(original_handle.get(), &original_info)) { + const std::error_code error(static_cast(GetLastError()), std::system_category()); - if (!restored) { - const std::error_code rollback_error(static_cast(restore_error), - std::system_category()); - throw std::runtime_error("Could not replace file '" + - display_path(destination) + - "': " + error.message() + - "; could not restore original attributes: " + - rollback_error.message()); - } - throw std::runtime_error("Could not replace file '" + + throw std::runtime_error("Could not inspect read-only destination '" + display_path(destination) + "': " + error.message()); } + if (original_info.nNumberOfLinks != 1) { + throw std::runtime_error( + "Read-only Windows destinations with multiple hard links are not " + "supported: '" + + display_path(destination) + "'"); + } + if (!set_handle_file_attributes( + original_handle.get(), + original_attributes & ~FILE_ATTRIBUTE_READONLY)) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + throw std::runtime_error("Could not prepare read-only destination '" + + display_path(destination) + + "': " + error.message()); + } + + const bool replaced = move(); + const DWORD retry_error = replaced ? ERROR_SUCCESS : GetLastError(); + if (replaced) { + return; + } + if (!set_handle_file_attributes(original_handle.get(), original_attributes)) { + const std::error_code rollback_error(static_cast(GetLastError()), + std::system_category()); + const std::error_code error(static_cast(retry_error), + std::system_category()); + throw std::runtime_error( + "Could not replace file '" + display_path(destination) + + "': " + error.message() + + "; could not restore original attributes: " + rollback_error.message()); + } + const std::error_code error(static_cast(retry_error), + std::system_category()); + throw std::runtime_error("Could not replace file '" + + display_path(destination) + "': " + error.message()); #else std::error_code error; std::filesystem::rename(source, destination, error); @@ -602,6 +643,10 @@ void preserve_permissions(ReservedTemporaryFile& temporary_file, throw std::runtime_error("Symlink destinations are not supported: '" + display_path(destination) + "'"); } + if (!std::filesystem::is_regular_file(status)) { + throw std::runtime_error("Destination is not a regular file: '" + + display_path(destination) + "'"); + } temporary_file.set_permissions(status.permissions() & std::filesystem::perms::all); @@ -637,7 +682,8 @@ void create_private_directories(const std::filesystem::path& directory) { for (auto iterator = missing.rbegin(); iterator != missing.rend(); ++iterator) { - if (::mkdir(iterator->c_str(), S_IRWXU) != 0) { + if (retry_on_eintr([&] { return ::mkdir(iterator->c_str(), S_IRWXU); }) != + 0) { const int mkdir_error = errno; if (mkdir_error == EEXIST && std::filesystem::is_directory(*iterator)) { continue; @@ -647,7 +693,8 @@ void create_private_directories(const std::filesystem::path& directory) { display_path(*iterator) + "': " + error.message()); } - if (::chmod(iterator->c_str(), S_IRWXU) != 0) { + if (retry_on_eintr([&] { return ::chmod(iterator->c_str(), S_IRWXU); }) != + 0) { const std::error_code error(errno, std::generic_category()); std::error_code ignored; std::filesystem::remove(*iterator, ignored); @@ -655,15 +702,19 @@ void create_private_directories(const std::filesystem::path& directory) { display_path(*iterator) + "': " + error.message()); } - const int descriptor = ::open( - iterator->c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + const int descriptor = retry_on_eintr([&] { + return ::open(iterator->c_str(), + O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + }); struct stat status{}; int permission_error = 0; if (descriptor == -1) { permission_error = errno; - } else if (::fchmod(descriptor, S_IRWXU) != 0) { + } else if (retry_on_eintr([&] { return ::fchmod(descriptor, S_IRWXU); }) != + 0) { permission_error = errno; - } else if (::fstat(descriptor, &status) != 0) { + } else if (retry_on_eintr([&] { return ::fstat(descriptor, &status); }) != + 0) { permission_error = errno; } else if ((status.st_mode & 0777) != S_IRWXU) { permission_error = EPERM; @@ -734,8 +785,8 @@ std::string read_text_file(const std::filesystem::path& path) { contents.append(buffer.data(), bytes_read); } #else - const int descriptor = - ::open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC); + const int descriptor = retry_on_eintr( + [&] { return ::open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC); }); if (descriptor == -1) { throw std::runtime_error("Could not open file for reading: '" + display_path(path) + "'"); diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index 931bcd6b5..26fddd735 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -244,6 +244,19 @@ TEST_F(FileIoTest, RejectsSymlinkDestinationsWithoutCopyingReferentMode) { EXPECT_TRUE(std::filesystem::is_symlink(link)); EXPECT_EQ(qdk::chemistry::utils::read_text_file(target), "target"); } + +TEST_F(FileIoTest, RejectsNonRegularDestinations) { + const auto path = root_ / "data.fifo"; + ASSERT_EQ(::mkfifo(path.c_str(), 0666), 0); + + EXPECT_THROW( + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"), + std::runtime_error); + + struct stat status{}; + ASSERT_EQ(::lstat(path.c_str(), &status), 0); + EXPECT_TRUE(S_ISFIFO(status.st_mode)); +} #endif TEST_F(FileIoTest, CreatesNewFilesWithOwnerOnlyPermissions) { @@ -310,6 +323,38 @@ TEST_F(FileIoTest, ReplacesReadOnlyDestinationOnWindows) { std::filesystem::perms::none); } +TEST_F(FileIoTest, RejectsReadOnlyDestinationWithSurvivingHardLink) { + const auto path = root_ / "data.txt"; + const auto alias = root_ / "alias.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + std::filesystem::create_hard_link(path, alias); + ASSERT_NE(SetFileAttributesW(path.c_str(), FILE_ATTRIBUTE_READONLY), 0); + + EXPECT_THROW( + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"), + std::runtime_error); + + EXPECT_EQ(qdk::chemistry::utils::read_text_file(path), "original"); + EXPECT_EQ(qdk::chemistry::utils::read_text_file(alias), "original"); + EXPECT_NE(GetFileAttributesW(path.c_str()) & FILE_ATTRIBUTE_READONLY, 0); + EXPECT_NE(GetFileAttributesW(alias.c_str()) & FILE_ATTRIBUTE_READONLY, 0); +} + +TEST_F(FileIoTest, DoesNotCopyTemporaryStorageAttributeToReplacement) { + const auto path = root_ / "data.txt"; + qdk::chemistry::utils::write_text_file_atomically(path, "original"); + ASSERT_NE(SetFileAttributesW(path.c_str(), FILE_ATTRIBUTE_READONLY | + FILE_ATTRIBUTE_TEMPORARY), + 0); + + qdk::chemistry::utils::write_text_file_atomically(path, "replacement"); + + const DWORD attributes = GetFileAttributesW(path.c_str()); + ASSERT_NE(attributes, INVALID_FILE_ATTRIBUTES); + EXPECT_NE(attributes & FILE_ATTRIBUTE_READONLY, 0); + EXPECT_EQ(attributes & FILE_ATTRIBUTE_TEMPORARY, 0); +} + TEST_F(FileIoTest, CleansUpReadOnlyTemporaryFileWhenWriterFails) { const auto path = root_ / "data.txt"; qdk::chemistry::utils::write_text_file_atomically(path, "original"); diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index f2dcb60b3..622e0fedc 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -94,17 +94,29 @@ def write_file_atomically( On POSIX, replacing an existing file preserves its ordinary read, write, and execute permission bits. New files are created with owner-only permissions. The filesystem must enforce POSIX permission bits; the write - fails rather than publishing a file with broader effective permissions. On - Windows, replacement preserves the read-only attribute and new files use - the filesystem's standard access controls. Other file-object metadata and - hard-link identity are not preserved. Atomic replacement prevents partial - visibility but does not guarantee durability after power loss. + fails rather than publishing a file with broader mode bits. Platform ACLs + are not inspected and may grant access beyond those bits. On Windows, + replacement preserves the read-only attribute and new files use the + filesystem's standard access controls. Existing Windows security + descriptors and DACLs are not preserved; the replacement uses access + controls inherited when its temporary file is created and may therefore + grant broader access than the file it replaced. Callers that rely on + explicit access-control entries must reapply them after the write. + Read-only Windows destinations with multiple hard links are rejected. + Other file-object metadata and hard-link identity are not preserved. + Atomic replacement prevents partial visibility but does not guarantee + durability after power loss. Windows alternate data streams are not + supported. The destination's parent directory and mutable ancestors must not be writable by principals less privileged than the process performing the write. Missing POSIX parent directories are created with owner-only permissions. Windows parent directories use inherited filesystem access controls. + + On POSIX, relative destinations are frozen to an absolute path before the + writer runs. A relative path may therefore be rejected when its expanded + absolute form exceeds the platform pathname limit. """ _validate_destination_path(path) destination = Path(path) @@ -132,13 +144,13 @@ def write_file_atomically( existing_status = destination.lstat() except FileNotFoundError: destination_mode = None - destination_status = None if os.name != "nt": _set_permissions(descriptor, stat.S_IRUSR | stat.S_IWUSR, temporary_path) else: - destination_status = existing_status if stat.S_ISLNK(existing_status.st_mode): raise ValueError(f"Symlink destinations are not supported: '{destination}'") + if not stat.S_ISREG(existing_status.st_mode): + raise OSError(f"Destination is not a regular file: '{destination}'") existing_mode = stat.S_IMODE(existing_status.st_mode) & 0o777 destination_mode = existing_mode if os.name != "nt" and hasattr(os, "fchmod"): @@ -149,7 +161,7 @@ def write_file_atomically( if os.name == "nt": os.close(descriptor) descriptor = -1 - _replace_file(temporary_path, destination, destination_mode, destination_status) + _replace_file(temporary_path, destination, destination_mode) except BaseException as error: if reserved_status is None and descriptor >= 0: try: @@ -245,6 +257,7 @@ def _open_windows_file( *, desired_access: int, creation_disposition: int, + flags_and_attributes: int = 0x00000080, ) -> int: if sys.platform != "win32": raise NotImplementedError("Windows file handles require Windows") @@ -266,7 +279,7 @@ def _open_windows_file( 0x00000001 | 0x00000002 | 0x00000004, None, creation_disposition, - 0x00000080, + flags_and_attributes, None, ) if handle == wintypes.HANDLE(-1).value: @@ -295,6 +308,56 @@ def _windows_error(path: PathLike, error: int) -> OSError: return OSError(0, ctypes.FormatError(error), os.fspath(path), error) +class _WindowsFileBasicInfo(ctypes.Structure): + _fields_ = ( + ("creation_time", ctypes.c_longlong), + ("last_access_time", ctypes.c_longlong), + ("last_write_time", ctypes.c_longlong), + ("change_time", ctypes.c_longlong), + ("file_attributes", wintypes.DWORD), + ) + + +def _windows_file_info(descriptor: int, path: Path) -> _WindowsFileBasicInfo: + if sys.platform != "win32": + raise NotImplementedError("Windows file attributes require Windows") + get_osfhandle = cast( + "Callable[[int], int]", + importlib.import_module("msvcrt").get_osfhandle, + ) + handle = get_osfhandle(descriptor) + info = _WindowsFileBasicInfo() + get_info = ctypes.WinDLL("kernel32", use_last_error=True).GetFileInformationByHandleEx + get_info.argtypes = (wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD) + get_info.restype = wintypes.BOOL + if not get_info(handle, 0, ctypes.byref(info), ctypes.sizeof(info)): + raise _windows_error(path, ctypes.get_last_error()) + return info + + +def _normalized_windows_file_attributes(attributes: int) -> int: + supported = 0x00000001 | 0x00000002 | 0x00000004 | 0x00000020 | 0x00000100 | 0x00001000 | 0x00002000 + result = attributes & supported + return result or 0x00000080 + + +def _set_windows_file_attributes(descriptor: int, attributes: int, path: Path) -> None: + if sys.platform != "win32": + raise NotImplementedError("Windows file attributes require Windows") + get_osfhandle = cast( + "Callable[[int], int]", + importlib.import_module("msvcrt").get_osfhandle, + ) + handle = get_osfhandle(descriptor) + info = _windows_file_info(descriptor, path) + info.file_attributes = _normalized_windows_file_attributes(attributes) + set_info = ctypes.WinDLL("kernel32", use_last_error=True).SetFileInformationByHandle + set_info.argtypes = (wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD) + set_info.restype = wintypes.BOOL + if not set_info(handle, 0, ctypes.byref(info), ctypes.sizeof(info)): + raise _windows_error(path, ctypes.get_last_error()) + + def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> int | None: descriptor = _create_exclusive_file(temporary_path) reserved_status: os.stat_result | None = None @@ -403,42 +466,63 @@ def _replace_file( temporary_path: Path, destination: Path, destination_mode: int | None, - destination_status: os.stat_result | None, ) -> None: """Replace a destination, handling Windows read-only files.""" + original_descriptor = -1 + original_attributes: int | None = None if os.name == "nt" and destination_mode is not None: - temporary_path.chmod(destination_mode) + try: + original_descriptor = _open_windows_file( + destination, + desired_access=0x00000080 | 0x00000100, + creation_disposition=3, + flags_and_attributes=0x00000080 | 0x00200000, + ) + except OSError: + original_descriptor = -1 + else: + try: + original_attributes = int(_windows_file_info(original_descriptor, destination).file_attributes) + except BaseException: + os.close(original_descriptor) + original_descriptor = -1 + raise try: - os.replace(temporary_path, destination) - return - except PermissionError: - read_only = os.name == "nt" and destination_mode is not None and destination_mode & stat.S_IWRITE == 0 - if not read_only: - raise + if os.name == "nt" and destination_mode is not None: + temporary_path.chmod(destination_mode) + try: + os.replace(temporary_path, destination) + return + except PermissionError as replace_error: + read_only = os.name == "nt" and destination_mode is not None and destination_mode & stat.S_IWRITE == 0 + if ( + not read_only + or getattr(replace_error, "winerror", None) != 5 + or original_descriptor < 0 + or original_attributes is None + ): + raise - assert destination_mode is not None - destination.chmod(destination_mode | stat.S_IWRITE) - try: - os.replace(temporary_path, destination) - except BaseException as replace_error: - if destination_status is None: - raise - identity_matches = _path_identity_state( + if os.fstat(original_descriptor).st_nlink != 1: + raise RuntimeError( + f"Read-only Windows destinations with multiple hard links are not supported: '{destination}'" + ) + _set_windows_file_attributes( + original_descriptor, + original_attributes & ~0x00000001, destination, - destination_status, - require_single_link=False, ) - if identity_matches is False: - raise - if identity_matches is None: - raise RuntimeError( - f"Could not safely restore attributes for '{destination}' because its identity could not be verified" - ) from replace_error try: - destination.chmod(destination_mode) - except OSError as rollback_error: - raise replace_error from rollback_error - raise + os.replace(temporary_path, destination) + except BaseException as replace_error: + try: + _set_windows_file_attributes(original_descriptor, original_attributes, destination) + except OSError as rollback_error: + raise replace_error from rollback_error + raise + finally: + if original_descriptor >= 0: + os.close(original_descriptor) def _set_permissions(descriptor: int, mode: int, path: Path) -> None: diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index cb2b11610..70b93fc3a 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -173,6 +173,17 @@ def test_reject_non_regular_file(tmp_path: Path): read_text_file(fifo) +@pytest.mark.skipif(os.name == "nt", reason="POSIX special files") +def test_reject_non_regular_destination(tmp_path: Path): + fifo = tmp_path / "data.fifo" + os.mkfifo(fifo) + + with pytest.raises(OSError, match="Destination is not a regular file"): + write_text_file_atomically(fifo, "replacement") + + assert stat.S_ISFIFO(fifo.lstat().st_mode) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are not portable to Windows") def test_preserve_destination_permissions(tmp_path: Path): path = tmp_path / "data.txt" @@ -257,9 +268,14 @@ def apply_broader_permissions(descriptor: int, _mode: int) -> None: @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_replace_read_only_destination_on_windows(tmp_path: Path): + if sys.platform != "win32": + raise AssertionError("Windows-only test ran on another platform") path = tmp_path / "data.txt" write_text_file_atomically(path, "original") - path.chmod(stat.S_IREAD) + set_attributes = ctypes.WinDLL("kernel32", use_last_error=True).SetFileAttributesW + set_attributes.argtypes = (wintypes.LPCWSTR, wintypes.DWORD) + set_attributes.restype = wintypes.BOOL + assert set_attributes(str(path), 0x00000001) write_text_file_atomically(path, "replacement") @@ -267,6 +283,22 @@ def test_replace_read_only_destination_on_windows(tmp_path: Path): assert path.stat().st_mode & stat.S_IWRITE == 0 +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") +def test_reject_read_only_destination_with_surviving_hard_link(tmp_path: Path): + path = tmp_path / "data.txt" + alias = tmp_path / "alias.txt" + write_text_file_atomically(path, "original") + os.link(path, alias) + path.chmod(stat.S_IREAD) + + with pytest.raises(RuntimeError, match="multiple hard links"): + write_text_file_atomically(path, "replacement") + + assert read_text_file(path) == "original" + assert read_text_file(alias) == "original" + assert alias.stat().st_mode & stat.S_IWRITE == 0 + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_preserve_writable_destination_on_windows(tmp_path: Path): path = tmp_path / "data.txt" From 450329003fed484151e0fa07029da20bfb2ae23f Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Fri, 21 Aug 2026 23:25:39 -0700 Subject: [PATCH 06/23] Close final FileIO review gaps --- cpp/include/qdk/chemistry/utils/file_io.hpp | 12 ++- cpp/src/qdk/chemistry/utils/file_io.cpp | 37 +++++-- python/src/qdk_chemistry/utils/file_io.py | 103 +++++++++++++++----- python/tests/test_utils_file_io.py | 58 +++++++++++ 4 files changed, 176 insertions(+), 34 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index 52c4e63ac..c553db334 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -40,10 +40,14 @@ std::string read_text_file(const std::filesystem::path& path); * for format-sensitive writers. * * On POSIX, replacing an existing file preserves its ordinary read, write, and - * execute permission bits. New files are created with owner-only permissions. - * The filesystem must enforce POSIX permission bits; the write fails rather - * than publishing a file with broader mode bits. Platform ACLs are not - * inspected and may grant access beyond those bits. + * execute permission bits. Existing destination ACLs and extended attributes + * are not preserved; the replacement uses metadata inherited when its + * temporary file is created and may therefore grant broader access than the + * file it replaced. Callers that rely on explicit ACLs or extended attributes + * must reapply them after the write. New files are created with owner-only + * permissions. The filesystem must enforce POSIX permission bits; the write + * fails rather than publishing a file with broader mode bits. Platform ACLs + * are not inspected and may grant access beyond those bits. * On Windows, replacement preserves the read-only attribute and new files use * the filesystem's standard access controls. Existing Windows security * descriptors and DACLs are not preserved; the replacement uses access diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index ecbd37e0e..7b225dfb4 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -265,7 +265,7 @@ class ReservedTemporaryFile { const std::filesystem::path& path() const { return path_; } bool has_same_identity(const std::filesystem::path& path) const noexcept { - return path_matches_identity(path, true); + return path_matches_identity(path, false); } void verify_identity() const { @@ -521,15 +521,28 @@ ReservedTemporaryFile reserve_temporary_file( void replace_file(const std::filesystem::path& source, const std::filesystem::path& destination) { #ifdef _WIN32 - const DWORD original_attributes = GetFileAttributesW(destination.c_str()); - const bool destination_exists = - original_attributes != INVALID_FILE_ATTRIBUTES; + const DWORD path_attributes = GetFileAttributesW(destination.c_str()); + const bool destination_exists = path_attributes != INVALID_FILE_ATTRIBUTES; HANDLE original_handle_value = INVALID_HANDLE_VALUE; + BY_HANDLE_FILE_INFORMATION original_info{}; + DWORD original_attributes = FILE_ATTRIBUTE_NORMAL; if (destination_exists) { original_handle_value = CreateFileW( destination.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + if (original_handle_value == INVALID_HANDLE_VALUE || + !GetFileInformationByHandle(original_handle_value, &original_info)) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + if (original_handle_value != INVALID_HANDLE_VALUE) { + CloseHandle(original_handle_value); + } + throw std::runtime_error("Could not inspect file attributes for '" + + display_path(destination) + + "': " + error.message()); + } + original_attributes = original_info.dwFileAttributes; } const ScopedReadHandle original_handle(original_handle_value); if (destination_exists && @@ -560,9 +573,7 @@ void replace_file(const std::filesystem::path& source, display_path(destination) + "': " + error.message()); } - BY_HANDLE_FILE_INFORMATION original_info{}; - if (original_handle.get() == INVALID_HANDLE_VALUE || - !GetFileInformationByHandle(original_handle.get(), &original_info)) { + if (original_handle.get() == INVALID_HANDLE_VALUE) { const std::error_code error(static_cast(GetLastError()), std::system_category()); throw std::runtime_error("Could not inspect read-only destination '" + @@ -588,6 +599,18 @@ void replace_file(const std::filesystem::path& source, const bool replaced = move(); const DWORD retry_error = replaced ? ERROR_SUCCESS : GetLastError(); if (replaced) { + BY_HANDLE_FILE_INFORMATION displaced_info{}; + if (GetFileInformationByHandle(original_handle.get(), &displaced_info) && + displaced_info.nNumberOfLinks > 0) { + if (!set_handle_file_attributes(original_handle.get(), + original_attributes)) { + const std::error_code error(static_cast(GetLastError()), + std::system_category()); + throw std::runtime_error( + "Could not restore attributes on the displaced file for '" + + display_path(destination) + "': " + error.message()); + } + } return; } if (!set_handle_file_attributes(original_handle.get(), original_attributes)) { diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 622e0fedc..da9e93d74 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -73,7 +73,7 @@ def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: with os.fdopen(descriptor, "r", encoding=encoding, newline="", closefd=False) as stream: return stream.read() finally: - os.close(descriptor) + _close_descriptor_preserving_error(descriptor) def write_file_atomically( @@ -92,10 +92,15 @@ def write_file_atomically( writers. On POSIX, replacing an existing file preserves its ordinary read, write, - and execute permission bits. New files are created with owner-only - permissions. The filesystem must enforce POSIX permission bits; the write - fails rather than publishing a file with broader mode bits. Platform ACLs - are not inspected and may grant access beyond those bits. On Windows, + and execute permission bits. Existing destination ACLs and extended + attributes are not preserved; the replacement uses metadata inherited when + its temporary file is created and may therefore grant broader access than + the file it replaced. Callers that rely on explicit ACLs or extended + attributes must reapply them after the write. New files are created with + owner-only permissions. The filesystem must enforce POSIX permission bits; + the write fails rather than publishing a file with broader mode bits. + Platform ACLs are not inspected and may grant access beyond those bits. On + Windows, replacement preserves the read-only attribute and new files use the filesystem's standard access controls. Existing Windows security descriptors and DACLs are not preserved; the replacement uses access @@ -159,8 +164,10 @@ def write_file_atomically( temporary_path.chmod(existing_mode) if os.name == "nt": - os.close(descriptor) - descriptor = -1 + owned_descriptor, descriptor = descriptor, -1 + close_error = _close_descriptor(owned_descriptor) + if close_error is not None: + raise close_error _replace_file(temporary_path, destination, destination_mode) except BaseException as error: if reserved_status is None and descriptor >= 0: @@ -169,17 +176,22 @@ def write_file_atomically( except OSError: reserved_status = None if descriptor >= 0: - os.close(descriptor) - descriptor = -1 + owned_descriptor, descriptor = descriptor, -1 + close_error = _close_descriptor(owned_descriptor) + else: + close_error = None if reserved_status is not None and _temporary_path_matches(temporary_path, reserved_status): try: _remove_temporary_file(temporary_path) except OSError as cleanup_error: raise error from cleanup_error + if close_error is not None: + raise error from close_error raise finally: if descriptor >= 0: - os.close(descriptor) + owned_descriptor, descriptor = descriptor, -1 + _close_descriptor_preserving_error(owned_descriptor) def _reserve_temporary_file(destination: Path) -> tuple[int, str]: @@ -239,13 +251,14 @@ def _create_exclusive_file(path: Path) -> int: reserved_status = os.fstat(descriptor) except OSError: reserved_status = None - finally: - os.close(descriptor) + close_error = _close_descriptor(descriptor) if reserved_status is not None and _temporary_path_matches(path, reserved_status): try: path.unlink() except OSError as cleanup_error: raise error from cleanup_error + if close_error is not None: + raise error from close_error raise return descriptor @@ -366,10 +379,10 @@ def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> destination_matches = _path_matches_identity( destination, reserved_status, - require_single_link=True, + require_single_link=False, ) except BaseException as error: - os.close(descriptor) + close_error = _close_descriptor(descriptor) if reserved_status is not None and _path_matches_identity( temporary_path, reserved_status, @@ -379,13 +392,17 @@ def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> _remove_temporary_file(temporary_path) except OSError as cleanup_error: raise error from cleanup_error + if close_error is not None: + raise error from close_error raise if not destination_matches: return descriptor - os.close(descriptor) + close_error = _close_descriptor(descriptor) if _path_matches_identity(temporary_path, reserved_status, require_single_link=False): _remove_temporary_file(temporary_path) + if close_error is not None: + raise close_error return None @@ -447,6 +464,24 @@ def _temporary_path_matches(temporary_path: Path, reserved_status: os.stat_resul return _path_matches_identity(temporary_path, reserved_status, require_single_link=False) +def _close_descriptor(descriptor: int) -> OSError | None: + try: + os.close(descriptor) + except OSError as error: + return error + return None + + +def _close_descriptor_preserving_error(descriptor: int) -> None: + active_error = sys.exc_info()[1] + close_error = _close_descriptor(descriptor) + if close_error is None: + return + if active_error is not None: + raise active_error from close_error + raise close_error + + def _remove_temporary_file(temporary_path: Path) -> None: """Remove a temporary file, including a Windows read-only file.""" try: @@ -483,9 +518,11 @@ def _replace_file( else: try: original_attributes = int(_windows_file_info(original_descriptor, destination).file_attributes) - except BaseException: - os.close(original_descriptor) - original_descriptor = -1 + except BaseException as error: + owned_descriptor, original_descriptor = original_descriptor, -1 + close_error = _close_descriptor(owned_descriptor) + if close_error is not None: + raise error from close_error raise try: if os.name == "nt" and destination_mode is not None: @@ -520,9 +557,21 @@ def _replace_file( except OSError as rollback_error: raise replace_error from rollback_error raise + try: + displaced_status = os.fstat(original_descriptor) + except OSError: + pass + else: + if displaced_status.st_nlink > 0: + _set_windows_file_attributes( + original_descriptor, + original_attributes, + destination, + ) finally: if original_descriptor >= 0: - os.close(original_descriptor) + owned_descriptor, original_descriptor = original_descriptor, -1 + _close_descriptor_preserving_error(owned_descriptor) def _set_permissions(descriptor: int, mode: int, path: Path) -> None: @@ -564,12 +613,20 @@ def _create_private_directories(directory: Path) -> None: | getattr(os, "O_CLOEXEC", 0), ) _set_permissions(descriptor, 0o700, missing_directory) - except BaseException: + except BaseException as error: + close_error = None if descriptor >= 0: - os.close(descriptor) - missing_directory.rmdir() + close_error = _close_descriptor(descriptor) + try: + missing_directory.rmdir() + except OSError as cleanup_error: + raise error from cleanup_error + if close_error is not None: + raise error from close_error raise - os.close(descriptor) + close_error = _close_descriptor(descriptor) + if close_error is not None: + raise close_error def write_text_file_atomically( diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 70b93fc3a..13c2766cf 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -130,6 +130,36 @@ def fail_after_write(temporary_path: Path) -> None: assert list(tmp_path.iterdir()) == [path] +def test_close_failure_does_not_skip_cleanup_or_retry_close( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + real_close = os.close + closed_descriptors: list[int] = [] + + def close_then_fail(descriptor: int) -> None: + closed_descriptors.append(descriptor) + real_close(descriptor) + raise OSError("close failed") + + def fail_after_write(temporary_path: Path) -> None: + temporary_path.write_text("incomplete", encoding="utf-8") + raise RuntimeError("writer failed") + + monkeypatch.setattr(file_io_module.os, "close", close_then_fail) + with pytest.raises(RuntimeError, match="writer failed") as caught: + write_file_atomically(path, fail_after_write) + monkeypatch.undo() + + assert isinstance(caught.value.__cause__, OSError) + assert str(caught.value.__cause__) == "close failed" + assert len(closed_descriptors) == 1 + assert read_text_file(path) == "original" + assert list(tmp_path.iterdir()) == [path] + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_clean_up_read_only_temporary_file_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" @@ -299,6 +329,34 @@ def test_reject_read_only_destination_with_surviving_hard_link(tmp_path: Path): assert alias.stat().st_mode & stat.S_IWRITE == 0 +@pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") +def test_restore_read_only_attribute_on_hard_link_created_during_replace( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + alias = tmp_path / "alias.txt" + write_text_file_atomically(path, "original") + path.chmod(stat.S_IREAD) + real_replace = os.replace + replace_calls = 0 + + def link_before_retry(source: Path, destination: Path) -> None: + nonlocal replace_calls + replace_calls += 1 + if replace_calls == 2: + os.link(destination, alias) + real_replace(source, destination) + + monkeypatch.setattr(file_io_module.os, "replace", link_before_retry) + write_text_file_atomically(path, "replacement") + + assert replace_calls == 2 + assert read_text_file(path) == "replacement" + assert read_text_file(alias) == "original" + assert alias.stat().st_mode & stat.S_IWRITE == 0 + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_preserve_writable_destination_on_windows(tmp_path: Path): path = tmp_path / "data.txt" From a3c4742b7d1d4008bcf2dcadd99f878bc1b62ef7 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 00:07:47 -0700 Subject: [PATCH 07/23] Resolve final FileIO verification findings --- cpp/include/qdk/chemistry/utils/file_io.hpp | 8 +- cpp/src/qdk/chemistry/utils/file_io.cpp | 16 +++ python/src/qdk_chemistry/utils/file_io.py | 108 ++++++++++++++------ python/tests/test_utils_file_io.py | 85 +++++++++++++++ 4 files changed, 182 insertions(+), 35 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index c553db334..59e54c8ca 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -55,9 +55,11 @@ std::string read_text_file(const std::filesystem::path& path); * grant broader access than the file it replaced. Callers that rely on * explicit access-control entries must reapply them after the write. Read-only * Windows destinations with multiple hard links are rejected. Other - * file-object metadata and hard-link identity are not preserved. Atomic - * replacement prevents partial visibility but does not guarantee durability - * after power loss. + * file-object metadata and hard-link identity are not preserved. The named + * temporary file also inherits the parent directory's access controls and may + * therefore be readable while the writer runs or after cleanup fails. Atomic + * replacement prevents partial visibility at the destination path but does not + * guarantee durability after power loss. * Windows alternate data streams are not supported. * * The destination's parent directory and mutable ancestors must not be diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 7b225dfb4..a834aedf8 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -526,11 +526,21 @@ void replace_file(const std::filesystem::path& source, HANDLE original_handle_value = INVALID_HANDLE_VALUE; BY_HANDLE_FILE_INFORMATION original_info{}; DWORD original_attributes = FILE_ATTRIBUTE_NORMAL; + bool can_write_original_attributes = false; if (destination_exists) { original_handle_value = CreateFileW( destination.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + can_write_original_attributes = + original_handle_value != INVALID_HANDLE_VALUE; + if (original_handle_value == INVALID_HANDLE_VALUE && + GetLastError() == ERROR_ACCESS_DENIED) { + original_handle_value = CreateFileW( + destination.c_str(), FILE_READ_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + } if (original_handle_value == INVALID_HANDLE_VALUE || !GetFileInformationByHandle(original_handle_value, &original_info)) { const std::error_code error(static_cast(GetLastError()), @@ -580,6 +590,12 @@ void replace_file(const std::filesystem::path& source, display_path(destination) + "': " + error.message()); } + if (!can_write_original_attributes) { + throw std::runtime_error( + "Could not replace read-only file without permission to change its " + "attributes: '" + + display_path(destination) + "'"); + } if (original_info.nNumberOfLinks != 1) { throw std::runtime_error( "Read-only Windows destinations with multiple hard links are not " diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index da9e93d74..4be756f61 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -34,8 +34,9 @@ def ensure_parent_directory(path: PathLike) -> None: """Create the parent directory of *path* when it does not exist.""" - _validate_destination_path(path) - parent = Path(path).parent + path_value = os.fspath(path) + _validate_destination_path(path_value) + parent = Path(path_value).parent if parent != Path("."): if os.name == "nt": parent.mkdir(parents=True, exist_ok=True) @@ -58,22 +59,31 @@ def _validate_destination_path(path: PathLike) -> None: def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: """Read an entire text file without changing its line endings.""" - _validate_destination_path(path) + path_value = os.fspath(path) + _validate_destination_path(path_value) if sys.platform == "win32": descriptor = _open_windows_file( - path, + path_value, desired_access=0x80000000, creation_disposition=3, ) else: - descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) + descriptor = os.open(path_value, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) + operation_error: BaseException | None = None try: if not stat.S_ISREG(os.fstat(descriptor).st_mode): - raise OSError(f"Path is not a regular file: '{path}'") + raise OSError(f"Path is not a regular file: '{path_value}'") with os.fdopen(descriptor, "r", encoding=encoding, newline="", closefd=False) as stream: return stream.read() + except BaseException as error: + operation_error = error + raise finally: - _close_descriptor_preserving_error(descriptor) + close_error = _close_descriptor(descriptor) + if close_error is not None: + if operation_error is not None: + raise operation_error from close_error + raise close_error def write_file_atomically( @@ -108,10 +118,12 @@ def write_file_atomically( grant broader access than the file it replaced. Callers that rely on explicit access-control entries must reapply them after the write. Read-only Windows destinations with multiple hard links are rejected. - Other file-object metadata and hard-link identity are not preserved. - Atomic replacement prevents partial visibility but does not guarantee - durability after power loss. Windows alternate data streams are not - supported. + Other file-object metadata and hard-link identity are not preserved. The + named temporary file also inherits the parent directory's access controls + and may therefore be readable while the writer runs or after cleanup + fails. Atomic replacement prevents partial visibility at the destination + path but does not guarantee durability after power loss. Windows alternate + data streams are not supported. The destination's parent directory and mutable ancestors must not be writable by principals less privileged than the process performing the @@ -123,8 +135,9 @@ def write_file_atomically( writer runs. A relative path may therefore be rejected when its expanded absolute form exceeds the platform pathname limit. """ - _validate_destination_path(path) - destination = Path(path) + path_value = os.fspath(path) + _validate_destination_path(path_value) + destination = Path(path_value) if not destination.is_absolute(): destination = Path(os.path.abspath(destination)) if os.name == "nt" else Path.cwd() / destination if create_parent_directories: @@ -169,6 +182,11 @@ def write_file_atomically( if close_error is not None: raise close_error _replace_file(temporary_path, destination, destination_mode) + if descriptor >= 0: + owned_descriptor, descriptor = descriptor, -1 + close_error = _close_descriptor(owned_descriptor) + if close_error is not None: + raise close_error except BaseException as error: if reserved_status is None and descriptor >= 0: try: @@ -188,10 +206,6 @@ def write_file_atomically( if close_error is not None: raise error from close_error raise - finally: - if descriptor >= 0: - owned_descriptor, descriptor = descriptor, -1 - _close_descriptor_preserving_error(owned_descriptor) def _reserve_temporary_file(destination: Path) -> tuple[int, str]: @@ -262,7 +276,7 @@ def _create_exclusive_file(path: Path) -> int: raise return descriptor - return _open_windows_file(path, desired_access=0, creation_disposition=1) + return _open_windows_file(path, desired_access=0x00010000, creation_disposition=1) def _open_windows_file( @@ -310,8 +324,22 @@ def _open_windows_file( cast("int", handle), os.O_RDONLY | getattr(os, "O_NOINHERIT", 0), ) - except BaseException: + except BaseException as error: + cleanup_error: OSError | None = None + if creation_disposition == 1: + + class _WindowsFileDispositionInfo(ctypes.Structure): + _fields_ = (("delete_file", ctypes.c_ubyte),) + + disposition = _WindowsFileDispositionInfo(True) + set_info = ctypes.WinDLL("kernel32", use_last_error=True).SetFileInformationByHandle + set_info.argtypes = (wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD) + set_info.restype = wintypes.BOOL + if not set_info(handle, 4, ctypes.byref(disposition), ctypes.sizeof(disposition)): + cleanup_error = _windows_error(path, ctypes.get_last_error()) close_handle(handle) + if cleanup_error is not None: + raise error from cleanup_error raise @@ -472,16 +500,6 @@ def _close_descriptor(descriptor: int) -> OSError | None: return None -def _close_descriptor_preserving_error(descriptor: int) -> None: - active_error = sys.exc_info()[1] - close_error = _close_descriptor(descriptor) - if close_error is None: - return - if active_error is not None: - raise active_error from close_error - raise close_error - - def _remove_temporary_file(temporary_path: Path) -> None: """Remove a temporary file, including a Windows read-only file.""" try: @@ -505,6 +523,8 @@ def _replace_file( """Replace a destination, handling Windows read-only files.""" original_descriptor = -1 original_attributes: int | None = None + operation_error: BaseException | None = None + read_only = False if os.name == "nt" and destination_mode is not None: try: original_descriptor = _open_windows_file( @@ -526,12 +546,19 @@ def _replace_file( raise try: if os.name == "nt" and destination_mode is not None: - temporary_path.chmod(destination_mode) + read_only = ( + original_attributes & 0x00000001 != 0 + if original_attributes is not None + else destination_mode & stat.S_IWRITE == 0 + ) + _set_windows_path_attributes( + temporary_path, + 0x00000001 if read_only else 0x00000080, + ) try: os.replace(temporary_path, destination) return except PermissionError as replace_error: - read_only = os.name == "nt" and destination_mode is not None and destination_mode & stat.S_IWRITE == 0 if ( not read_only or getattr(replace_error, "winerror", None) != 5 @@ -568,10 +595,27 @@ def _replace_file( original_attributes, destination, ) + except BaseException as error: + operation_error = error + raise finally: if original_descriptor >= 0: owned_descriptor, original_descriptor = original_descriptor, -1 - _close_descriptor_preserving_error(owned_descriptor) + close_error = _close_descriptor(owned_descriptor) + if close_error is not None: + if operation_error is not None: + raise operation_error from close_error + raise close_error + + +def _set_windows_path_attributes(path: Path, attributes: int) -> None: + if sys.platform != "win32": + raise NotImplementedError("Windows file attributes require Windows") + set_attributes = ctypes.WinDLL("kernel32", use_last_error=True).SetFileAttributesW + set_attributes.argtypes = (wintypes.LPCWSTR, wintypes.DWORD) + set_attributes.restype = wintypes.BOOL + if not set_attributes(os.fspath(path), attributes): + raise _windows_error(path, ctypes.get_last_error()) def _set_permissions(descriptor: int, mode: int, path: Path) -> None: diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 13c2766cf..a7f2281b8 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -34,6 +34,23 @@ def test_write_read_and_replace_text(tmp_path: Path): assert read_text_file(path) == "second" +def test_resolve_pathlike_once(tmp_path: Path): + path = tmp_path / "data.txt" + + class ChangingPath: + calls = 0 + + def __fspath__(self) -> str: + self.calls += 1 + return str(path) if self.calls == 1 else "invalid\0path" + + changing_path = ChangingPath() + write_text_file_atomically(changing_path, "contents") + + assert changing_path.calls == 1 + assert read_text_file(path) == "contents" + + def test_create_parent_directories_when_requested(tmp_path: Path): path = tmp_path / "nested" / "directory" / "data.txt" @@ -160,6 +177,28 @@ def fail_after_write(temporary_path: Path) -> None: assert list(tmp_path.iterdir()) == [path] +def test_close_failure_does_not_resurrect_callers_exception( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + real_close = os.close + + def close_then_fail(descriptor: int) -> None: + real_close(descriptor) + raise OSError("close failed") + + monkeypatch.setattr(file_io_module.os, "close", close_then_fail) + try: + raise ValueError("caller error") + except ValueError: + with pytest.raises(OSError, match="close failed"): + write_text_file_atomically(path, "contents") + monkeypatch.undo() + + assert read_text_file(path) == "contents" + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_clean_up_read_only_temporary_file_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" @@ -372,6 +411,52 @@ def write_read_only_temporary_file(temporary_path: Path) -> None: assert path.stat().st_mode & stat.S_IWRITE != 0 +@pytest.mark.skipif(os.name != "nt", reason="Windows file attributes") +def test_strip_temporary_attributes_from_replacement_on_windows(tmp_path: Path): + if sys.platform != "win32": + raise AssertionError("Windows-only test ran on another platform") + path = tmp_path / "data.txt" + write_text_file_atomically(path, "original") + set_attributes = ctypes.WinDLL("kernel32", use_last_error=True).SetFileAttributesW + set_attributes.argtypes = (wintypes.LPCWSTR, wintypes.DWORD) + set_attributes.restype = wintypes.BOOL + + def write_temporary_file(temporary_path: Path) -> None: + temporary_path.write_text("replacement", encoding="utf-8") + assert set_attributes(str(temporary_path), 0x00000002 | 0x00000100) + + write_file_atomically(path, write_temporary_file) + + get_attributes = ctypes.WinDLL("kernel32", use_last_error=True).GetFileAttributesW + get_attributes.argtypes = (wintypes.LPCWSTR,) + get_attributes.restype = wintypes.DWORD + assert get_attributes(str(path)) & (0x00000002 | 0x00000100) == 0 + + +@pytest.mark.skipif(os.name != "nt", reason="Windows descriptor conversion") +def test_remove_created_file_when_windows_descriptor_conversion_fails( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + + class FailingMsvcrt: + @staticmethod + def open_osfhandle(_handle: int, _flags: int) -> int: + raise OSError("descriptor conversion failed") + + monkeypatch.setattr(file_io_module.importlib, "import_module", lambda _name: FailingMsvcrt) + + with pytest.raises(OSError, match="descriptor conversion failed"): + file_io_module._open_windows_file( + path, + desired_access=0x00010000, + creation_disposition=1, + ) + + assert not path.exists() + + @pytest.mark.skipif(os.name != "nt", reason="Windows file-sharing behavior") def test_allow_exclusive_writer_on_windows(tmp_path: Path): if sys.platform != "win32": From f69fbbe01db2cf3981c08b6add19ea2e4ae8f967 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 00:24:55 -0700 Subject: [PATCH 08/23] Preserve Windows writer sharing --- python/src/qdk_chemistry/utils/file_io.py | 18 ++++++------------ python/tests/test_utils_file_io.py | 2 +- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 4be756f61..291d5606b 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -276,7 +276,7 @@ def _create_exclusive_file(path: Path) -> int: raise return descriptor - return _open_windows_file(path, desired_access=0x00010000, creation_disposition=1) + return _open_windows_file(path, desired_access=0, creation_disposition=1) def _open_windows_file( @@ -326,18 +326,12 @@ def _open_windows_file( ) except BaseException as error: cleanup_error: OSError | None = None - if creation_disposition == 1: - - class _WindowsFileDispositionInfo(ctypes.Structure): - _fields_ = (("delete_file", ctypes.c_ubyte),) - - disposition = _WindowsFileDispositionInfo(True) - set_info = ctypes.WinDLL("kernel32", use_last_error=True).SetFileInformationByHandle - set_info.argtypes = (wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD) - set_info.restype = wintypes.BOOL - if not set_info(handle, 4, ctypes.byref(disposition), ctypes.sizeof(disposition)): - cleanup_error = _windows_error(path, ctypes.get_last_error()) close_handle(handle) + if creation_disposition == 1: + try: + _remove_temporary_file(Path(path)) + except OSError as caught_error: + cleanup_error = caught_error if cleanup_error is not None: raise error from cleanup_error raise diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index a7f2281b8..d871cef38 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -450,7 +450,7 @@ def open_osfhandle(_handle: int, _flags: int) -> int: with pytest.raises(OSError, match="descriptor conversion failed"): file_io_module._open_windows_file( path, - desired_access=0x00010000, + desired_access=0, creation_disposition=1, ) From 5bb0e841760c45595f8f07b24df25b63a76fd847 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 00:45:33 -0700 Subject: [PATCH 09/23] Guard Windows handle setup --- python/src/qdk_chemistry/utils/file_io.py | 14 +++++++------- python/tests/test_utils_file_io.py | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 291d5606b..5db557809 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -300,6 +300,13 @@ def _open_windows_file( wintypes.HANDLE, ) create_file.restype = wintypes.HANDLE + close_handle = ctypes.WinDLL("kernel32", use_last_error=True).CloseHandle + close_handle.argtypes = (wintypes.HANDLE,) + close_handle.restype = wintypes.BOOL + open_osfhandle = cast( + "Callable[[int, int], int]", + importlib.import_module("msvcrt").open_osfhandle, + ) handle = create_file( os.fspath(path), desired_access, @@ -312,13 +319,6 @@ def _open_windows_file( if handle == wintypes.HANDLE(-1).value: error = ctypes.get_last_error() raise _windows_error(path, error) - close_handle = ctypes.WinDLL("kernel32", use_last_error=True).CloseHandle - close_handle.argtypes = (wintypes.HANDLE,) - close_handle.restype = wintypes.BOOL - open_osfhandle = cast( - "Callable[[int, int], int]", - importlib.import_module("msvcrt").open_osfhandle, - ) try: return open_osfhandle( cast("int", handle), diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index d871cef38..095775046 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -457,6 +457,28 @@ def open_osfhandle(_handle: int, _flags: int) -> int: assert not path.exists() +@pytest.mark.skipif(os.name != "nt", reason="Windows descriptor conversion") +def test_resolve_windows_descriptor_converter_before_creating_file( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + path = tmp_path / "data.txt" + + def fail_import(_name: str) -> None: + raise OSError("converter unavailable") + + monkeypatch.setattr(file_io_module.importlib, "import_module", fail_import) + + with pytest.raises(OSError, match="converter unavailable"): + file_io_module._open_windows_file( + path, + desired_access=0, + creation_disposition=1, + ) + + assert not path.exists() + + @pytest.mark.skipif(os.name != "nt", reason="Windows file-sharing behavior") def test_allow_exclusive_writer_on_windows(tmp_path: Path): if sys.platform != "win32": From 9421ecab3a5bd5bebd8326472e79df3c7e7c7252 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 01:14:26 -0700 Subject: [PATCH 10/23] Own FileIO reservations immediately --- cpp/src/qdk/chemistry/utils/file_io.cpp | 21 +++++---- python/src/qdk_chemistry/utils/file_io.py | 53 +++++++++++++++++------ 2 files changed, 52 insertions(+), 22 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index a834aedf8..e778e3bbc 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -393,37 +393,40 @@ class ReservedTemporaryFile { ReservedTemporaryFile create_exclusive_file(const std::filesystem::path& path, std::error_code& error) { + std::filesystem::path owned_path(path); #ifdef _WIN32 - HANDLE handle = CreateFileW( - path.c_str(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); + HANDLE handle = + CreateFileW(owned_path.c_str(), 0, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); if (handle == INVALID_HANDLE_VALUE) { error = std::error_code(static_cast(GetLastError()), std::system_category()); - return {path, ReservedTemporaryFile::invalid_handle()}; + return {std::move(owned_path), ReservedTemporaryFile::invalid_handle()}; } #else const int descriptor = retry_on_eintr([&] { - return ::open(path.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, 0600); + return ::open(owned_path.c_str(), O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC, + 0600); }); if (descriptor == -1) { error = std::error_code(errno, std::generic_category()); - return {path, ReservedTemporaryFile::invalid_handle()}; + return {std::move(owned_path), ReservedTemporaryFile::invalid_handle()}; } if (retry_on_eintr([&] { return ::fchmod(descriptor, S_IRUSR | S_IWUSR); }) != 0) { const int permission_error = errno; error = std::error_code(permission_error, std::generic_category()); - return {path, descriptor}; + return {std::move(owned_path), descriptor}; } struct stat status{}; if (retry_on_eintr([&] { return ::fstat(descriptor, &status); }) != 0 || (status.st_mode & 0777) != (S_IRUSR | S_IWUSR)) { error = std::make_error_code(std::errc::permission_denied); - return {path, descriptor}; + return {std::move(owned_path), descriptor}; } #endif - return {path, + return {std::move(owned_path), #ifdef _WIN32 handle #else diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 5db557809..b116e5387 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -147,13 +147,13 @@ def write_file_atomically( if not parent.is_dir(): raise FileNotFoundError(f"Parent directory does not exist for '{destination}'") - descriptor, temporary_name = _reserve_temporary_file(destination) - temporary_path = Path(temporary_name) + descriptor = -1 + temporary_path: Path | None = None reserved_status: os.stat_result | None = None try: + descriptor, temporary_path, reserved_status = _reserve_temporary_file(destination) writer(temporary_path) - reserved_status = os.fstat(descriptor) current_status = temporary_path.lstat() if not _same_file_identity(reserved_status, current_status): raise RuntimeError(f"Temporary file identity changed: '{temporary_path}'") @@ -198,7 +198,11 @@ def write_file_atomically( close_error = _close_descriptor(owned_descriptor) else: close_error = None - if reserved_status is not None and _temporary_path_matches(temporary_path, reserved_status): + if ( + temporary_path is not None + and reserved_status is not None + and _temporary_path_matches(temporary_path, reserved_status) + ): try: _remove_temporary_file(temporary_path) except OSError as cleanup_error: @@ -208,7 +212,7 @@ def write_file_atomically( raise -def _reserve_temporary_file(destination: Path) -> tuple[int, str]: +def _reserve_temporary_file(destination: Path) -> tuple[int, Path, os.stat_result]: """Reserve a private temporary sibling and keep its descriptor open.""" suffix = "".join(destination.suffixes) for _ in range(64): @@ -216,15 +220,15 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: if _component_is_too_long(temporary_path): break try: - descriptor = _reserve_distinct_temporary_file(destination, temporary_path) + reservation = _reserve_distinct_temporary_file(destination, temporary_path) except FileExistsError: continue except OSError as error: if not _is_name_too_long(error, destination): raise break - if descriptor is not None: - return descriptor, str(temporary_path) + if reservation is not None: + return reservation alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-" for stem_length in range(16, 0, -1): @@ -236,19 +240,38 @@ def _reserve_temporary_file(destination: Path) -> tuple[int, str]: if _component_is_too_long(temporary_path): break try: - descriptor = _reserve_distinct_temporary_file(destination, temporary_path) + reservation = _reserve_distinct_temporary_file(destination, temporary_path) except FileExistsError: continue except OSError as error: if _is_name_too_long(error, destination): break raise - if descriptor is not None: - return descriptor, str(temporary_path) + if reservation is not None: + return reservation raise FileExistsError(f"Could not create a unique temporary file beside '{destination}'") +def _package_reservation( + descriptor: int, + temporary_path: Path, + reserved_status: os.stat_result, +) -> tuple[int, Path, os.stat_result]: + try: + return descriptor, temporary_path, reserved_status + except BaseException as error: + close_error = _close_descriptor(descriptor) + if _temporary_path_matches(temporary_path, reserved_status): + try: + _remove_temporary_file(temporary_path) + except OSError as cleanup_error: + raise error from cleanup_error + if close_error is not None: + raise error from close_error + raise + + def _component_is_too_long(path: Path) -> bool: if os.name != "nt": return False @@ -393,7 +416,10 @@ def _set_windows_file_attributes(descriptor: int, attributes: int, path: Path) - raise _windows_error(path, ctypes.get_last_error()) -def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> int | None: +def _reserve_distinct_temporary_file( + destination: Path, + temporary_path: Path, +) -> tuple[int, Path, os.stat_result] | None: descriptor = _create_exclusive_file(temporary_path) reserved_status: os.stat_result | None = None try: @@ -417,8 +443,9 @@ def _reserve_distinct_temporary_file(destination: Path, temporary_path: Path) -> if close_error is not None: raise error from close_error raise + assert reserved_status is not None if not destination_matches: - return descriptor + return _package_reservation(descriptor, temporary_path, reserved_status) close_error = _close_descriptor(descriptor) if _path_matches_identity(temporary_path, reserved_status, require_single_link=False): From b463d279c43e8ea168757b87f7fa36995393124a Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 01:33:59 -0700 Subject: [PATCH 11/23] Keep Python reservation ownership intact --- python/src/qdk_chemistry/utils/file_io.py | 53 +++++++++++++++++++---- python/tests/test_utils_file_io.py | 15 +++++++ 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index b116e5387..f112fdefb 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -32,6 +32,35 @@ ] +class _TemporaryFileReservation: + def __init__( + self, + descriptor: int, + path: Path, + status: os.stat_result, + ) -> None: + self.descriptor = descriptor + self.path = path + self.status = status + self.cleanup = True + + def take_descriptor(self) -> int: + descriptor, self.descriptor = self.descriptor, -1 + return descriptor + + def disarm(self) -> None: + self.cleanup = False + + def __del__(self) -> None: + try: + if self.descriptor >= 0: + _close_descriptor(self.take_descriptor()) + if self.cleanup and _temporary_path_matches(self.path, self.status): + _remove_temporary_file(self.path) + except BaseException: # noqa: BLE001 + pass + + def ensure_parent_directory(path: PathLike) -> None: """Create the parent directory of *path* when it does not exist.""" path_value = os.fspath(path) @@ -150,9 +179,13 @@ def write_file_atomically( descriptor = -1 temporary_path: Path | None = None reserved_status: os.stat_result | None = None + reservation: _TemporaryFileReservation | None = None try: - descriptor, temporary_path, reserved_status = _reserve_temporary_file(destination) + reservation = _reserve_temporary_file(destination) + descriptor = reservation.descriptor + temporary_path = reservation.path + reserved_status = reservation.status writer(temporary_path) current_status = temporary_path.lstat() if not _same_file_identity(reserved_status, current_status): @@ -177,16 +210,17 @@ def write_file_atomically( temporary_path.chmod(existing_mode) if os.name == "nt": - owned_descriptor, descriptor = descriptor, -1 + owned_descriptor, descriptor = reservation.take_descriptor(), -1 close_error = _close_descriptor(owned_descriptor) if close_error is not None: raise close_error _replace_file(temporary_path, destination, destination_mode) if descriptor >= 0: - owned_descriptor, descriptor = descriptor, -1 + owned_descriptor, descriptor = reservation.take_descriptor(), -1 close_error = _close_descriptor(owned_descriptor) if close_error is not None: raise close_error + reservation.disarm() except BaseException as error: if reserved_status is None and descriptor >= 0: try: @@ -194,7 +228,8 @@ def write_file_atomically( except OSError: reserved_status = None if descriptor >= 0: - owned_descriptor, descriptor = descriptor, -1 + owned_descriptor = reservation.take_descriptor() if reservation is not None else descriptor + descriptor = -1 close_error = _close_descriptor(owned_descriptor) else: close_error = None @@ -209,10 +244,12 @@ def write_file_atomically( raise error from cleanup_error if close_error is not None: raise error from close_error + if reservation is not None: + reservation.disarm() raise -def _reserve_temporary_file(destination: Path) -> tuple[int, Path, os.stat_result]: +def _reserve_temporary_file(destination: Path) -> _TemporaryFileReservation: """Reserve a private temporary sibling and keep its descriptor open.""" suffix = "".join(destination.suffixes) for _ in range(64): @@ -257,9 +294,9 @@ def _package_reservation( descriptor: int, temporary_path: Path, reserved_status: os.stat_result, -) -> tuple[int, Path, os.stat_result]: +) -> _TemporaryFileReservation: try: - return descriptor, temporary_path, reserved_status + return _TemporaryFileReservation(descriptor, temporary_path, reserved_status) except BaseException as error: close_error = _close_descriptor(descriptor) if _temporary_path_matches(temporary_path, reserved_status): @@ -419,7 +456,7 @@ def _set_windows_file_attributes(descriptor: int, attributes: int, path: Path) - def _reserve_distinct_temporary_file( destination: Path, temporary_path: Path, -) -> tuple[int, Path, os.stat_result] | None: +) -> _TemporaryFileReservation | None: descriptor = _create_exclusive_file(temporary_path) reserved_status: os.stat_result | None = None try: diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 095775046..6ed35b330 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -6,6 +6,7 @@ # -------------------------------------------------------------------------------------------- import ctypes +import gc import os import stat import sys @@ -199,6 +200,20 @@ def close_then_fail(descriptor: int) -> None: assert read_text_file(path) == "contents" +def test_reservation_finalizer_closes_and_removes_temporary_file(tmp_path: Path): + destination = tmp_path / "data.txt" + reservation = file_io_module._reserve_temporary_file(destination) + descriptor = reservation.descriptor + temporary_path = reservation.path + + del reservation + gc.collect() + + with pytest.raises(OSError, match="(?i)(bad file descriptor|handle is invalid)"): + os.fstat(descriptor) + assert not temporary_path.exists() + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_clean_up_read_only_temporary_file_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" From 509c0aebe76398c303cd76edff3f89e3afae4d18 Mon Sep 17 00:00:00 2001 From: Conrad Johnston Date: Sat, 22 Aug 2026 01:53:43 -0700 Subject: [PATCH 12/23] Make reservation adoption single-owner --- python/src/qdk_chemistry/utils/file_io.py | 14 +++++++--- python/tests/test_utils_file_io.py | 31 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index f112fdefb..1f7e053c1 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -35,15 +35,17 @@ class _TemporaryFileReservation: def __init__( self, - descriptor: int, path: Path, status: os.stat_result, ) -> None: - self.descriptor = descriptor + self.descriptor = -1 self.path = path self.status = status self.cleanup = True + def adopt_descriptor(self, descriptor: int) -> None: + self.descriptor = descriptor + def take_descriptor(self) -> int: descriptor, self.descriptor = self.descriptor, -1 return descriptor @@ -295,9 +297,15 @@ def _package_reservation( temporary_path: Path, reserved_status: os.stat_result, ) -> _TemporaryFileReservation: + reservation: _TemporaryFileReservation | None = None try: - return _TemporaryFileReservation(descriptor, temporary_path, reserved_status) + reservation = _TemporaryFileReservation(temporary_path, reserved_status) + reservation.adopt_descriptor(descriptor) + return reservation except BaseException as error: + if reservation is not None and reservation.descriptor >= 0: + descriptor = reservation.take_descriptor() + reservation.disarm() close_error = _close_descriptor(descriptor) if _temporary_path_matches(temporary_path, reserved_status): try: diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 6ed35b330..c21ff91d1 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -214,6 +214,37 @@ def test_reservation_finalizer_closes_and_removes_temporary_file(tmp_path: Path) assert not temporary_path.exists() +def test_failed_reservation_adoption_has_one_descriptor_owner( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + temporary_path = tmp_path / "temporary.txt" + descriptor = file_io_module._create_exclusive_file(temporary_path) + reserved_status = os.fstat(descriptor) + reservations: list[file_io_module._TemporaryFileReservation] = [] + adopt_descriptor = file_io_module._TemporaryFileReservation.adopt_descriptor + + def adopt_then_fail( + reservation: file_io_module._TemporaryFileReservation, + owned_descriptor: int, + ) -> None: + adopt_descriptor(reservation, owned_descriptor) + reservations.append(reservation) + raise MemoryError("adoption failed") + + monkeypatch.setattr( + file_io_module._TemporaryFileReservation, + "adopt_descriptor", + adopt_then_fail, + ) + + with pytest.raises(MemoryError, match="adoption failed"): + file_io_module._package_reservation(descriptor, temporary_path, reserved_status) + + assert reservations[0].descriptor == -1 + assert not temporary_path.exists() + + @pytest.mark.skipif(os.name != "nt", reason="Windows read-only behavior") def test_clean_up_read_only_temporary_file_when_writer_fails(tmp_path: Path): path = tmp_path / "data.txt" From 319863fe32f4c39dd57212d24801d7b7e8aad403 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Mon, 24 Aug 2026 21:56:41 +0000 Subject: [PATCH 13/23] Fix FileIO review findings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/include/qdk/chemistry/utils/file_io.hpp | 3 +- cpp/src/qdk/chemistry/utils/file_io.cpp | 73 ++++++++----- cpp/tests/test_file_io.cpp | 46 ++++++++ python/src/qdk_chemistry/utils/file_io.py | 41 +++++-- python/tests/test_utils_file_io.py | 115 ++++++++++++++++++++ 5 files changed, 238 insertions(+), 40 deletions(-) diff --git a/cpp/include/qdk/chemistry/utils/file_io.hpp b/cpp/include/qdk/chemistry/utils/file_io.hpp index 59e54c8ca..d874671d6 100644 --- a/cpp/include/qdk/chemistry/utils/file_io.hpp +++ b/cpp/include/qdk/chemistry/utils/file_io.hpp @@ -18,7 +18,8 @@ using AtomicFileWriter = * @brief Create the parent directory of a path when it does not exist. * * A path without a parent component refers to the current directory and needs - * no action. + * no action. Relative paths are frozen to an absolute path before creation + * begins. */ void ensure_parent_directory(const std::filesystem::path& path); diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index e778e3bbc..2433c716c 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -99,6 +99,16 @@ void validate_path(const std::filesystem::path& path) { #endif } +std::filesystem::path freeze_path(const std::filesystem::path& path) { + std::error_code error; + const auto frozen_path = std::filesystem::absolute(path, error); + if (error) { + throw std::runtime_error("Could not resolve absolute path for '" + + display_path(path) + "': " + error.message()); + } + return frozen_path; +} + #ifdef _WIN32 enum class IdentityMatch { match, different, unknown }; @@ -524,33 +534,44 @@ ReservedTemporaryFile reserve_temporary_file( void replace_file(const std::filesystem::path& source, const std::filesystem::path& destination) { #ifdef _WIN32 - const DWORD path_attributes = GetFileAttributesW(destination.c_str()); - const bool destination_exists = path_attributes != INVALID_FILE_ATTRIBUTES; + bool destination_exists = true; HANDLE original_handle_value = INVALID_HANDLE_VALUE; BY_HANDLE_FILE_INFORMATION original_info{}; DWORD original_attributes = FILE_ATTRIBUTE_NORMAL; bool can_write_original_attributes = false; - if (destination_exists) { + original_handle_value = CreateFileW( + destination.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, + OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + can_write_original_attributes = original_handle_value != INVALID_HANDLE_VALUE; + DWORD inspection_error = + can_write_original_attributes ? ERROR_SUCCESS : GetLastError(); + if (original_handle_value == INVALID_HANDLE_VALUE && + inspection_error == ERROR_ACCESS_DENIED) { original_handle_value = CreateFileW( - destination.c_str(), FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, + destination.c_str(), FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); - can_write_original_attributes = - original_handle_value != INVALID_HANDLE_VALUE; - if (original_handle_value == INVALID_HANDLE_VALUE && - GetLastError() == ERROR_ACCESS_DENIED) { - original_handle_value = CreateFileW( - destination.c_str(), FILE_READ_ATTRIBUTES, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr, - OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT, nullptr); + inspection_error = original_handle_value == INVALID_HANDLE_VALUE + ? GetLastError() + : ERROR_SUCCESS; + } + if (original_handle_value == INVALID_HANDLE_VALUE) { + if (inspection_error == ERROR_FILE_NOT_FOUND || + inspection_error == ERROR_PATH_NOT_FOUND) { + destination_exists = false; + } else { + const std::error_code error(static_cast(inspection_error), + std::system_category()); + throw std::runtime_error("Could not inspect file attributes for '" + + display_path(destination) + + "': " + error.message()); } - if (original_handle_value == INVALID_HANDLE_VALUE || - !GetFileInformationByHandle(original_handle_value, &original_info)) { + } else { + if (!GetFileInformationByHandle(original_handle_value, &original_info)) { const std::error_code error(static_cast(GetLastError()), std::system_category()); - if (original_handle_value != INVALID_HANDLE_VALUE) { - CloseHandle(original_handle_value); - } + CloseHandle(original_handle_value); throw std::runtime_error("Could not inspect file attributes for '" + display_path(destination) + "': " + error.message()); @@ -786,7 +807,7 @@ void ensure_parent_directory(const std::filesystem::path& path) { return; } - create_private_directories(parent); + create_private_directories(freeze_path(path).parent_path()); } std::string read_text_file(const std::filesystem::path& path) { @@ -827,8 +848,12 @@ std::string read_text_file(const std::filesystem::path& path) { contents.append(buffer.data(), bytes_read); } #else - const int descriptor = retry_on_eintr( - [&] { return ::open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC); }); + int open_flags = O_RDONLY | O_NONBLOCK | O_CLOEXEC; +#ifdef O_NOCTTY + open_flags |= O_NOCTTY; +#endif + const int descriptor = + retry_on_eintr([&] { return ::open(path.c_str(), open_flags); }); if (descriptor == -1) { throw std::runtime_error("Could not open file for reading: '" + display_path(path) + "'"); @@ -867,13 +892,7 @@ void write_file_atomically(const std::filesystem::path& path, const AtomicFileWriter& writer, bool create_parent_directories) { validate_path(path); - std::error_code absolute_error; - const auto destination = std::filesystem::absolute(path, absolute_error); - if (absolute_error) { - throw std::runtime_error("Could not resolve absolute path for '" + - display_path(path) + - "': " + absolute_error.message()); - } + const auto destination = freeze_path(path); if (create_parent_directories) { ensure_parent_directory(destination); diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index 26fddd735..e366c1be0 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include #include #include #include @@ -16,6 +18,7 @@ #ifndef _WIN32 #include #include +#include #include #else #ifndef NOMINMAX @@ -171,6 +174,49 @@ TEST_F(FileIoTest, RejectsDirectoryReads) { std::runtime_error); } +#ifndef _WIN32 +TEST_F(FileIoTest, DoesNotAcquireControllingTerminalWhenRejectingTerminal) { + const int master_descriptor = ::posix_openpt(O_RDWR | O_NOCTTY | O_CLOEXEC); + ASSERT_NE(master_descriptor, -1); + ASSERT_EQ(::grantpt(master_descriptor), 0); + ASSERT_EQ(::unlockpt(master_descriptor), 0); + const char* slave_name = ::ptsname(master_descriptor); + ASSERT_NE(slave_name, nullptr); + const std::string slave_path(slave_name); + + const pid_t child = ::fork(); + ASSERT_NE(child, -1); + if (child == 0) { + ::close(master_descriptor); + if (::setsid() == -1) { + _exit(2); + } + try { + static_cast(qdk::chemistry::utils::read_text_file(slave_path)); + _exit(3); + } catch (const std::runtime_error& error) { + if (std::string(error.what()).find("not a regular file") == + std::string::npos) { + _exit(5); + } + } + const int terminal_descriptor = + ::open("/dev/tty", O_RDONLY | O_NOCTTY | O_CLOEXEC); + if (terminal_descriptor != -1) { + ::close(terminal_descriptor); + _exit(4); + } + _exit(errno == ENXIO ? 0 : 6); + } + + int status = 0; + ASSERT_EQ(::waitpid(child, &status, 0), child); + ::close(master_descriptor); + ASSERT_TRUE(WIFEXITED(status)); + EXPECT_EQ(WEXITSTATUS(status), 0); +} +#endif + TEST_F(FileIoTest, PreservesDestinationPermissions) { #ifndef _WIN32 const auto path = root_ / "data.txt"; diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 1f7e053c1..caa725c14 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -63,16 +63,27 @@ def __del__(self) -> None: pass +def _freeze_path(path_value: str) -> Path: + destination = Path(path_value) + if destination.is_absolute(): + return destination + return Path(os.path.abspath(destination)) if os.name == "nt" else Path.cwd() / destination + + def ensure_parent_directory(path: PathLike) -> None: - """Create the parent directory of *path* when it does not exist.""" + """Create the parent directory of *path* when it does not exist. + + Relative paths are frozen to an absolute path before creation begins. + """ path_value = os.fspath(path) _validate_destination_path(path_value) - parent = Path(path_value).parent - if parent != Path("."): - if os.name == "nt": - parent.mkdir(parents=True, exist_ok=True) - else: - _create_private_directories(parent) + if Path(path_value).parent == Path("."): + return + parent = _freeze_path(path_value).parent + if os.name == "nt": + parent.mkdir(parents=True, exist_ok=True) + else: + _create_private_directories(parent) def _validate_destination_path(path: PathLike) -> None: @@ -99,7 +110,10 @@ def read_text_file(path: PathLike, *, encoding: str = "utf-8") -> str: creation_disposition=3, ) else: - descriptor = os.open(path_value, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) + descriptor = os.open( + path_value, + os.O_RDONLY | getattr(os, "O_NONBLOCK", 0) | getattr(os, "O_NOCTTY", 0), + ) operation_error: BaseException | None = None try: if not stat.S_ISREG(os.fstat(descriptor).st_mode): @@ -168,9 +182,7 @@ def write_file_atomically( """ path_value = os.fspath(path) _validate_destination_path(path_value) - destination = Path(path_value) - if not destination.is_absolute(): - destination = Path(os.path.abspath(destination)) if os.name == "nt" else Path.cwd() / destination + destination = _freeze_path(path_value) if create_parent_directories: ensure_parent_directory(destination) @@ -320,7 +332,7 @@ def _package_reservation( def _component_is_too_long(path: Path) -> bool: if os.name != "nt": return False - return len(path.name.encode("utf-16-le")) // 2 > 255 + return len(path.name.encode("utf-16-le", errors="surrogatepass")) // 2 > 255 def _create_exclusive_file(path: Path) -> int: @@ -475,6 +487,11 @@ def _reserve_distinct_temporary_file( require_single_link=False, ) except BaseException as error: + if reserved_status is None: + try: + reserved_status = os.fstat(descriptor) + except OSError: + reserved_status = None close_error = _close_descriptor(descriptor) if reserved_status is not None and _path_matches_identity( temporary_path, diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index c21ff91d1..0ed4c86c2 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -6,6 +6,7 @@ # -------------------------------------------------------------------------------------------- import ctypes +import errno import gc import os import stat @@ -63,6 +64,36 @@ def test_create_parent_directories_when_requested(tmp_path: Path): assert stat.S_IMODE(path.parent.parent.stat().st_mode) == 0o700 +def test_freeze_relative_parent_before_creation(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + first_directory = tmp_path / "first" + second_directory = tmp_path / "second" + first_directory.mkdir() + second_directory.mkdir() + monkeypatch.chdir(first_directory) + + if os.name == "nt": + mkdir = Path.mkdir + + def change_directory_then_create(directory: Path, *args, **kwargs) -> None: + os.chdir(second_directory) + mkdir(directory, *args, **kwargs) + + monkeypatch.setattr(Path, "mkdir", change_directory_then_create) + else: + create_private_directories = file_io_module._create_private_directories + + def change_directory_then_create(directory: Path) -> None: + os.chdir(second_directory) + create_private_directories(directory) + + monkeypatch.setattr(file_io_module, "_create_private_directories", change_directory_then_create) + + ensure_parent_directory("nested/data.txt") + + assert (first_directory / "nested").is_dir() + assert not (second_directory / "nested").exists() + + @pytest.mark.skipif(os.name == "nt", reason="POSIX umask semantics") def test_create_private_parent_directories_under_restrictive_umask(tmp_path: Path): path = tmp_path / "private" / "nested" / "data.txt" @@ -214,6 +245,38 @@ def test_reservation_finalizer_closes_and_removes_temporary_file(tmp_path: Path) assert not temporary_path.exists() +def test_clean_up_after_initial_reservation_fstat_failure( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + destination = tmp_path / "data.txt" + fstat = file_io_module.os.fstat + fstat_calls = 0 + closed_descriptors: list[int] = [] + close_descriptor = file_io_module._close_descriptor + + def fail_initial_reservation_fstat(descriptor: int) -> os.stat_result: + nonlocal fstat_calls + fstat_calls += 1 + if fstat_calls == 2: + raise OSError("identity snapshot failed") + return fstat(descriptor) + + def record_close(descriptor: int) -> OSError | None: + closed_descriptors.append(descriptor) + return close_descriptor(descriptor) + + monkeypatch.setattr(file_io_module.os, "fstat", fail_initial_reservation_fstat) + monkeypatch.setattr(file_io_module, "_close_descriptor", record_close) + + with pytest.raises(OSError, match="identity snapshot failed"): + write_text_file_atomically(destination, "contents") + + assert fstat_calls == 3 + assert len(closed_descriptors) == 1 + assert list(tmp_path.iterdir()) == [] + + def test_failed_reservation_adoption_has_one_descriptor_owner( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -616,6 +679,19 @@ def test_reject_alternate_data_stream_destination_on_windows(tmp_path: Path): assert not (tmp_path / "data.txt").exists() +def test_count_windows_surrogate_code_units(monkeypatch: pytest.MonkeyPatch): + short_path = Path("data.\ud800") + long_path = Path("data." + "\ud800" * 256) + + class WindowsOs: + name = "nt" + + monkeypatch.setattr(file_io_module, "os", WindowsOs()) + + assert not file_io_module._component_is_too_long(short_path) + assert file_io_module._component_is_too_long(long_path) + + @pytest.mark.skipif(os.name != "nt", reason="Windows path semantics") def test_fall_back_for_near_max_path_destination_on_windows(tmp_path: Path): parent = tmp_path @@ -783,6 +859,45 @@ def change_directory(temporary_path: Path) -> None: assert not (second_directory / "data.txt").exists() +@pytest.mark.skipif( + os.name == "nt" or not all(hasattr(os, name) for name in ("fork", "openpty", "setsid")), + reason="POSIX controlling-terminal semantics", +) +def test_reading_terminal_does_not_acquire_controlling_terminal(): + master_descriptor, slave_descriptor = os.openpty() + slave_path = os.ttyname(slave_descriptor) + child = os.fork() + if child == 0: + os.close(master_descriptor) + os.close(slave_descriptor) + try: + os.setsid() + except OSError: + os._exit(2) + try: + read_text_file(slave_path) + except OSError as error: + if "not a regular file" not in str(error): + os._exit(4) + else: + os._exit(3) + try: + terminal_descriptor = os.open( + "/dev/tty", + os.O_RDONLY | getattr(os, "O_NOCTTY", 0), + ) + except OSError as error: + os._exit(0 if error.errno == errno.ENXIO else 5) + os.close(terminal_descriptor) + os._exit(1) + + os.close(slave_descriptor) + _, status = os.waitpid(child, 0) + os.close(master_descriptor) + + assert os.waitstatus_to_exitcode(status) == 0 + + @pytest.mark.skipif(os.name == "nt", reason="POSIX symlink traversal semantics") def test_preserve_symlink_parent_traversal_when_freezing_path(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): working_directory = tmp_path / "working" From 21ccef1aedec011fcd5a3bb864edf3c869b93fc0 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:31:09 +0000 Subject: [PATCH 14/23] Make reservation regression platform-neutral Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/tests/test_utils_file_io.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 0ed4c86c2..48c463a69 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -251,28 +251,37 @@ def test_clean_up_after_initial_reservation_fstat_failure( ): destination = tmp_path / "data.txt" fstat = file_io_module.os.fstat - fstat_calls = 0 + create_exclusive_file = file_io_module._create_exclusive_file + reservation_descriptor: int | None = None + reservation_fstat_calls = 0 closed_descriptors: list[int] = [] close_descriptor = file_io_module._close_descriptor + def create_and_track_reservation(temporary_path: Path) -> int: + nonlocal reservation_descriptor + reservation_descriptor = create_exclusive_file(temporary_path) + return reservation_descriptor + def fail_initial_reservation_fstat(descriptor: int) -> os.stat_result: - nonlocal fstat_calls - fstat_calls += 1 - if fstat_calls == 2: - raise OSError("identity snapshot failed") + nonlocal reservation_fstat_calls + if descriptor == reservation_descriptor: + reservation_fstat_calls += 1 + if reservation_fstat_calls == 1: + raise OSError("identity snapshot failed") return fstat(descriptor) def record_close(descriptor: int) -> OSError | None: closed_descriptors.append(descriptor) return close_descriptor(descriptor) + monkeypatch.setattr(file_io_module, "_create_exclusive_file", create_and_track_reservation) monkeypatch.setattr(file_io_module.os, "fstat", fail_initial_reservation_fstat) monkeypatch.setattr(file_io_module, "_close_descriptor", record_close) with pytest.raises(OSError, match="identity snapshot failed"): write_text_file_atomically(destination, "contents") - assert fstat_calls == 3 + assert reservation_fstat_calls == 2 assert len(closed_descriptors) == 1 assert list(tmp_path.iterdir()) == [] From 42ce8acb28b82eaba87a63d0b33acd3309f32877 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:31:53 +0000 Subject: [PATCH 15/23] Handle concurrent private directory creation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 186 ++++++++++++++-------- python/src/qdk_chemistry/utils/file_io.py | 33 +++- python/tests/test_utils_file_io.py | 77 +++++++++ 3 files changed, 231 insertions(+), 65 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 2433c716c..dfcd6cdfa 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #ifdef _WIN32 @@ -79,6 +80,24 @@ std::string display_path(const std::filesystem::path& path) { return {value.begin(), value.end()}; } +#ifndef _WIN32 +class DirectoryPermissionError : public std::runtime_error { + public: + using std::runtime_error::runtime_error; +}; + +[[noreturn]] void throw_directory_error(const std::string& action, + const std::filesystem::path& path, + const std::error_code& error) { + const auto message = + action + " '" + display_path(path) + "': " + error.message(); + if (error == std::errc::permission_denied) { + throw DirectoryPermissionError(message); + } + throw std::runtime_error(message); +} +#endif + void validate_path(const std::filesystem::path& path) { const auto& native_path = path.native(); if (native_path.find(static_cast('\0')) != @@ -724,76 +743,115 @@ void create_private_directories(const std::filesystem::path& directory) { display_path(directory) + "': " + error.message()); } #else - std::vector missing; - auto current = directory; - std::error_code status_error; - while (!current.empty() && - !std::filesystem::is_directory(current, status_error)) { - if (status_error && status_error != std::errc::no_such_file_or_directory) { - throw std::runtime_error("Could not inspect directory '" + - display_path(current) + - "': " + status_error.message()); - } - status_error.clear(); - missing.push_back(current); - const auto parent = current.parent_path(); - if (parent == current) { - break; + constexpr auto retry_delay = std::chrono::milliseconds(1); + constexpr auto retry_timeout = std::chrono::seconds(1); + const auto deadline = std::chrono::steady_clock::now() + retry_timeout; + + auto create_once = [&]() { + std::vector missing; + auto current = directory; + std::error_code status_error; + while (!current.empty() && + !std::filesystem::is_directory(current, status_error)) { + if (status_error && + status_error != std::errc::no_such_file_or_directory) { + throw_directory_error("Could not inspect directory", current, + status_error); + } + status_error.clear(); + missing.push_back(current); + const auto parent = current.parent_path(); + if (parent == current) { + break; + } + current = parent; } - current = parent; - } - for (auto iterator = missing.rbegin(); iterator != missing.rend(); - ++iterator) { - if (retry_on_eintr([&] { return ::mkdir(iterator->c_str(), S_IRWXU); }) != - 0) { - const int mkdir_error = errno; - if (mkdir_error == EEXIST && std::filesystem::is_directory(*iterator)) { - continue; + for (auto iterator = missing.rbegin(); iterator != missing.rend(); + ++iterator) { + if (retry_on_eintr([&] { return ::mkdir(iterator->c_str(), S_IRWXU); }) != + 0) { + const int mkdir_error = errno; + if (mkdir_error == EEXIST) { + std::error_code existing_error; + if (std::filesystem::is_directory(*iterator, existing_error) && + !existing_error) { + continue; + } + if (existing_error) { + throw_directory_error("Could not inspect directory", *iterator, + existing_error); + } + } + throw_directory_error( + "Could not create directory", *iterator, + std::error_code(mkdir_error, std::generic_category())); } - const std::error_code error(mkdir_error, std::generic_category()); - throw std::runtime_error("Could not create directory '" + - display_path(*iterator) + - "': " + error.message()); - } - if (retry_on_eintr([&] { return ::chmod(iterator->c_str(), S_IRWXU); }) != - 0) { - const std::error_code error(errno, std::generic_category()); - std::error_code ignored; - std::filesystem::remove(*iterator, ignored); - throw std::runtime_error("Could not secure directory '" + - display_path(*iterator) + - "': " + error.message()); - } - const int descriptor = retry_on_eintr([&] { - return ::open(iterator->c_str(), - O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); - }); - struct stat status{}; - int permission_error = 0; - if (descriptor == -1) { - permission_error = errno; - } else if (retry_on_eintr([&] { return ::fchmod(descriptor, S_IRWXU); }) != - 0) { - permission_error = errno; - } else if (retry_on_eintr([&] { return ::fstat(descriptor, &status); }) != - 0) { - permission_error = errno; - } else if ((status.st_mode & 0777) != S_IRWXU) { - permission_error = EPERM; + if (retry_on_eintr([&] { return ::chmod(iterator->c_str(), S_IRWXU); }) != + 0) { + const std::error_code error(errno, std::generic_category()); + std::error_code ignored; + std::filesystem::remove(*iterator, ignored); + throw_directory_error("Could not secure directory", *iterator, error); + } + const int descriptor = retry_on_eintr([&] { + return ::open(iterator->c_str(), + O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + }); + struct stat status{}; + int permission_error = 0; + if (descriptor == -1) { + permission_error = errno; + } else if (retry_on_eintr( + [&] { return ::fchmod(descriptor, S_IRWXU); }) != 0) { + permission_error = errno; + } else if (retry_on_eintr([&] { return ::fstat(descriptor, &status); }) != + 0) { + permission_error = errno; + } else if ((status.st_mode & 0777) != S_IRWXU) { + permission_error = EPERM; + } + if (permission_error != 0) { + const std::error_code error(permission_error, std::generic_category()); + if (descriptor != -1) { + ::close(descriptor); + } + std::error_code ignored; + std::filesystem::remove(*iterator, ignored); + throw_directory_error("Could not secure directory", *iterator, error); + } + ::close(descriptor); } - if (permission_error != 0) { - const std::error_code error(permission_error, std::generic_category()); - if (descriptor != -1) { - ::close(descriptor); + }; + + while (true) { + try { + create_once(); + int access_flags = 0; +#ifdef AT_EACCESS + access_flags = AT_EACCESS; +#endif + if (retry_on_eintr([&] { + return ::faccessat(AT_FDCWD, directory.c_str(), W_OK | X_OK, + access_flags); + }) != 0) { + const int access_error = errno; + if (access_error == EACCES || access_error == ENOENT) { + throw DirectoryPermissionError( + "Directory is not ready for writing: '" + + display_path(directory) + "'"); + } + throw_directory_error( + "Could not inspect directory", directory, + std::error_code(access_error, std::generic_category())); } - std::error_code ignored; - std::filesystem::remove(*iterator, ignored); - throw std::runtime_error("Could not secure directory '" + - display_path(*iterator) + - "': " + error.message()); + return; + } catch (const DirectoryPermissionError&) { + if (std::chrono::steady_clock::now() >= deadline) { + throw; + } + std::this_thread::sleep_for(retry_delay); } - ::close(descriptor); } #endif } diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index caa725c14..920406e8d 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -14,6 +14,7 @@ import os import stat import sys +import time from collections.abc import Callable from ctypes import wintypes from pathlib import Path @@ -22,6 +23,9 @@ PathLike: TypeAlias = str | os.PathLike[str] AtomicFileWriter: TypeAlias = Callable[[Path], None] +_DIRECTORY_CREATION_RETRY_DELAY_SECONDS = 0.001 +_DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS = 1.0 + __all__ = [ "AtomicFileWriter", "PathLike", @@ -712,7 +716,7 @@ def _set_permissions(descriptor: int, mode: int, path: Path) -> None: ) -def _create_private_directories(directory: Path) -> None: +def _create_private_directories_once(directory: Path) -> None: missing: list[Path] = [] current = directory while not current.is_dir(): @@ -756,6 +760,33 @@ def _create_private_directories(directory: Path) -> None: raise close_error +def _directory_may_be_initializing(directory: Path) -> bool: + access_options = {"effective_ids": True} if os.access in os.supports_effective_ids else {} + return not os.access(directory, os.W_OK | os.X_OK, **access_options) + + +def _create_private_directories(directory: Path) -> None: + deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS + while True: + permission_error: PermissionError | None = None + try: + _create_private_directories_once(directory) + if not _directory_may_be_initializing(directory): + return + permission_error = PermissionError( + errno.EACCES, + f"Directory is not ready for writing: '{directory}'", + os.fspath(directory), + ) + except PermissionError as error: + permission_error = error + + if time.monotonic() >= deadline: + assert permission_error is not None + raise permission_error + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + + def write_text_file_atomically( path: PathLike, contents: str, diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 48c463a69..7477a2bad 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -107,6 +107,83 @@ def test_create_private_parent_directories_under_restrictive_umask(tmp_path: Pat assert read_text_file(path) == "contents" +@pytest.mark.skipif(os.name == "nt", reason="POSIX umask semantics") +def test_serialize_concurrent_parent_creation_under_restrictive_umask( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + shared_parent = tmp_path / "shared" + first_path = shared_parent / "first" / "data.txt" + second_path = shared_parent / "second" / "data.txt" + mkdir = file_io_module.os.mkdir + parent_created = threading.Event() + release_creator = threading.Event() + second_finished = threading.Event() + paused = False + errors: list[Exception] = [] + + def pause_after_creating_parent(path: Path, mode: int) -> None: + nonlocal paused + mkdir(path, mode) + if Path(path) == shared_parent and not paused: + paused = True + parent_created.set() + if not release_creator.wait(timeout=10): + raise RuntimeError("timed out waiting to release directory creator") + + def write(path: Path, finished: threading.Event | None = None) -> None: + try: + write_text_file_atomically(path, "contents", create_parent_directories=True) + except (OSError, RuntimeError, ValueError) as error: + errors.append(error) + finally: + if finished is not None: + finished.set() + + monkeypatch.setattr(file_io_module.os, "mkdir", pause_after_creating_parent) + original_umask = os.umask(0o777) + first = threading.Thread(target=write, args=(first_path,)) + second = threading.Thread(target=write, args=(second_path, second_finished)) + try: + first.start() + assert parent_created.wait(timeout=10) + second.start() + assert not second_finished.wait(timeout=0.05) + release_creator.set() + first.join(timeout=10) + second.join(timeout=10) + finally: + release_creator.set() + first.join(timeout=10) + second.join(timeout=10) + os.umask(original_umask) + + assert not first.is_alive() + assert not second.is_alive() + assert errors == [] + assert read_text_file(first_path) == "contents" + assert read_text_file(second_path) == "contents" + assert stat.S_IMODE(shared_parent.stat().st_mode) == 0o700 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_do_not_modify_permanently_inaccessible_parent( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + parent = tmp_path / "inaccessible" + parent.mkdir() + parent.chmod(0) + monkeypatch.setattr(file_io_module, "_DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS", 0) + + try: + with pytest.raises(PermissionError): + ensure_parent_directory(parent / "nested" / "data.txt") + assert stat.S_IMODE(parent.stat().st_mode) == 0 + finally: + parent.chmod(0o700) + + def test_reject_missing_parent_directory_by_default(tmp_path: Path): path = tmp_path / "missing" / "data.txt" From 4b879cef184fa5db813b25c22c96f2daa55a48ff Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:06:09 +0000 Subject: [PATCH 16/23] Scope parent creation retries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 83 +++++++++++++++-------- python/src/qdk_chemistry/utils/file_io.py | 44 +++++++++--- python/tests/test_utils_file_io.py | 17 ++++- 3 files changed, 102 insertions(+), 42 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index dfcd6cdfa..8d572a04f 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -81,21 +81,46 @@ std::string display_path(const std::filesystem::path& path) { } #ifndef _WIN32 -class DirectoryPermissionError : public std::runtime_error { +class TransientPermissionError : public std::runtime_error { public: using std::runtime_error::runtime_error; }; +#endif [[noreturn]] void throw_directory_error(const std::string& action, const std::filesystem::path& path, const std::error_code& error) { const auto message = action + " '" + display_path(path) + "': " + error.message(); +#ifndef _WIN32 if (error == std::errc::permission_denied) { - throw DirectoryPermissionError(message); + throw TransientPermissionError(message); } +#endif throw std::runtime_error(message); } + +#ifndef _WIN32 +bool has_initializing_directory(const std::filesystem::path& directory) { + auto current = directory; + while (!current.empty()) { + struct stat status{}; + if (retry_on_eintr([&] { return ::stat(current.c_str(), &status); }) == 0) { + if (S_ISDIR(status.st_mode) && status.st_uid == geteuid() && + (status.st_mode & 0777) == 0) { + return true; + } + } else if (errno != EACCES && errno != ENOENT) { + return false; + } + const auto parent = current.parent_path(); + if (parent == current) { + return false; + } + current = parent; + } + return false; +} #endif void validate_path(const std::filesystem::path& path) { @@ -508,9 +533,8 @@ ReservedTemporaryFile reserve_temporary_file( if (is_name_too_long(error, destination)) { break; } - throw std::runtime_error("Could not create temporary file beside '" + - display_path(destination) + - "': " + error.message()); + throw_directory_error("Could not create temporary file beside", + destination, error); } } @@ -540,9 +564,8 @@ ReservedTemporaryFile reserve_temporary_file( if (is_name_too_long(error, destination)) { break; } - throw std::runtime_error("Could not create temporary file beside '" + - display_path(destination) + - "': " + error.message()); + throw_directory_error("Could not create temporary file beside", + destination, error); } } @@ -827,27 +850,10 @@ void create_private_directories(const std::filesystem::path& directory) { while (true) { try { create_once(); - int access_flags = 0; -#ifdef AT_EACCESS - access_flags = AT_EACCESS; -#endif - if (retry_on_eintr([&] { - return ::faccessat(AT_FDCWD, directory.c_str(), W_OK | X_OK, - access_flags); - }) != 0) { - const int access_error = errno; - if (access_error == EACCES || access_error == ENOENT) { - throw DirectoryPermissionError( - "Directory is not ready for writing: '" + - display_path(directory) + "'"); - } - throw_directory_error( - "Could not inspect directory", directory, - std::error_code(access_error, std::generic_category())); - } return; - } catch (const DirectoryPermissionError&) { - if (std::chrono::steady_clock::now() >= deadline) { + } catch (const TransientPermissionError&) { + if (!has_initializing_directory(directory) || + std::chrono::steady_clock::now() >= deadline) { throw; } std::this_thread::sleep_for(retry_delay); @@ -967,7 +973,26 @@ void write_file_atomically(const std::filesystem::path& path, } } - auto temporary_file = reserve_temporary_file(destination); + auto temporary_file = [&]() { +#ifdef _WIN32 + return reserve_temporary_file(destination); +#else + const auto deadline = + std::chrono::steady_clock::now() + std::chrono::seconds(1); + while (true) { + try { + return reserve_temporary_file(destination); + } catch (const TransientPermissionError&) { + if (!create_parent_directories || + !has_initializing_directory(destination.parent_path()) || + std::chrono::steady_clock::now() >= deadline) { + throw; + } + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + } +#endif + }(); writer(temporary_file.path()); temporary_file.verify_identity(); preserve_permissions(temporary_file, destination); diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 920406e8d..c8e9dc3dd 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -200,7 +200,11 @@ def write_file_atomically( reservation: _TemporaryFileReservation | None = None try: - reservation = _reserve_temporary_file(destination) + reservation = ( + _reserve_temporary_file_with_parent_retry(destination) + if create_parent_directories and os.name != "nt" + else _reserve_temporary_file(destination) + ) descriptor = reservation.descriptor temporary_path = reservation.path reserved_status = reservation.status @@ -760,9 +764,20 @@ def _create_private_directories_once(directory: Path) -> None: raise close_error -def _directory_may_be_initializing(directory: Path) -> bool: - access_options = {"effective_ids": True} if os.access in os.supports_effective_ids else {} - return not os.access(directory, os.W_OK | os.X_OK, **access_options) +def _has_initializing_directory(directory: Path) -> bool: + current = directory + while True: + try: + status = current.stat() + except (FileNotFoundError, PermissionError): + pass + else: + if stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and stat.S_IMODE(status.st_mode) == 0: + return True + parent = current.parent + if parent == current: + return False + current = parent def _create_private_directories(directory: Path) -> None: @@ -771,14 +786,10 @@ def _create_private_directories(directory: Path) -> None: permission_error: PermissionError | None = None try: _create_private_directories_once(directory) - if not _directory_may_be_initializing(directory): - return - permission_error = PermissionError( - errno.EACCES, - f"Directory is not ready for writing: '{directory}'", - os.fspath(directory), - ) + return except PermissionError as error: + if not _has_initializing_directory(directory): + raise permission_error = error if time.monotonic() >= deadline: @@ -787,6 +798,17 @@ def _create_private_directories(directory: Path) -> None: time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) +def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFileReservation: + deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS + while True: + try: + return _reserve_temporary_file(destination) + except PermissionError: + if not _has_initializing_directory(destination.parent) or time.monotonic() >= deadline: + raise + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + + def write_text_file_atomically( path: PathLike, contents: str, diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 7477a2bad..a352c5226 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -113,8 +113,8 @@ def test_serialize_concurrent_parent_creation_under_restrictive_umask( monkeypatch: pytest.MonkeyPatch, ): shared_parent = tmp_path / "shared" - first_path = shared_parent / "first" / "data.txt" - second_path = shared_parent / "second" / "data.txt" + first_path = shared_parent / "first.txt" + second_path = shared_parent / "second.txt" mkdir = file_io_module.os.mkdir parent_created = threading.Event() release_creator = threading.Event() @@ -184,6 +184,19 @@ def test_do_not_modify_permanently_inaccessible_parent( parent.chmod(0o700) +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_existing_parent_is_a_noop_without_write_permission(tmp_path: Path): + parent = tmp_path / "existing" + parent.mkdir() + parent.chmod(0o500) + + try: + ensure_parent_directory(parent / "data.txt") + assert stat.S_IMODE(parent.stat().st_mode) == 0o500 + finally: + parent.chmod(0o700) + + def test_reject_missing_parent_directory_by_default(tmp_path: Path): path = tmp_path / "missing" / "data.txt" From 3c60a3979515882d98c44d47f5f08f2413eca2a6 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:41:37 +0000 Subject: [PATCH 17/23] Complete parent creation retry handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 28 ++++++-- python/src/qdk_chemistry/utils/file_io.py | 20 +++++- python/tests/test_utils_file_io.py | 81 ++++++++++++++++++++++- 3 files changed, 118 insertions(+), 11 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 8d572a04f..b5e97f34d 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -847,13 +847,21 @@ void create_private_directories(const std::filesystem::path& directory) { } }; + bool retry_without_marker = true; while (true) { try { create_once(); return; } catch (const TransientPermissionError&) { - if (!has_initializing_directory(directory) || - std::chrono::steady_clock::now() >= deadline) { + if (has_initializing_directory(directory)) { + retry_without_marker = true; + } else if (retry_without_marker) { + retry_without_marker = false; + continue; + } else { + throw; + } + if (std::chrono::steady_clock::now() >= deadline) { throw; } std::this_thread::sleep_for(retry_delay); @@ -979,13 +987,23 @@ void write_file_atomically(const std::filesystem::path& path, #else const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(1); + bool retry_without_marker = true; while (true) { try { return reserve_temporary_file(destination); } catch (const TransientPermissionError&) { - if (!create_parent_directories || - !has_initializing_directory(destination.parent_path()) || - std::chrono::steady_clock::now() >= deadline) { + if (!create_parent_directories) { + throw; + } + if (has_initializing_directory(destination.parent_path())) { + retry_without_marker = true; + } else if (retry_without_marker) { + retry_without_marker = false; + continue; + } else { + throw; + } + if (std::chrono::steady_clock::now() >= deadline) { throw; } std::this_thread::sleep_for(std::chrono::milliseconds(1)); diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index c8e9dc3dd..7ab947284 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -772,7 +772,7 @@ def _has_initializing_directory(directory: Path) -> bool: except (FileNotFoundError, PermissionError): pass else: - if stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and stat.S_IMODE(status.st_mode) == 0: + if stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and status.st_mode & 0o777 == 0: return True parent = current.parent if parent == current: @@ -782,13 +782,19 @@ def _has_initializing_directory(directory: Path) -> bool: def _create_private_directories(directory: Path) -> None: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS + retry_without_marker = True while True: permission_error: PermissionError | None = None try: _create_private_directories_once(directory) return except PermissionError as error: - if not _has_initializing_directory(directory): + if _has_initializing_directory(directory): + retry_without_marker = True + elif retry_without_marker: + retry_without_marker = False + continue + else: raise permission_error = error @@ -800,11 +806,19 @@ def _create_private_directories(directory: Path) -> None: def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFileReservation: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS + retry_without_marker = True while True: try: return _reserve_temporary_file(destination) except PermissionError: - if not _has_initializing_directory(destination.parent) or time.monotonic() >= deadline: + if _has_initializing_directory(destination.parent): + retry_without_marker = True + elif retry_without_marker: + retry_without_marker = False + continue + else: + raise + if time.monotonic() >= deadline: raise time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index a352c5226..b8fbf8046 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -107,14 +107,28 @@ def test_create_private_parent_directories_under_restrictive_umask(tmp_path: Pat assert read_text_file(path) == "contents" +@pytest.mark.parametrize("use_setgid_parent", [False, True]) +@pytest.mark.parametrize("use_descendant_parent", [False, True]) @pytest.mark.skipif(os.name == "nt", reason="POSIX umask semantics") def test_serialize_concurrent_parent_creation_under_restrictive_umask( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, + use_setgid_parent: bool, + use_descendant_parent: bool, ): - shared_parent = tmp_path / "shared" - first_path = shared_parent / "first.txt" - second_path = shared_parent / "second.txt" + root = tmp_path / "root" + root.mkdir() + if use_setgid_parent: + if sys.platform != "linux": + pytest.skip("setgid directory inheritance is verified on Linux") + root.chmod(0o2700) + shared_parent = root / "shared" + if use_descendant_parent: + first_path = shared_parent / "first" / "data.txt" + second_path = shared_parent / "second" / "data.txt" + else: + first_path = shared_parent / "first.txt" + second_path = shared_parent / "second.txt" mkdir = file_io_module.os.mkdir parent_created = threading.Event() release_creator = threading.Event() @@ -166,6 +180,67 @@ def write(path: Path, finished: threading.Event | None = None) -> None: assert stat.S_IMODE(shared_parent.stat().st_mode) == 0o700 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_after_directory_initialization_completes( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + directory = tmp_path / "shared" + directory.mkdir() + directory.chmod(0) + create_private_directories_once = file_io_module._create_private_directories_once + calls = 0 + + def fail_with_stale_permission_error(path: Path) -> None: + nonlocal calls + calls += 1 + if calls == 1: + directory.chmod(0o700) + raise PermissionError(errno.EACCES, "initializing", os.fspath(path)) + create_private_directories_once(path) + + monkeypatch.setattr( + file_io_module, + "_create_private_directories_once", + fail_with_stale_permission_error, + ) + + file_io_module._create_private_directories(directory) + + assert calls == 2 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_reservation_after_directory_initialization_completes( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + directory = tmp_path / "shared" + directory.mkdir() + directory.chmod(0) + destination = directory / "data.txt" + reservation = object() + calls = 0 + + def fail_with_stale_permission_error(path: Path) -> object: + nonlocal calls + calls += 1 + if calls == 1: + directory.chmod(0o700) + raise PermissionError(errno.EACCES, "initializing", os.fspath(path)) + assert path == destination + return reservation + + monkeypatch.setattr( + file_io_module, + "_reserve_temporary_file", + fail_with_stale_permission_error, + ) + + assert file_io_module._reserve_temporary_file_with_parent_retry(destination) is reservation + assert calls == 2 + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") def test_do_not_modify_permanently_inaccessible_parent( tmp_path: Path, From 474b78aa228b1ccfd929d1c0b1e65d518efcdc3a Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:16:50 +0000 Subject: [PATCH 18/23] Track parent initialization progress Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 73 ++++++++++++++--------- python/src/qdk_chemistry/utils/file_io.py | 61 +++++++++++-------- python/tests/test_utils_file_io.py | 73 +++++++++++++++++++++++ 3 files changed, 155 insertions(+), 52 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index b5e97f34d..89527da31 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #ifdef _WIN32 @@ -101,25 +102,33 @@ class TransientPermissionError : public std::runtime_error { } #ifndef _WIN32 -bool has_initializing_directory(const std::filesystem::path& directory) { +using DirectoryStateEntry = std::tuple; + +std::pair> +directory_initialization_state(const std::filesystem::path& directory) { + bool initializing = false; + std::vector state; auto current = directory; while (!current.empty()) { struct stat status{}; if (retry_on_eintr([&] { return ::stat(current.c_str(), &status); }) == 0) { + const mode_t permissions = status.st_mode & 0777; + state.emplace_back(status.st_dev, status.st_ino, permissions, 0); if (S_ISDIR(status.st_mode) && status.st_uid == geteuid() && - (status.st_mode & 0777) == 0) { - return true; + permissions == 0) { + initializing = true; } - } else if (errno != EACCES && errno != ENOENT) { - return false; + } else { + const int status_error = errno; + state.emplace_back(0, 0, 0, status_error); } const auto parent = current.parent_path(); if (parent == current) { - return false; + return {initializing, std::move(state)}; } current = parent; } - return false; + return {initializing, std::move(state)}; } #endif @@ -847,24 +856,29 @@ void create_private_directories(const std::filesystem::path& directory) { } }; - bool retry_without_marker = true; + auto [ignored_initializing, previous_state] = + directory_initialization_state(directory); + static_cast(ignored_initializing); while (true) { try { create_once(); return; } catch (const TransientPermissionError&) { - if (has_initializing_directory(directory)) { - retry_without_marker = true; - } else if (retry_without_marker) { - retry_without_marker = false; - continue; - } else { - throw; - } if (std::chrono::steady_clock::now() >= deadline) { throw; } - std::this_thread::sleep_for(retry_delay); + auto [initializing, current_state] = + directory_initialization_state(directory); + if (initializing) { + previous_state = std::move(current_state); + std::this_thread::sleep_for(retry_delay); + continue; + } + if (current_state != previous_state) { + previous_state = std::move(current_state); + continue; + } + throw; } } #endif @@ -987,7 +1001,9 @@ void write_file_atomically(const std::filesystem::path& path, #else const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(1); - bool retry_without_marker = true; + auto [ignored_initializing, previous_state] = + directory_initialization_state(destination.parent_path()); + static_cast(ignored_initializing); while (true) { try { return reserve_temporary_file(destination); @@ -995,18 +1011,21 @@ void write_file_atomically(const std::filesystem::path& path, if (!create_parent_directories) { throw; } - if (has_initializing_directory(destination.parent_path())) { - retry_without_marker = true; - } else if (retry_without_marker) { - retry_without_marker = false; - continue; - } else { - throw; - } if (std::chrono::steady_clock::now() >= deadline) { throw; } - std::this_thread::sleep_for(std::chrono::milliseconds(1)); + auto [initializing, current_state] = + directory_initialization_state(destination.parent_path()); + if (initializing) { + previous_state = std::move(current_state); + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + continue; + } + if (current_state != previous_state) { + previous_state = std::move(current_state); + continue; + } + throw; } } #endif diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 7ab947284..645be7e8a 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -764,63 +764,74 @@ def _create_private_directories_once(directory: Path) -> None: raise close_error -def _has_initializing_directory(directory: Path) -> bool: +def _directory_initialization_state( + directory: Path, +) -> tuple[bool, tuple[tuple[str, int, int, int], ...]]: + initializing = False + state: list[tuple[str, int, int, int]] = [] current = directory while True: try: status = current.stat() - except (FileNotFoundError, PermissionError): - pass + except FileNotFoundError: + state.append((os.fspath(current), -1, -1, errno.ENOENT)) + except PermissionError: + state.append((os.fspath(current), -1, -1, errno.EACCES)) else: - if stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and status.st_mode & 0o777 == 0: - return True + permissions = status.st_mode & 0o777 + state.append((os.fspath(current), status.st_dev, status.st_ino, permissions)) + initializing = initializing or ( + stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and permissions == 0 + ) parent = current.parent if parent == current: - return False + return initializing, tuple(state) current = parent def _create_private_directories(directory: Path) -> None: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS - retry_without_marker = True + _, previous_state = _directory_initialization_state(directory) while True: permission_error: PermissionError | None = None try: _create_private_directories_once(directory) return except PermissionError as error: - if _has_initializing_directory(directory): - retry_without_marker = True - elif retry_without_marker: - retry_without_marker = False - continue - else: - raise permission_error = error if time.monotonic() >= deadline: assert permission_error is not None raise permission_error - time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + initializing, current_state = _directory_initialization_state(directory) + if initializing: + previous_state = current_state + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + continue + if current_state != previous_state: + previous_state = current_state + continue + raise permission_error def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFileReservation: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS - retry_without_marker = True + _, previous_state = _directory_initialization_state(destination.parent) while True: try: return _reserve_temporary_file(destination) - except PermissionError: - if _has_initializing_directory(destination.parent): - retry_without_marker = True - elif retry_without_marker: - retry_without_marker = False - continue - else: - raise + except PermissionError as error: if time.monotonic() >= deadline: raise - time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + initializing, current_state = _directory_initialization_state(destination.parent) + if initializing: + previous_state = current_state + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) + continue + if current_state != previous_state: + previous_state = current_state + continue + raise error def write_text_file_atomically( diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index b8fbf8046..0df3109c2 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -241,6 +241,79 @@ def fail_with_stale_permission_error(path: Path) -> object: assert calls == 2 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_after_multiple_directory_state_changes( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + directory = tmp_path / "nested" / "parent" + states = iter( + [ + (False, (("root", 1, 1, 0o700),)), + (False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), + ( + False, + ( + ("root", 1, 1, 0o700), + ("nested", 1, 2, 0o700), + ("parent", 1, 3, 0o700), + ), + ), + ] + ) + calls = 0 + + def fail_twice(_: Path) -> None: + nonlocal calls + calls += 1 + if calls < 3: + raise PermissionError(errno.EACCES, "initializing") + + monkeypatch.setattr(file_io_module, "_directory_initialization_state", lambda _: next(states)) + monkeypatch.setattr(file_io_module, "_create_private_directories_once", fail_twice) + + file_io_module._create_private_directories(directory) + + assert calls == 3 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_reservation_after_multiple_directory_state_changes( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + destination = tmp_path / "nested" / "parent" / "data.txt" + reservation = object() + states = iter( + [ + (False, (("root", 1, 1, 0o700),)), + (False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), + ( + False, + ( + ("root", 1, 1, 0o700), + ("nested", 1, 2, 0o700), + ("parent", 1, 3, 0o700), + ), + ), + ] + ) + calls = 0 + + def fail_twice(_: Path) -> object: + nonlocal calls + calls += 1 + if calls < 3: + raise PermissionError(errno.EACCES, "initializing") + return reservation + + monkeypatch.setattr(file_io_module, "_directory_initialization_state", lambda _: next(states)) + monkeypatch.setattr(file_io_module, "_reserve_temporary_file", fail_twice) + + assert file_io_module._reserve_temporary_file_with_parent_retry(destination) is reservation + assert calls == 3 + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") def test_do_not_modify_permanently_inaccessible_parent( tmp_path: Path, From 6eca8d35f551b86cf95889b4e57c708890dba290 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:53:17 +0000 Subject: [PATCH 19/23] Make parent retries progress-aware Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 31 ++++++++++++----------- python/src/qdk_chemistry/utils/file_io.py | 22 ++++++++-------- python/tests/test_utils_file_io.py | 16 ++++++++++++ 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 89527da31..c7c042abd 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -114,8 +114,9 @@ directory_initialization_state(const std::filesystem::path& directory) { if (retry_on_eintr([&] { return ::stat(current.c_str(), &status); }) == 0) { const mode_t permissions = status.st_mode & 0777; state.emplace_back(status.st_dev, status.st_ino, permissions, 0); + constexpr mode_t required_permissions = S_IWUSR | S_IXUSR; if (S_ISDIR(status.st_mode) && status.st_uid == geteuid() && - permissions == 0) { + (permissions & required_permissions) != required_permissions) { initializing = true; } } else { @@ -776,7 +777,7 @@ void create_private_directories(const std::filesystem::path& directory) { } #else constexpr auto retry_delay = std::chrono::milliseconds(1); - constexpr auto retry_timeout = std::chrono::seconds(1); + constexpr auto retry_timeout = std::chrono::milliseconds(100); const auto deadline = std::chrono::steady_clock::now() + retry_timeout; auto create_once = [&]() { @@ -856,9 +857,8 @@ void create_private_directories(const std::filesystem::path& directory) { } }; - auto [ignored_initializing, previous_state] = - directory_initialization_state(directory); - static_cast(ignored_initializing); + std::vector previous_state; + bool has_previous_state = false; while (true) { try { create_once(); @@ -869,13 +869,14 @@ void create_private_directories(const std::filesystem::path& directory) { } auto [initializing, current_state] = directory_initialization_state(directory); - if (initializing) { + if (!has_previous_state || current_state != previous_state) { previous_state = std::move(current_state); + has_previous_state = true; std::this_thread::sleep_for(retry_delay); continue; } - if (current_state != previous_state) { - previous_state = std::move(current_state); + if (initializing) { + std::this_thread::sleep_for(retry_delay); continue; } throw; @@ -1000,10 +1001,9 @@ void write_file_atomically(const std::filesystem::path& path, return reserve_temporary_file(destination); #else const auto deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(1); - auto [ignored_initializing, previous_state] = - directory_initialization_state(destination.parent_path()); - static_cast(ignored_initializing); + std::chrono::steady_clock::now() + std::chrono::milliseconds(100); + std::vector previous_state; + bool has_previous_state = false; while (true) { try { return reserve_temporary_file(destination); @@ -1016,13 +1016,14 @@ void write_file_atomically(const std::filesystem::path& path, } auto [initializing, current_state] = directory_initialization_state(destination.parent_path()); - if (initializing) { + if (!has_previous_state || current_state != previous_state) { previous_state = std::move(current_state); + has_previous_state = true; std::this_thread::sleep_for(std::chrono::milliseconds(1)); continue; } - if (current_state != previous_state) { - previous_state = std::move(current_state); + if (initializing) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); continue; } throw; diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 645be7e8a..59a234121 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -24,7 +24,7 @@ AtomicFileWriter: TypeAlias = Callable[[Path], None] _DIRECTORY_CREATION_RETRY_DELAY_SECONDS = 0.001 -_DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS = 1.0 +_DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS = 0.1 __all__ = [ "AtomicFileWriter", @@ -781,7 +781,9 @@ def _directory_initialization_state( permissions = status.st_mode & 0o777 state.append((os.fspath(current), status.st_dev, status.st_ino, permissions)) initializing = initializing or ( - stat.S_ISDIR(status.st_mode) and status.st_uid == os.geteuid() and permissions == 0 + stat.S_ISDIR(status.st_mode) + and status.st_uid == os.geteuid() + and permissions & (stat.S_IWUSR | stat.S_IXUSR) != (stat.S_IWUSR | stat.S_IXUSR) ) parent = current.parent if parent == current: @@ -791,7 +793,7 @@ def _directory_initialization_state( def _create_private_directories(directory: Path) -> None: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS - _, previous_state = _directory_initialization_state(directory) + previous_state: tuple[tuple[str, int, int, int], ...] | None = None while True: permission_error: PermissionError | None = None try: @@ -804,19 +806,19 @@ def _create_private_directories(directory: Path) -> None: assert permission_error is not None raise permission_error initializing, current_state = _directory_initialization_state(directory) - if initializing: + if previous_state is None or current_state != previous_state: previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue - if current_state != previous_state: - previous_state = current_state + if initializing: + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue raise permission_error def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFileReservation: deadline = time.monotonic() + _DIRECTORY_CREATION_RETRY_TIMEOUT_SECONDS - _, previous_state = _directory_initialization_state(destination.parent) + previous_state: tuple[tuple[str, int, int, int], ...] | None = None while True: try: return _reserve_temporary_file(destination) @@ -824,12 +826,12 @@ def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFi if time.monotonic() >= deadline: raise initializing, current_state = _directory_initialization_state(destination.parent) - if initializing: + if previous_state is None or current_state != previous_state: previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue - if current_state != previous_state: - previous_state = current_state + if initializing: + time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue raise error diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index 0df3109c2..ca4436944 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -314,6 +314,22 @@ def fail_twice(_: Path) -> object: assert calls == 3 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_skip_directory_state_snapshot_without_permission_failure( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + def fail_if_called(_: Path) -> tuple[bool, tuple[tuple[str, int, int, int], ...]]: + raise AssertionError("directory state should be collected only after a permission failure") + + monkeypatch.setattr(file_io_module, "_directory_initialization_state", fail_if_called) + + path = tmp_path / "nested" / "data.txt" + write_text_file_atomically(path, "contents", create_parent_directories=True) + + assert read_text_file(path) == "contents" + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") def test_do_not_modify_permanently_inaccessible_parent( tmp_path: Path, From 2dde6dcbe85cfe95ea5a48545ab39eeb18dc75a5 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:59:37 +0000 Subject: [PATCH 20/23] Retry unresolved parent states Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 24 +++++---- python/src/qdk_chemistry/utils/file_io.py | 21 +++++--- python/tests/test_utils_file_io.py | 59 +++++++++++++++++++++-- 3 files changed, 82 insertions(+), 22 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index c7c042abd..2f2772272 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -104,9 +104,10 @@ class TransientPermissionError : public std::runtime_error { #ifndef _WIN32 using DirectoryStateEntry = std::tuple; -std::pair> +std::tuple> directory_initialization_state(const std::filesystem::path& directory) { bool initializing = false; + bool unresolved = false; std::vector state; auto current = directory; while (!current.empty()) { @@ -121,15 +122,16 @@ directory_initialization_state(const std::filesystem::path& directory) { } } else { const int status_error = errno; + unresolved = true; state.emplace_back(0, 0, 0, status_error); } const auto parent = current.parent_path(); if (parent == current) { - return {initializing, std::move(state)}; + return {initializing, unresolved, std::move(state)}; } current = parent; } - return {initializing, std::move(state)}; + return {initializing, unresolved, std::move(state)}; } #endif @@ -867,15 +869,17 @@ void create_private_directories(const std::filesystem::path& directory) { if (std::chrono::steady_clock::now() >= deadline) { throw; } - auto [initializing, current_state] = + auto [initializing, unresolved, current_state] = directory_initialization_state(directory); - if (!has_previous_state || current_state != previous_state) { + if (unresolved || initializing) { previous_state = std::move(current_state); has_previous_state = true; std::this_thread::sleep_for(retry_delay); continue; } - if (initializing) { + if (!has_previous_state || current_state != previous_state) { + previous_state = std::move(current_state); + has_previous_state = true; std::this_thread::sleep_for(retry_delay); continue; } @@ -1014,15 +1018,17 @@ void write_file_atomically(const std::filesystem::path& path, if (std::chrono::steady_clock::now() >= deadline) { throw; } - auto [initializing, current_state] = + auto [initializing, unresolved, current_state] = directory_initialization_state(destination.parent_path()); - if (!has_previous_state || current_state != previous_state) { + if (unresolved || initializing) { previous_state = std::move(current_state); has_previous_state = true; std::this_thread::sleep_for(std::chrono::milliseconds(1)); continue; } - if (initializing) { + if (!has_previous_state || current_state != previous_state) { + previous_state = std::move(current_state); + has_previous_state = true; std::this_thread::sleep_for(std::chrono::milliseconds(1)); continue; } diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 59a234121..96cc2e76e 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -766,16 +766,19 @@ def _create_private_directories_once(directory: Path) -> None: def _directory_initialization_state( directory: Path, -) -> tuple[bool, tuple[tuple[str, int, int, int], ...]]: +) -> tuple[bool, bool, tuple[tuple[str, int, int, int], ...]]: initializing = False + unresolved = False state: list[tuple[str, int, int, int]] = [] current = directory while True: try: status = current.stat() except FileNotFoundError: + unresolved = True state.append((os.fspath(current), -1, -1, errno.ENOENT)) except PermissionError: + unresolved = True state.append((os.fspath(current), -1, -1, errno.EACCES)) else: permissions = status.st_mode & 0o777 @@ -787,7 +790,7 @@ def _directory_initialization_state( ) parent = current.parent if parent == current: - return initializing, tuple(state) + return initializing, unresolved, tuple(state) current = parent @@ -805,12 +808,13 @@ def _create_private_directories(directory: Path) -> None: if time.monotonic() >= deadline: assert permission_error is not None raise permission_error - initializing, current_state = _directory_initialization_state(directory) - if previous_state is None or current_state != previous_state: + initializing, unresolved, current_state = _directory_initialization_state(directory) + if unresolved or initializing: previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue - if initializing: + if previous_state is None or current_state != previous_state: + previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue raise permission_error @@ -825,12 +829,13 @@ def _reserve_temporary_file_with_parent_retry(destination: Path) -> _TemporaryFi except PermissionError as error: if time.monotonic() >= deadline: raise - initializing, current_state = _directory_initialization_state(destination.parent) - if previous_state is None or current_state != previous_state: + initializing, unresolved, current_state = _directory_initialization_state(destination.parent) + if unresolved or initializing: previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue - if initializing: + if previous_state is None or current_state != previous_state: + previous_state = current_state time.sleep(_DIRECTORY_CREATION_RETRY_DELAY_SECONDS) continue raise error diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index ca4436944..fd1dada50 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -249,9 +249,10 @@ def test_retry_after_multiple_directory_state_changes( directory = tmp_path / "nested" / "parent" states = iter( [ - (False, (("root", 1, 1, 0o700),)), - (False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), + (False, False, (("root", 1, 1, 0o700),)), + (False, False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), ( + False, False, ( ("root", 1, 1, 0o700), @@ -286,9 +287,10 @@ def test_retry_reservation_after_multiple_directory_state_changes( reservation = object() states = iter( [ - (False, (("root", 1, 1, 0o700),)), - (False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), + (False, False, (("root", 1, 1, 0o700),)), + (False, False, (("root", 1, 1, 0o700), ("nested", 1, 2, 0o700))), ( + False, False, ( ("root", 1, 1, 0o700), @@ -319,7 +321,7 @@ def test_skip_directory_state_snapshot_without_permission_failure( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ): - def fail_if_called(_: Path) -> tuple[bool, tuple[tuple[str, int, int, int], ...]]: + def fail_if_called(_: Path) -> tuple[bool, bool, tuple[tuple[str, int, int, int], ...]]: raise AssertionError("directory state should be collected only after a permission failure") monkeypatch.setattr(file_io_module, "_directory_initialization_state", fail_if_called) @@ -330,6 +332,53 @@ def fail_if_called(_: Path) -> tuple[bool, tuple[tuple[str, int, int, int], ...] assert read_text_file(path) == "contents" +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_while_directory_state_is_unresolved( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + directory = tmp_path / "nested" + unresolved_state = (False, True, (("nested", -1, -1, errno.EACCES),)) + calls = 0 + + def fail_twice(_: Path) -> None: + nonlocal calls + calls += 1 + if calls < 3: + raise PermissionError(errno.EACCES, "initializing") + + monkeypatch.setattr(file_io_module, "_directory_initialization_state", lambda _: unresolved_state) + monkeypatch.setattr(file_io_module, "_create_private_directories_once", fail_twice) + + file_io_module._create_private_directories(directory) + + assert calls == 3 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_retry_reservation_while_directory_state_is_unresolved( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +): + destination = tmp_path / "nested" / "data.txt" + reservation = object() + unresolved_state = (False, True, (("nested", -1, -1, errno.EACCES),)) + calls = 0 + + def fail_twice(_: Path) -> object: + nonlocal calls + calls += 1 + if calls < 3: + raise PermissionError(errno.EACCES, "initializing") + return reservation + + monkeypatch.setattr(file_io_module, "_directory_initialization_state", lambda _: unresolved_state) + monkeypatch.setattr(file_io_module, "_reserve_temporary_file", fail_twice) + + assert file_io_module._reserve_temporary_file_with_parent_retry(destination) is reservation + assert calls == 3 + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") def test_do_not_modify_permanently_inaccessible_parent( tmp_path: Path, From 53251307a2ceebb6575b00353724c65eb489220e Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:24:41 +0000 Subject: [PATCH 21/23] Preserve ENOENT retry progress Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/src/qdk/chemistry/utils/file_io.cpp | 2 +- python/src/qdk_chemistry/utils/file_io.py | 1 - python/tests/test_utils_file_io.py | 11 +++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/cpp/src/qdk/chemistry/utils/file_io.cpp b/cpp/src/qdk/chemistry/utils/file_io.cpp index 2f2772272..c39af150e 100644 --- a/cpp/src/qdk/chemistry/utils/file_io.cpp +++ b/cpp/src/qdk/chemistry/utils/file_io.cpp @@ -122,7 +122,7 @@ directory_initialization_state(const std::filesystem::path& directory) { } } else { const int status_error = errno; - unresolved = true; + unresolved = unresolved || status_error == EACCES; state.emplace_back(0, 0, 0, status_error); } const auto parent = current.parent_path(); diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index 96cc2e76e..d56385e22 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -775,7 +775,6 @@ def _directory_initialization_state( try: status = current.stat() except FileNotFoundError: - unresolved = True state.append((os.fspath(current), -1, -1, errno.ENOENT)) except PermissionError: unresolved = True diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index fd1dada50..be27619b1 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -379,6 +379,17 @@ def fail_twice(_: Path) -> object: assert calls == 3 +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") +def test_missing_directory_state_is_resolved(tmp_path: Path): + directory = tmp_path / "missing" / "nested" + + initializing, unresolved, state = file_io_module._directory_initialization_state(directory) + + assert not initializing + assert not unresolved + assert state[0][3] == errno.ENOENT + + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission semantics") def test_do_not_modify_permanently_inaccessible_parent( tmp_path: Path, From a5d1565f18bc263db334f9686d0ef0f5f580d721 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:43:44 +0000 Subject: [PATCH 22/23] Fix FileIO test helper typing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/tests/test_utils_file_io.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/tests/test_utils_file_io.py b/python/tests/test_utils_file_io.py index be27619b1..e3b716db6 100644 --- a/python/tests/test_utils_file_io.py +++ b/python/tests/test_utils_file_io.py @@ -74,19 +74,19 @@ def test_freeze_relative_parent_before_creation(tmp_path: Path, monkeypatch: pyt if os.name == "nt": mkdir = Path.mkdir - def change_directory_then_create(directory: Path, *args, **kwargs) -> None: + def change_directory_then_mkdir(directory: Path, *args, **kwargs) -> None: os.chdir(second_directory) mkdir(directory, *args, **kwargs) - monkeypatch.setattr(Path, "mkdir", change_directory_then_create) + monkeypatch.setattr(Path, "mkdir", change_directory_then_mkdir) else: create_private_directories = file_io_module._create_private_directories - def change_directory_then_create(directory: Path) -> None: + def change_directory_then_create_private(directory: Path) -> None: os.chdir(second_directory) create_private_directories(directory) - monkeypatch.setattr(file_io_module, "_create_private_directories", change_directory_then_create) + monkeypatch.setattr(file_io_module, "_create_private_directories", change_directory_then_create_private) ensure_parent_directory("nested/data.txt") From 165a312b971fdb3aee6950f5740c5fd3ac2cb5b0 Mon Sep 17 00:00:00 2001 From: Conrad Johnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 25 Aug 2026 19:16:31 +0000 Subject: [PATCH 23/23] Address Copilot FileIO review comments Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cpp/tests/test_file_io.cpp | 56 ++++++++++++++++++++--- python/src/qdk_chemistry/utils/file_io.py | 3 +- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/cpp/tests/test_file_io.cpp b/cpp/tests/test_file_io.cpp index e366c1be0..ebd704201 100644 --- a/cpp/tests/test_file_io.cpp +++ b/cpp/tests/test_file_io.cpp @@ -32,16 +32,54 @@ namespace { class FileIoTest : public ::testing::Test { protected: void SetUp() override { - root_ = std::filesystem::temp_directory_path() / - ("qdk_file_io_test_" + - std::to_string( - std::chrono::steady_clock::now().time_since_epoch().count())); - std::filesystem::create_directories(root_); + const auto timestamp = + std::chrono::steady_clock::now().time_since_epoch().count(); +#ifdef _WIN32 + const auto process_id = GetCurrentProcessId(); +#else + const auto process_id = ::getpid(); +#endif + for (int attempt = 0; attempt < 64; ++attempt) { + const auto candidate = + std::filesystem::temp_directory_path() / + ("qdk_file_io_test_" + std::to_string(process_id) + "_" + + std::to_string(timestamp) + "_" + std::to_string(attempt)); + std::error_code error; + if (std::filesystem::create_directory(candidate, error)) { + root_ = candidate; + return; + } + ASSERT_FALSE(error) << "Could not create test directory '" << candidate + << "': " << error.message(); + } + FAIL() << "Could not create a unique FileIO test directory"; } void TearDown() override { - std::error_code ignored; - std::filesystem::remove_all(root_, ignored); + if (root_.empty()) { + return; + } +#ifdef _WIN32 + if (std::filesystem::exists(root_)) { + for (const auto& entry : + std::filesystem::recursive_directory_iterator(root_)) { + const DWORD attributes = GetFileAttributesW(entry.path().c_str()); + ASSERT_NE(attributes, INVALID_FILE_ATTRIBUTES); + if ((attributes & FILE_ATTRIBUTE_READONLY) != 0) { + const DWORD writable_attributes = + (attributes & ~FILE_ATTRIBUTE_READONLY) == 0 + ? FILE_ATTRIBUTE_NORMAL + : attributes & ~FILE_ATTRIBUTE_READONLY; + ASSERT_NE( + SetFileAttributesW(entry.path().c_str(), writable_attributes), 0); + } + } + } +#endif + std::error_code error; + std::filesystem::remove_all(root_, error); + EXPECT_FALSE(error) << "Could not remove test directory '" << root_ + << "': " << error.message(); } std::filesystem::path root_; @@ -609,7 +647,11 @@ TEST_F(FileIoTest, RejectsReplacedTemporaryFile) { }), std::runtime_error); EXPECT_FALSE(std::filesystem::exists(path)); +#ifdef _WIN32 + EXPECT_FALSE(std::filesystem::exists(replacement_path)); +#else EXPECT_TRUE(std::filesystem::exists(replacement_path)); +#endif } #ifndef _WIN32 diff --git a/python/src/qdk_chemistry/utils/file_io.py b/python/src/qdk_chemistry/utils/file_io.py index d56385e22..17f637b77 100644 --- a/python/src/qdk_chemistry/utils/file_io.py +++ b/python/src/qdk_chemistry/utils/file_io.py @@ -92,12 +92,11 @@ def ensure_parent_directory(path: PathLike) -> None: def _validate_destination_path(path: PathLike) -> None: value = os.fspath(path) - separators = tuple(separator for separator in (os.sep, os.altsep) if separator) path_module = ntpath if sys.platform == "win32" else os.path final_component = path_module.basename(value) if "\0" in value: raise ValueError(f"Path contains an embedded NUL character: '{value}'") - if not value or value.endswith(separators) or final_component in ("", ".", ".."): + if not value or final_component in ("", ".", ".."): raise ValueError(f"Destination path must name a file: '{value}'") if sys.platform == "win32" and ":" in ntpath.splitdrive(value)[1]: raise ValueError(f"Windows alternate data streams are not supported: '{value}'")