diff options
| author | Stefan Boberg <[email protected]> | 2026-03-30 15:07:08 +0200 |
|---|---|---|
| committer | GitHub Enterprise <[email protected]> | 2026-03-30 15:07:08 +0200 |
| commit | 3540d676733efaddecf504b30e9a596465bd43f8 (patch) | |
| tree | 7a8d8b3d2da993e30c34e3ff36f659b90a2b228e /src/zencore | |
| parent | include rawHash in structure output for builds ls command (#903) (diff) | |
| download | zen-3540d676733efaddecf504b30e9a596465bd43f8.tar.xz zen-3540d676733efaddecf504b30e9a596465bd43f8.zip | |
Request validation and resilience improvements (#864)
### Security: Input validation & path safety
- **Reject local file references by default** in package parsing — only allow when explicitly opted in by the service (`ParseFlags::kAllowLocalReferences`) and validated by an `ILocalRefPolicy` (fail-closed: no policy = rejected)
- **`DataRootLocalRefPolicy`** restricts local ref paths to the server's data root via canonical path prefix matching
- **Validate attachment hashes** in compute HTTP handlers — decompresses and re-hashes each attachment at ingestion time to reject tampered payloads
- **Path traversal validation** for worker descriptions (`pathvalidation.h`) — rejects absolute paths, `..` components, Windows reserved device names, and invalid filename characters
- **Harden CbPackage parsing** against corrupt inputs — overflow-safe attachment count, bounds checks on local ref offset/size, graceful failure instead of `ZEN_ASSERT` for untrusted data
- **Harden legacy package parser** — reject zero-size binary fields, missing mappers, and optionally validate resolved attachment hashes
- **Bounds check in `CbPackageReader::MarshalLocalChunkReference`** — detect when `MakeFromFile` silently clamps offset+size to file size
### Reliability: Lock consolidation & bug fixes
- **Consolidate three action map locks into one** (`m_ActionMapLock`) — eliminates deadlock risk from multi-lock ordering, simplifies state transitions, and fixes a race where newly enqueued actions were briefly invisible to `GetActionResult`/`FindActionResult`
- **Fix infinite loop in `BaseRunnerGroup::SubmitActions`** when actions exceed total runner capacity — cap round-robin at `TotalCapacity` and default unassigned results to "No capacity"
- **Fix `MakeSafeAbsolutePathInPlace` for UNC paths** — `\server\share` now correctly becomes `\?\UNC\server\share` instead of `\?\server\share`
- **Fix `max_retries=0`** — previously fell through to the default of 3; now correctly means "no retries"
### New: ManagedProcessRunner
- Cross-platform process runner backed by `SubprocessManager` — uses async exit callbacks instead of polling, delegates CPU/memory metrics to the manager's built-in sampler
- `ProcessGroup` (JobObject on Windows, process group on POSIX) for bulk cancellation on shutdown
- `--managed` flag on `zen exec inproc` to select this runner
- Refactored monitor thread lifecycle — `StartMonitorThread()` now called from derived constructors to avoid calling virtual functions from base constructor
### Process management
- **Suppress crash dialogs** via `JOB_OBJECT_UILIMIT_ERRORMODE` + `SEM_NOGPFAULTERRORBOX` in both `WindowsProcessRunner` and `JobObject::Initialize` — prevents WER/Dr. Watson modal dialogs from blocking the monitor thread
- **CREATE_SUSPENDED → AssignProcessToJobObject → ResumeThread** pattern in `WindowsProcessRunner` — ensures job object assignment before process execution
- **Move stdout/stderr callbacks to `Spawn()` parameters** in `SubprocessManager` — prevents race where early output could be missed before callback installation
- Consistent PID logging across all runner types
### Test infrastructure
- **`zentest-appstub`**: Added `Fail` (configurable exit code) and `Crash` (abort / nullptr deref) test functions
- **Compute integration tests**: exit code handling, auto-retry exhaustion, manual reschedule after failure, mixed success/failure queues, crash handling (abort + nullptr), crash auto-retry, immediate query visibility after enqueue
- **Package format tests**: truncated header, bad magic, attachment count overflow, truncated data, local ref rejection/acceptance, policy enforcement (inside/outside root, traversal, no-policy fail-closed)
- **Legacy package parser tests**: empty input, zero-size binary, hash resolution with/without mapper, hash mismatch detection
- **UNC path tests** for `MakeSafeAbsolutePath`
### Misc
- ANSI color helper macros (`ZEN_RED`, `ZEN_BRIGHT_WHITE`, etc.) and `ZEN_BOLD`/`ZEN_DIM`/etc.
- Generic `fmt::formatter` for types with free `ToString` functions
- Compute dashboard: truncated hash display with monospace font and hover for full value
- Renamed `usonpackage_forcelink` → `cbpackage_forcelink`
- Compute enabled by default in xmake config (releases still explicitly disable)
Diffstat (limited to 'src/zencore')
| -rw-r--r-- | src/zencore/compactbinarypackage.cpp | 122 | ||||
| -rw-r--r-- | src/zencore/filesystem.cpp | 69 | ||||
| -rw-r--r-- | src/zencore/include/zencore/compactbinarypackage.h | 19 | ||||
| -rw-r--r-- | src/zencore/include/zencore/logging.h | 28 | ||||
| -rw-r--r-- | src/zencore/process.cpp | 16 | ||||
| -rw-r--r-- | src/zencore/zencore.cpp | 2 |
6 files changed, 235 insertions, 21 deletions
diff --git a/src/zencore/compactbinarypackage.cpp b/src/zencore/compactbinarypackage.cpp index 56a292ca6..cd268745c 100644 --- a/src/zencore/compactbinarypackage.cpp +++ b/src/zencore/compactbinarypackage.cpp @@ -684,14 +684,22 @@ namespace legacy { Writer.Save(Ar); } - bool TryLoadCbPackage(CbPackage& Package, IoBuffer InBuffer, BufferAllocator Allocator, CbPackage::AttachmentResolver* Mapper) + bool TryLoadCbPackage(CbPackage& Package, + IoBuffer InBuffer, + BufferAllocator Allocator, + CbPackage::AttachmentResolver* Mapper, + bool ValidateHashes) { BinaryReader Reader(InBuffer.Data(), InBuffer.Size()); - return TryLoadCbPackage(Package, Reader, Allocator, Mapper); + return TryLoadCbPackage(Package, Reader, Allocator, Mapper, ValidateHashes); } - bool TryLoadCbPackage(CbPackage& Package, BinaryReader& Reader, BufferAllocator Allocator, CbPackage::AttachmentResolver* Mapper) + bool TryLoadCbPackage(CbPackage& Package, + BinaryReader& Reader, + BufferAllocator Allocator, + CbPackage::AttachmentResolver* Mapper, + bool ValidateHashes) { Package = CbPackage(); for (;;) @@ -708,7 +716,11 @@ namespace legacy { if (ValueField.IsBinary()) { const MemoryView View = ValueField.AsBinaryView(); - if (View.GetSize() > 0) + if (View.GetSize() == 0) + { + return false; + } + else { SharedBuffer Buffer = SharedBuffer::MakeView(View, ValueField.GetOuterBuffer()).MakeOwned(); CbField HashField = LoadCompactBinary(Reader, Allocator); @@ -748,7 +760,11 @@ namespace legacy { { const IoHash Hash = ValueField.AsHash(); - ZEN_ASSERT(Mapper); + if (!Mapper) + { + return false; + } + if (SharedBuffer AttachmentData = (*Mapper)(Hash)) { IoHash RawHash; @@ -763,6 +779,10 @@ namespace legacy { } else { + if (ValidateHashes && IoHash::HashBuffer(AttachmentData) != Hash) + { + return false; + } const CbValidateError ValidationResult = ValidateCompactBinary(AttachmentData.GetView(), CbValidateMode::All); if (ValidationResult != CbValidateError::None) { @@ -801,13 +821,13 @@ namespace legacy { #if ZEN_WITH_TESTS void -usonpackage_forcelink() +cbpackage_forcelink() { } TEST_SUITE_BEGIN("core.compactbinarypackage"); -TEST_CASE("usonpackage") +TEST_CASE("cbpackage") { using namespace std::literals; @@ -997,7 +1017,7 @@ TEST_CASE("usonpackage") } } -TEST_CASE("usonpackage.serialization") +TEST_CASE("cbpackage.serialization") { using namespace std::literals; @@ -1303,7 +1323,7 @@ TEST_CASE("usonpackage.serialization") } } -TEST_CASE("usonpackage.invalidpackage") +TEST_CASE("cbpackage.invalidpackage") { const auto TestLoad = [](std::initializer_list<uint8_t> RawData, BufferAllocator Allocator = UniqueBuffer::Alloc) { const MemoryView RawView = MakeMemoryView(RawData); @@ -1345,6 +1365,90 @@ TEST_CASE("usonpackage.invalidpackage") } } +TEST_CASE("cbpackage.legacy.invalidpackage") +{ + const auto TestLegacyLoad = [](std::initializer_list<uint8_t> RawData) { + const MemoryView RawView = MakeMemoryView(RawData); + IoBuffer Buffer(IoBuffer::Wrap, const_cast<void*>(RawView.GetData()), RawView.GetSize()); + CbPackage Package; + CHECK_FALSE(legacy::TryLoadCbPackage(Package, Buffer, &UniqueBuffer::Alloc)); + }; + + SUBCASE("Empty") { TestLegacyLoad({}); } + + SUBCASE("Zero size binary rejects") + { + // A binary field with zero payload size should be rejected (would desync the reader) + BinaryWriter Writer; + CbWriter Cb; + Cb.AddBinary(MemoryView()); // zero-size binary + Cb.Save(Writer); + + IoBuffer Buffer(IoBuffer::Wrap, const_cast<void*>(MakeMemoryView(Writer).GetData()), MakeMemoryView(Writer).GetSize()); + CbPackage Package; + CHECK_FALSE(legacy::TryLoadCbPackage(Package, Buffer, &UniqueBuffer::Alloc)); + } +} + +TEST_CASE("cbpackage.legacy.hashresolution") +{ + // Build a valid legacy package with an object, then round-trip it + CbObjectWriter RootWriter; + RootWriter.AddString("name", "test"); + CbObject RootObject = RootWriter.Save(); + + CbAttachment ObjectAttach(RootObject); + + CbPackage OriginalPkg; + OriginalPkg.SetObject(RootObject); + OriginalPkg.AddAttachment(ObjectAttach); + + BinaryWriter Writer; + legacy::SaveCbPackage(OriginalPkg, Writer); + + IoBuffer Buffer(IoBuffer::Wrap, const_cast<void*>(MakeMemoryView(Writer).GetData()), MakeMemoryView(Writer).GetSize()); + CbPackage LoadedPkg; + CHECK(legacy::TryLoadCbPackage(LoadedPkg, Buffer, &UniqueBuffer::Alloc)); + + // The hash-only path requires a mapper — without one it should fail + CbWriter HashOnlyCb; + HashOnlyCb.AddHash(ObjectAttach.GetHash()); + HashOnlyCb.AddNull(); + BinaryWriter HashOnlyWriter; + HashOnlyCb.Save(HashOnlyWriter); + + IoBuffer HashOnlyBuffer(IoBuffer::Wrap, + const_cast<void*>(MakeMemoryView(HashOnlyWriter).GetData()), + MakeMemoryView(HashOnlyWriter).GetSize()); + CbPackage HashOnlyPkg; + CHECK_FALSE(legacy::TryLoadCbPackage(HashOnlyPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, nullptr)); + + // With a mapper that returns valid data, it should succeed + CbPackage::AttachmentResolver Resolver = [&](const IoHash& Hash) -> SharedBuffer { + if (Hash == ObjectAttach.GetHash()) + { + return RootObject.GetBuffer(); + } + return {}; + }; + CHECK(legacy::TryLoadCbPackage(HashOnlyPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &Resolver)); + + // Build a different but structurally valid CbObject to use as mismatched data + CbObjectWriter DifferentWriter; + DifferentWriter.AddString("name", "different"); + CbObject DifferentObject = DifferentWriter.Save(); + + CbPackage::AttachmentResolver BadResolver = [&](const IoHash&) -> SharedBuffer { return DifferentObject.GetBuffer(); }; + CbPackage BadPkg; + + // With ValidateHashes enabled and a mapper that returns mismatched data, it should fail + CHECK_FALSE(legacy::TryLoadCbPackage(BadPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &BadResolver, /*ValidateHashes*/ true)); + + // Without ValidateHashes, the mismatched data is accepted (structure is still valid CB) + CbPackage UncheckedPkg; + CHECK(legacy::TryLoadCbPackage(UncheckedPkg, HashOnlyBuffer, &UniqueBuffer::Alloc, &BadResolver, /*ValidateHashes*/ false)); +} + TEST_SUITE_END(); #endif diff --git a/src/zencore/filesystem.cpp b/src/zencore/filesystem.cpp index 416312cae..a63594be9 100644 --- a/src/zencore/filesystem.cpp +++ b/src/zencore/filesystem.cpp @@ -3277,12 +3277,23 @@ MakeSafeAbsolutePathInPlace(std::filesystem::path& Path) { Path = std::filesystem::absolute(Path).make_preferred(); #if ZEN_PLATFORM_WINDOWS - const std::string_view Prefix = "\\\\?\\"; - const std::u8string PrefixU8(Prefix.begin(), Prefix.end()); - std::u8string PathString = Path.u8string(); - if (!PathString.empty() && !PathString.starts_with(PrefixU8)) + const std::u8string_view LongPathPrefix = u8"\\\\?\\"; + const std::u8string_view UncPrefix = u8"\\\\"; + const std::u8string_view LongPathUncPrefix = u8"\\\\?\\UNC\\"; + + std::u8string PathString = Path.u8string(); + if (!PathString.empty() && !PathString.starts_with(LongPathPrefix)) { - PathString.insert(0, PrefixU8); + if (PathString.starts_with(UncPrefix)) + { + // UNC path: \\server\share → \\?\UNC\server\share + PathString.replace(0, UncPrefix.size(), LongPathUncPrefix); + } + else + { + // Local path: C:\foo → \\?\C:\foo + PathString.insert(0, LongPathPrefix); + } Path = PathString; } #endif // ZEN_PLATFORM_WINDOWS @@ -4049,6 +4060,54 @@ TEST_CASE("SharedMemory") CHECK(!OpenSharedMemory("SharedMemoryTest0", 482, false)); } +TEST_CASE("filesystem.MakeSafeAbsolutePath") +{ +# if ZEN_PLATFORM_WINDOWS + // Local path gets \\?\ prefix + { + std::filesystem::path Local = MakeSafeAbsolutePath("C:\\Users\\test"); + CHECK(Local.u8string().starts_with(u8"\\\\?\\")); + CHECK(Local.u8string().find(u8"C:\\Users\\test") != std::u8string::npos); + } + + // UNC path gets \\?\UNC\ prefix + { + std::filesystem::path Unc = MakeSafeAbsolutePath("\\\\server\\share\\path"); + std::u8string UncStr = Unc.u8string(); + CHECK_MESSAGE(UncStr.starts_with(u8"\\\\?\\UNC\\"), fmt::format("Expected \\\\?\\UNC\\ prefix, got '{}'", Unc)); + CHECK_MESSAGE(UncStr.find(u8"server\\share\\path") != std::u8string::npos, + fmt::format("Expected server\\share\\path in '{}'", Unc)); + // Must NOT produce \\?\\\server (double backslash after \\?\) + CHECK_MESSAGE(UncStr.find(u8"\\\\?\\\\\\") == std::u8string::npos, + fmt::format("Path contains invalid double-backslash after prefix: '{}'", Unc)); + } + + // Already-prefixed path is not double-prefixed + { + std::filesystem::path Already = MakeSafeAbsolutePath("\\\\?\\C:\\already\\prefixed"); + size_t Count = 0; + std::u8string Str = Already.u8string(); + for (size_t Pos = Str.find(u8"\\\\?\\"); Pos != std::u8string::npos; Pos = Str.find(u8"\\\\?\\", Pos + 1)) + { + ++Count; + } + CHECK_EQ(Count, 1); + } + + // Already-prefixed UNC path is not double-prefixed + { + std::filesystem::path AlreadyUnc = MakeSafeAbsolutePath("\\\\?\\UNC\\server\\share"); + size_t Count = 0; + std::u8string Str = AlreadyUnc.u8string(); + for (size_t Pos = Str.find(u8"\\\\?\\"); Pos != std::u8string::npos; Pos = Str.find(u8"\\\\?\\", Pos + 1)) + { + ++Count; + } + CHECK_EQ(Count, 1); + } +# endif // ZEN_PLATFORM_WINDOWS +} + TEST_SUITE_END(); #endif diff --git a/src/zencore/include/zencore/compactbinarypackage.h b/src/zencore/include/zencore/compactbinarypackage.h index 64b62e2c0..148c0d3fd 100644 --- a/src/zencore/include/zencore/compactbinarypackage.h +++ b/src/zencore/include/zencore/compactbinarypackage.h @@ -278,10 +278,10 @@ public: * @return The attachment, or null if the attachment is not found. * @note The returned pointer is only valid until the attachments on this package are modified. */ - const CbAttachment* FindAttachment(const IoHash& Hash) const; + [[nodiscard]] const CbAttachment* FindAttachment(const IoHash& Hash) const; /** Find an attachment if it exists in the package. */ - inline const CbAttachment* FindAttachment(const CbAttachment& Attachment) const { return FindAttachment(Attachment.GetHash()); } + [[nodiscard]] const CbAttachment* FindAttachment(const CbAttachment& Attachment) const { return FindAttachment(Attachment.GetHash()); } /** Add the attachment to this package. */ inline void AddAttachment(const CbAttachment& Attachment) { AddAttachment(Attachment, nullptr); } @@ -336,17 +336,26 @@ private: IoHash ObjectHash; }; +/** In addition to the above, we also support a legacy format which is used by + * the HTTP project store for historical reasons. Don't use the below functions + * for new code. + */ namespace legacy { void SaveCbAttachment(const CbAttachment& Attachment, CbWriter& Writer); void SaveCbPackage(const CbPackage& Package, CbWriter& Writer); void SaveCbPackage(const CbPackage& Package, BinaryWriter& Ar); - bool TryLoadCbPackage(CbPackage& Package, IoBuffer Buffer, BufferAllocator Allocator, CbPackage::AttachmentResolver* Mapper = nullptr); + bool TryLoadCbPackage(CbPackage& Package, + IoBuffer Buffer, + BufferAllocator Allocator, + CbPackage::AttachmentResolver* Mapper = nullptr, + bool ValidateHashes = false); bool TryLoadCbPackage(CbPackage& Package, BinaryReader& Reader, BufferAllocator Allocator, - CbPackage::AttachmentResolver* Mapper = nullptr); + CbPackage::AttachmentResolver* Mapper = nullptr, + bool ValidateHashes = false); } // namespace legacy -void usonpackage_forcelink(); // internal +void cbpackage_forcelink(); // internal } // namespace zen diff --git a/src/zencore/include/zencore/logging.h b/src/zencore/include/zencore/logging.h index 3427991d2..1608ad523 100644 --- a/src/zencore/include/zencore/logging.h +++ b/src/zencore/include/zencore/logging.h @@ -90,6 +90,34 @@ using zen::ConsoleLog; using zen::ErrorLog; using zen::Log; +//////////////////////////////////////////////////////////////////////// +// Color helpers + +#define ZEN_RED(str) "\033[31m" str "\033[0m" +#define ZEN_GREEN(str) "\033[32m" str "\033[0m" +#define ZEN_YELLOW(str) "\033[33m" str "\033[0m" +#define ZEN_BLUE(str) "\033[34m" str "\033[0m" +#define ZEN_MAGENTA(str) "\033[35m" str "\033[0m" +#define ZEN_CYAN(str) "\033[36m" str "\033[0m" +#define ZEN_WHITE(str) "\033[37m" str "\033[0m" + +#define ZEN_BRIGHT_RED(str) "\033[91m" str "\033[0m" +#define ZEN_BRIGHT_GREEN(str) "\033[92m" str "\033[0m" +#define ZEN_BRIGHT_YELLOW(str) "\033[93m" str "\033[0m" +#define ZEN_BRIGHT_BLUE(str) "\033[94m" str "\033[0m" +#define ZEN_BRIGHT_MAGENTA(str) "\033[95m" str "\033[0m" +#define ZEN_BRIGHT_CYAN(str) "\033[96m" str "\033[0m" +#define ZEN_BRIGHT_WHITE(str) "\033[97m" str "\033[0m" + +#define ZEN_BOLD(str) "\033[1m" str "\033[0m" +#define ZEN_UNDERLINE(str) "\033[4m" str "\033[0m" +#define ZEN_DIM(str) "\033[2m" str "\033[0m" +#define ZEN_ITALIC(str) "\033[3m" str "\033[0m" +#define ZEN_STRIKETHROUGH(str) "\033[9m" str "\033[0m" +#define ZEN_INVERSE(str) "\033[7m" str "\033[0m" + +//////////////////////////////////////////////////////////////////////// + #if ZEN_BUILD_DEBUG # define ZEN_CHECK_FORMAT_STRING(fmtstr, ...) \ while (false) \ diff --git a/src/zencore/process.cpp b/src/zencore/process.cpp index e7baa3f8e..9cbbfa56a 100644 --- a/src/zencore/process.cpp +++ b/src/zencore/process.cpp @@ -1252,14 +1252,28 @@ JobObject::Initialize() } JOBOBJECT_EXTENDED_LIMIT_INFORMATION LimitInfo = {}; - LimitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + LimitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION; if (!SetInformationJobObject(m_JobHandle, JobObjectExtendedLimitInformation, &LimitInfo, sizeof(LimitInfo))) { ZEN_WARN("Failed to set job object limits: {}", zen::GetLastError()); CloseHandle(m_JobHandle); m_JobHandle = nullptr; + return; } + + // Prevent child processes from clearing SEM_NOGPFAULTERRORBOX, which + // suppresses WER/Dr. Watson crash dialogs. Without this, a crashing + // child can pop a modal dialog and block the monitor thread. +# if !defined(JOB_OBJECT_UILIMIT_ERRORMODE) +# define JOB_OBJECT_UILIMIT_ERRORMODE 0x00000400 +# endif + JOBOBJECT_BASIC_UI_RESTRICTIONS UiRestrictions{}; + UiRestrictions.UIRestrictionsClass = JOB_OBJECT_UILIMIT_ERRORMODE; + SetInformationJobObject(m_JobHandle, JobObjectBasicUIRestrictions, &UiRestrictions, sizeof(UiRestrictions)); + + // Set error mode on the current process so children inherit it. + SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX); } bool diff --git a/src/zencore/zencore.cpp b/src/zencore/zencore.cpp index 8c29a8962..c1ac63621 100644 --- a/src/zencore/zencore.cpp +++ b/src/zencore/zencore.cpp @@ -273,7 +273,7 @@ zencore_forcelinktests() zen::uid_forcelink(); zen::uson_forcelink(); zen::usonbuilder_forcelink(); - zen::usonpackage_forcelink(); + zen::cbpackage_forcelink(); zen::cbjson_forcelink(); zen::cbyaml_forcelink(); zen::workthreadpool_forcelink(); |