aboutsummaryrefslogtreecommitdiff
path: root/src/zencore
diff options
context:
space:
mode:
authorStefan Boberg <[email protected]>2026-03-30 15:07:08 +0200
committerGitHub Enterprise <[email protected]>2026-03-30 15:07:08 +0200
commit3540d676733efaddecf504b30e9a596465bd43f8 (patch)
tree7a8d8b3d2da993e30c34e3ff36f659b90a2b228e /src/zencore
parentinclude rawHash in structure output for builds ls command (#903) (diff)
downloadzen-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.cpp122
-rw-r--r--src/zencore/filesystem.cpp69
-rw-r--r--src/zencore/include/zencore/compactbinarypackage.h19
-rw-r--r--src/zencore/include/zencore/logging.h28
-rw-r--r--src/zencore/process.cpp16
-rw-r--r--src/zencore/zencore.cpp2
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();