diff --git a/CHANGELOG.md b/CHANGELOG.md index 021c1a51..0e8851a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,20 @@ SceneEntities and PlotMarkers: - `PlotMarkers`: the decoder drops a marker whose `kind` it does not know instead of drawing it as a time region; wire numbers unchanged. +Plugin catalog and source cache, errors that threw instead of being returned: + +- On Windows, a plugin path that the active code page cannot represent no longer makes + `inspectPluginDso` or `scanPluginDsos` throw. They formatted their own error text with + `path.string()`, and the scan checked each file's suffix the same way, so one such file anywhere + under the plugin folder made the whole scan throw. Paths in these messages are now UTF-8 on + Windows whatever the code page; a consumer that decoded them as the local code page must decode + UTF-8. +- `scanPluginDsos` reports a subdirectory it cannot read and goes on with the rest. It threw + (`recursive_directory_iterator cannot pop`) on the first unreadable directory, on every platform. +- The source request cache has the same Windows fix for its lock and flush errors. "Lock is held + elsewhere", an ordinary outcome during cleanup and eviction, threw under a cache root the code + page cannot represent. + Removed: - **Breaking (source):** the `pj.colormap.v1` service: `PJ_colormap_registry_vtable_t`, diff --git a/pj_base/src/source/file_lock.cpp b/pj_base/src/source/file_lock.cpp index 571035bc..5e4772cb 100644 --- a/pj_base/src/source/file_lock.cpp +++ b/pj_base/src/source/file_lock.cpp @@ -6,6 +6,8 @@ #include #include +#include "source/path_utf8.hpp" + #if defined(_WIN32) // windows.h min/max macros break std::max / numeric_limits<>::max. #ifndef NOMINMAX @@ -46,7 +48,7 @@ std::optional tryAcquireHandle( FILE_ATTRIBUTE_NORMAL, nullptr); if (handle == INVALID_HANDLE_VALUE) { if (error != nullptr) { - *error = "could not open lock file " + path.string() + " (error " + std::to_string(::GetLastError()) + ")"; + *error = "could not open lock file " + pathUtf8(path) + " (error " + std::to_string(::GetLastError()) + ")"; } return std::nullopt; } @@ -58,7 +60,7 @@ std::optional tryAcquireHandle( *contended = true; // FAIL_IMMEDIATELY: the region is held elsewhere } if (error != nullptr) { - *error = "lock is held elsewhere: " + path.string(); + *error = "lock is held elsewhere: " + pathUtf8(path); } return std::nullopt; } @@ -67,7 +69,7 @@ std::optional tryAcquireHandle( const int fd = ::open(path.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0600); if (fd < 0) { if (error != nullptr) { - *error = "could not open lock file " + path.string() + ": " + + *error = "could not open lock file " + pathUtf8(path) + ": " + std::error_code(errno, std::generic_category()).message(); } return std::nullopt; @@ -80,8 +82,8 @@ std::optional tryAcquireHandle( *contended = held_elsewhere; } if (error != nullptr) { - *error = held_elsewhere ? "lock is held elsewhere: " + path.string() - : "flock failed on " + path.string() + ": " + + *error = held_elsewhere ? "lock is held elsewhere: " + pathUtf8(path) + : "flock failed on " + pathUtf8(path) + ": " + std::error_code(flock_errno, std::generic_category()).message(); } return std::nullopt; diff --git a/pj_base/src/source/fs_durability.cpp b/pj_base/src/source/fs_durability.cpp index efe307d4..393f61f5 100644 --- a/pj_base/src/source/fs_durability.cpp +++ b/pj_base/src/source/fs_durability.cpp @@ -6,6 +6,8 @@ #include #include +#include "source/path_utf8.hpp" + #if defined(_WIN32) // windows.h min/max macros break std::max / numeric_limits<>::max. #ifndef NOMINMAX @@ -44,21 +46,21 @@ bool syncFile(const fs::path& file, std::string* error) { file.c_str(), GENERIC_WRITE, FILE_SHARE_READ, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); if (handle == INVALID_HANDLE_VALUE) { if (error != nullptr) { - *error = "could not reopen for flush: " + file.string(); + *error = "could not reopen for flush: " + pathUtf8(file); } return false; } const bool ok = ::FlushFileBuffers(handle) != 0; ::CloseHandle(handle); if (!ok && error != nullptr) { - *error = "FlushFileBuffers failed: " + file.string(); + *error = "FlushFileBuffers failed: " + pathUtf8(file); } return ok; #else const int fd = ::open(file.c_str(), O_RDONLY | O_CLOEXEC); if (fd < 0) { if (error != nullptr) { - *error = "could not reopen for fsync: " + file.string() + ": " + + *error = "could not reopen for fsync: " + pathUtf8(file) + ": " + std::error_code(errno, std::generic_category()).message(); } return false; @@ -67,7 +69,7 @@ bool syncFile(const fs::path& file, std::string* error) { const int fsync_errno = errno; ::close(fd); if (!ok && error != nullptr) { - *error = "fsync failed: " + file.string() + ": " + std::error_code(fsync_errno, std::generic_category()).message(); + *error = "fsync failed: " + pathUtf8(file) + ": " + std::error_code(fsync_errno, std::generic_category()).message(); } return ok; #endif diff --git a/pj_base/src/source/path_utf8.hpp b/pj_base/src/source/path_utf8.hpp new file mode 100644 index 00000000..b1507323 --- /dev/null +++ b/pj_base/src/source/path_utf8.hpp @@ -0,0 +1,27 @@ +#pragma once +// Copyright 2026 Davide Faconti +// SPDX-License-Identifier: Apache-2.0 + +#include +#include + +namespace PJ { +namespace sdk { +namespace source { +namespace detail { + +/// The path as UTF-8, for error text and name checks. +/// +/// Use this instead of `path.string()`: on Windows that narrows through the +/// active code page and throws `std::system_error` for a name the code page +/// cannot represent, so a function meant to return an error throws instead. +/// On POSIX the result is the same bytes `string()` returns. +[[nodiscard]] inline std::string pathUtf8(const std::filesystem::path& path) { + const std::u8string utf8 = path.u8string(); + return {utf8.begin(), utf8.end()}; +} + +} // namespace detail +} // namespace source +} // namespace sdk +} // namespace PJ diff --git a/pj_base/src/source/request_cache.cpp b/pj_base/src/source/request_cache.cpp index 949954ae..f3ac27d7 100644 --- a/pj_base/src/source/request_cache.cpp +++ b/pj_base/src/source/request_cache.cpp @@ -11,6 +11,7 @@ #include "pj_base/sdk/testing/request_cache_probe.hpp" #include "source/file_lock.hpp" #include "source/fs_durability.hpp" +#include "source/path_utf8.hpp" #if defined(_WIN32) // windows.h min/max macros break std::max / numeric_limits<>::max. @@ -85,10 +86,9 @@ class CacheLayout { /// Managed names only: "" and ".partial."; /// sidecars, foreign files and malformed digests are kOther. [[nodiscard]] Classified classify(const fs::path& file) const { - // u8string(): a foreign Unicode name classifies as kOther instead of + // pathUtf8(): a foreign Unicode name classifies as kOther instead of // throwing through .string() on a non-ACP Windows code page. - const std::u8string u8name = file.filename().u8string(); - const std::string name(u8name.begin(), u8name.end()); + const std::string name = detail::pathUtf8(file.filename()); const std::size_t hex = spec_.identity.hexChars(); const auto digestOk = [&name, hex] { return name.size() >= hex && std::all_of( @@ -238,7 +238,7 @@ Expected RequestArtifactCache::WriteTrans impl.lock.reset(); return unexpected(CacheError{std::move(reason)}); }; - const std::string what = "cache commit rejected " + impl.partial.filename().string() + ": "; + const std::string what = "cache commit rejected " + detail::pathUtf8(impl.partial.filename()) + ": "; if (!impl.validator) { return fail(what + "no artifact validator configured"); } diff --git a/pj_base/tests/descriptor_import_request_cache_test.cpp b/pj_base/tests/descriptor_import_request_cache_test.cpp index 25cc65e8..b8d0b24a 100644 --- a/pj_base/tests/descriptor_import_request_cache_test.cpp +++ b/pj_base/tests/descriptor_import_request_cache_test.cpp @@ -434,6 +434,23 @@ TEST_F(RequestCacheTest, UnicodeCacheRootRoundTripsAndCleans) { EXPECT_FALSE(fs::exists(ucache.pathFor(identityFor(kHexA)))); } +TEST_F(RequestCacheTest, UnicodeCacheRootReportsContentionInsteadOfThrowing) { + // "Lock is held elsewhere" is an ordinary outcome, and its message names the + // lock file. On Windows that name went through path.string(), which throws + // for a root the active code page cannot represent. + const fs::path uroot = root() / fs::path(u8"пж-缓存-ü"); + fs::create_directories(uroot); + RequestArtifactCache ucache(CacheSpec{uroot, kSuffix, IdentityScheme{kPrefix, 32}}, acceptAll()); + auto first = ucache.beginWrite(identityFor(kHexA)); + ASSERT_TRUE(first) << first.error().message; + auto second = ucache.beginWrite(identityFor(kHexA)); + ASSERT_FALSE(second); + EXPECT_TRUE(second.error().retryable); + const std::u8string u8root = uroot.filename().u8string(); + EXPECT_NE(second.error().message.find(std::string(u8root.begin(), u8root.end())), std::string::npos) + << second.error().message; +} + TEST_F(RequestCacheTest, OutOfRangeDigestWidthStillRoundTrips) { // The spec's scheme normalizes the width exactly like identity minting, so // a provider that misconfigures digest_hex_chars still gets a coherent diff --git a/pj_plugins/src/plugin_catalog.cpp b/pj_plugins/src/plugin_catalog.cpp index 83bbd32c..55d27c2c 100644 --- a/pj_plugins/src/plugin_catalog.cpp +++ b/pj_plugins/src/plugin_catalog.cpp @@ -43,8 +43,10 @@ struct ManifestCandidate { std::string manifest_json; }; +// Compared as paths, so no file name is ever narrowed: a name that cannot be +// converted must not make the scan throw. bool hasDsoSuffix(const std::filesystem::path& path) { - return path.extension().string() == kDsoSuffix; + return path.extension() == std::filesystem::path(kDsoSuffix); } bool nativeSymbolPresent(void* handle, const char* symbol) noexcept { @@ -402,13 +404,13 @@ std::string_view toString(PluginFamily family) noexcept { Expected inspectPluginDso(const std::filesystem::path& dso_path) { if (!hasDsoSuffix(dso_path)) { - return unexpected(fmt::format("not a platform plugin DSO: {}", dso_path.string())); + return unexpected(fmt::format("not a platform plugin DSO: {}", detail::pathForLegacyAccessor(dso_path))); } detail::LibraryPathIdentity recorded_path; auto raw_handle = detail::loadLibraryHandle(dso_path, &recorded_path); if (!raw_handle) { - return unexpected(fmt::format("{}: {}", dso_path.string(), raw_handle.error())); + return unexpected(fmt::format("{}: {}", detail::pathForLegacyAccessor(dso_path), raw_handle.error())); } return inspectPluginDso(detail::adoptLibraryHandle(*raw_handle), recorded_path.load_path); } @@ -416,9 +418,11 @@ Expected inspectPluginDso(const std::filesystem::path& dso_pat Expected inspectPluginDso( const std::shared_ptr& handle, const std::filesystem::path& dso_path) { if (!hasDsoSuffix(dso_path)) { - return unexpected(fmt::format("not a platform plugin DSO: {}", dso_path.string())); + return unexpected(fmt::format("not a platform plugin DSO: {}", detail::pathForLegacyAccessor(dso_path))); } - auto with_path = [&](const std::string& error) { return fmt::format("{}: {}", dso_path.string(), error); }; + auto with_path = [&](const std::string& error) { + return fmt::format("{}: {}", detail::pathForLegacyAccessor(dso_path), error); + }; if (handle == nullptr) { return unexpected(with_path("library not loaded")); } @@ -487,10 +491,10 @@ Expected> exportedPluginFamilies( Expected scanPluginDsos(const std::filesystem::path& directory) { std::error_code ec; if (!std::filesystem::exists(directory, ec)) { - return unexpected(fmt::format("plugin directory not found: {}", directory.string())); + return unexpected(fmt::format("plugin directory not found: {}", detail::pathForLegacyAccessor(directory))); } if (!std::filesystem::is_directory(directory, ec)) { - return unexpected(fmt::format("plugin path is not a directory: {}", directory.string())); + return unexpected(fmt::format("plugin path is not a directory: {}", detail::pathForLegacyAccessor(directory))); } PluginScanResult result; @@ -518,12 +522,25 @@ Expected scanPluginDsos(const std::filesystem::path& directory result.diagnostics.push_back({entry.path(), fmt::format("stat failed: {}", entry_ec.message())}); } + // increment() descends into a directory entry, and when that fails the + // iterator is left at the end: the scan is over. So a directory that + // cannot be opened is found here, reported, and not descended into. + std::error_code type_ec; + if (it.recursion_pending() && entry.is_directory(type_ec) && !entry.is_symlink(type_ec)) { + std::error_code open_ec; + const std::filesystem::directory_iterator probe(entry.path(), open_ec); + if (open_ec) { + result.diagnostics.push_back({entry.path(), fmt::format("directory iteration failed: {}", open_ec.message())}); + it.disable_recursion_pending(); + } + } + std::error_code inc_ec; it.increment(inc_ec); if (inc_ec) { + // The iterator is at the end now; pop() on it would throw. result.diagnostics.push_back({entry.path(), fmt::format("directory iteration failed: {}", inc_ec.message())}); - // Skip the unreadable subtree but continue with the rest of the scan. - it.pop(); + break; } } diff --git a/pj_plugins/tests/plugin_catalog_test.cpp b/pj_plugins/tests/plugin_catalog_test.cpp index 3fcb8545..e90db44f 100644 --- a/pj_plugins/tests/plugin_catalog_test.cpp +++ b/pj_plugins/tests/plugin_catalog_test.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include "detail/library_loader.hpp" @@ -25,6 +26,8 @@ #define WIN32_LEAN_AND_MEAN #endif #include +#else +#include #endif namespace PJ { @@ -68,6 +71,58 @@ class PluginCatalogTest : public ::testing::Test { std::filesystem::path dir_; }; +// Unicode errors must be UTF-8 on every Windows ACP, not only UTF-8 systems. +TEST_F(PluginCatalogTest, UnicodeFailureDiagnosticsPreservePath) { + const auto directory = dir_ / std::filesystem::path(u8"\u8cc7\u6599-\U0001f680"); + const auto utf8 = directory.u8string(); + const std::string expected(utf8.begin(), utf8.end()); + auto missing = scanPluginDsos(directory); + ASSERT_FALSE(missing.has_value()); + EXPECT_NE(missing.error().find(expected), std::string::npos); + // Some filesystems cannot store this name (no supplementary-plane characters, + // or a non-Unicode encoding). That is not a catalog failure: skip the cases + // that need the directory, but still fail on any other I/O error. + std::error_code created; + std::filesystem::create_directories(directory, created); + if (created == std::errc::illegal_byte_sequence || created == std::errc::invalid_argument) { + GTEST_SKIP() << "filesystem cannot store the Unicode test name: " << created.message(); + } + ASSERT_FALSE(created) << created.message(); + const auto broken = directory / pluginFileName("broken"); + { + std::ofstream output(broken); + ASSERT_TRUE(output.is_open()); + output << "not a shared library"; + ASSERT_TRUE(output.good()); + } + auto invalid = inspectPluginDso(broken); + ASSERT_FALSE(invalid.has_value()); + EXPECT_NE(invalid.error().find(expected), std::string::npos); + auto unopened = inspectPluginDso(std::shared_ptr{}, broken); + ASSERT_FALSE(unopened.has_value()); + EXPECT_NE(unopened.error().find(expected), std::string::npos); + // A suffix made of a supplementary-plane character is refused as "not a + // plugin", by both overloads, before anything is loaded. + const auto not_a_plugin = directory / std::filesystem::path(u8"file.\U0001f680"); + auto unsupported = inspectPluginDso(not_a_plugin); + ASSERT_FALSE(unsupported.has_value()); + EXPECT_NE(unsupported.error().find("not a platform plugin DSO"), std::string::npos) << unsupported.error(); + EXPECT_NE(unsupported.error().find(expected), std::string::npos); + auto unsupported_handle = inspectPluginDso(std::shared_ptr{}, not_a_plugin); + ASSERT_FALSE(unsupported_handle.has_value()); + EXPECT_NE(unsupported_handle.error().find("not a platform plugin DSO"), std::string::npos) + << unsupported_handle.error(); + EXPECT_NE(unsupported_handle.error().find(expected), std::string::npos); + auto scan = scanPluginDsos(directory); + ASSERT_TRUE(scan.has_value()) << scan.error(); + EXPECT_TRUE(scan->plugins.empty()); + ASSERT_EQ(scan->diagnostics.size(), 1u); + EXPECT_NE(scan->diagnostics.front().message.find(expected), std::string::npos); + auto not_directory = scanPluginDsos(broken); + ASSERT_FALSE(not_directory.has_value()); + EXPECT_NE(not_directory.error().find(expected), std::string::npos); +} + TEST_F(PluginCatalogTest, MissingDirectoryReturnsError) { auto result = scanPluginDsos("/nonexistent/path/xyz"); EXPECT_FALSE(result.has_value()); @@ -184,6 +239,42 @@ TEST_F(PluginCatalogTest, ForwardedEntryPointIsRejected) { } #endif +#if !defined(_WIN32) +// A subdirectory the scan cannot read (an extension folder owned by another +// user) is reported and skipped. It must not end the scan, and it must not +// throw: the scan's contract is to return diagnostics. +TEST_F(PluginCatalogTest, ScanReportsAnUnreadableSubdirectoryAndContinues) { + if (::geteuid() == 0) { + GTEST_SKIP() << "root can read every directory"; + } + const std::filesystem::path locked = dir_ / "a_locked"; + const std::filesystem::path later = dir_ / "z_later"; + std::filesystem::create_directories(locked); + std::filesystem::create_directories(later); + std::ofstream(later / pluginFileName("broken")) << "not a shared library"; + std::filesystem::permissions(locked, std::filesystem::perms::none); + struct Restore { + std::filesystem::path path; + ~Restore() { + std::error_code ec; // so TearDown can remove the tree whatever the test did + std::filesystem::permissions(path, std::filesystem::perms::owner_all, ec); + } + } restore{locked}; + + auto scan = scanPluginDsos(dir_); + ASSERT_TRUE(scan.has_value()) << scan.error(); + EXPECT_TRUE(scan->plugins.empty()); + bool reported_locked = false; + bool reached_later = false; + for (const auto& diagnostic : scan->diagnostics) { + reported_locked = reported_locked || diagnostic.path == locked; + reached_later = reached_later || diagnostic.path == later / pluginFileName("broken"); + } + EXPECT_TRUE(reported_locked) << "the unreadable directory is named in a diagnostic"; + EXPECT_TRUE(reached_later) << "the scan went on to the entries after it"; +} +#endif + TEST_F(PluginCatalogTest, UnicodeExtensionPathLoadsOnWindows) { const std::filesystem::path unicode_dir = dir_ / std::filesystem::path(u8"插件-π"); std::filesystem::create_directories(unicode_dir);