From e8aad9ca20736607390e05006c270f230d2adf60 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 11:33:44 -0500 Subject: [PATCH 1/5] Low-severity hardening: CSPRNG UUIDs (POSIX) + null-safe zlib error logs Two low-severity issues found during a repo-wide review: 1) PAL::generateUuidString POSIX/Android fallback built the UUID entirely from std::rand(), seeded once with srand(time(0) ^ nanos). std::rand() is a weak, predictable PRNG with a guessable time-based seed, so the session / event / instance identifiers derived from it were predictable. Source the bytes from std::random_device instead (backed by /dev/urandom on Linux/Android), matching the existing CorrelationVector.cpp / PseudoRandomGenerator usage. Windows (CoCreateGuid) and Apple (CFUUIDCreate) paths are unchanged. Test: PalTests.UuidGeneration extended to assert 1000 generated UUIDs are all distinct (in addition to the existing format/entropy checks). 2) zlib error-path logs passed stream.msg / zs.msg straight to a %s conversion. zlib leaves msg == Z_NULL for several error codes (Z_MEM_ERROR, Z_BUF_ERROR, after a failed deflateInit2), and printf("%s", NULL) is undefined behavior -- benign "(null)" on glibc but not guaranteed across the MSVC/Android/Apple CRTs this SDK targets. Guard with `msg ? msg : "(null)"` in ZlibUtils.cpp and the two HttpDeflateCompression.cpp sites. Verified on Linux host: UnitTests builds; PalTests (incl. the UUID uniqueness check) and ZlibUtilsTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/compression/HttpDeflateCompression.cpp | 4 ++-- lib/pal/PAL.cpp | 18 ++++++++---------- lib/utils/ZlibUtils.cpp | 2 +- tests/unittests/PalTests.cpp | 11 +++++++++++ 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/compression/HttpDeflateCompression.cpp b/lib/compression/HttpDeflateCompression.cpp index f8e2b1779..679cef080 100644 --- a/lib/compression/HttpDeflateCompression.cpp +++ b/lib/compression/HttpDeflateCompression.cpp @@ -44,7 +44,7 @@ namespace MAT_NS_BEGIN { int result = deflateInit2(&stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, m_windowBits, 8 /*DEF_MEM_LEVEL*/, Z_DEFAULT_STRATEGY); if (result != Z_OK) { - LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 1, result, stream.msg); + LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 1, result, (stream.msg ? stream.msg : "(null)")); compressionFailed(ctx); return false; } @@ -80,7 +80,7 @@ namespace MAT_NS_BEGIN { deflateEnd(&stream); if (result != Z_STREAM_END) { - LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 2, result, stream.msg); + LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 2, result, (stream.msg ? stream.msg : "(null)")); compressionFailed(ctx); return false; } diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 01c6e6f75..2bb4ca1d0 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -369,19 +369,17 @@ namespace PAL_NS_BEGIN { std::transform(uuidStr.begin(), uuidStr.end(), uuidStr.begin(), ::tolower); return uuidStr; #else - static std::once_flag flag; - std::call_once(flag, [](){ - auto now = std::chrono::high_resolution_clock::now(); - auto nanos = std::chrono::duration_cast(now.time_since_epoch()).count(); - std::srand(static_cast(std::time(0) ^ nanos)); - }); + // Use a CSPRNG-backed source (std::random_device reads from /dev/urandom + // on Linux/Android) rather than std::rand()/srand(time(0)), so the session + // and event identifiers built from this are not predictable. + std::random_device rd; GUID_t uuid; - uuid.Data1 = (static_cast(std::rand()) << 16) | static_cast(std::rand()); - uuid.Data2 = static_cast(std::rand()); - uuid.Data3 = static_cast(std::rand()); + uuid.Data1 = static_cast(rd()); + uuid.Data2 = static_cast(rd()); + uuid.Data3 = static_cast(rd()); for (size_t i = 0; i < sizeof(uuid.Data4); i++) - uuid.Data4[i] = static_cast(std::rand()); + uuid.Data4[i] = static_cast(rd()); // TODO: [MG] - replace this sprintf by more robust GUID to string converter char buf[40] = { 0 }; diff --git a/lib/utils/ZlibUtils.cpp b/lib/utils/ZlibUtils.cpp index d091ab3fa..e45ef35f1 100644 --- a/lib/utils/ZlibUtils.cpp +++ b/lib/utils/ZlibUtils.cpp @@ -48,7 +48,7 @@ namespace MAT_NS_BEGIN } while (ret == Z_OK); if (ret != Z_STREAM_END) { - LOG_WARN("Inflate failed, error=%u/%u (%s)", 2, ret, zs.msg); + LOG_WARN("Inflate failed, error=%u/%u (%s)", 2, ret, (zs.msg ? zs.msg : "(null)")); result = false; } inflateEnd(&zs); diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 0bf8a06a3..9a967b228 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -9,6 +9,7 @@ #include #include +#include #ifdef HAVE_MAT_LOGGING #include "pal/PAL.hpp" @@ -68,6 +69,16 @@ TEST_F(PalTests, UuidGeneration) diff += (uuid0[i] != uuid1[i]); } EXPECT_THAT(diff, Gt(20u)); + + // A batch of generated UUIDs must all be distinct (guards against a stuck + // or low-entropy generator). + std::set uuids; + for (size_t i = 0; i < 1000; i++) { + std::string u = PAL::generateUuidString(); + EXPECT_THAT(u.length(), 36u); + uuids.insert(u); + } + EXPECT_THAT(uuids.size(), 1000u); } TEST_F(PalTests, PseudoRandomGenerator) From 9976930357bb0cde5187932b2d4aca701feba3dd Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 16:11:25 -0500 Subject: [PATCH 2/5] Address review comment: avoid reopening entropy source per UUID lib/pal/PAL.cpp (generateUuidString, POSIX/Android path): std::random_device was default-constructed on every call, which reopens the entropy source (/dev/urandom) per UUID on the event-logging hot path. Mark it thread_local so it is opened once per thread and reused; each operator() still draws fresh CSPRNG bytes, so the unpredictability property is unchanged and per-thread isolation keeps it lock-free. Verified the distinctness guard (PalTests.UuidGeneration, 1000 unique UUIDs) still passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/pal/PAL.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 2bb4ca1d0..770056f11 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -372,7 +372,10 @@ namespace PAL_NS_BEGIN { // Use a CSPRNG-backed source (std::random_device reads from /dev/urandom // on Linux/Android) rather than std::rand()/srand(time(0)), so the session // and event identifiers built from this are not predictable. - std::random_device rd; + // thread_local so the entropy source is opened once per thread instead of + // on every call (generateUuidString is on the event logging hot path); + // each operator() call still draws fresh CSPRNG bytes. + thread_local std::random_device rd; GUID_t uuid; uuid.Data1 = static_cast(rd()); From d13c80192efef43c51a0aca9201688454d3596b3 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 16:38:37 -0500 Subject: [PATCH 3/5] Address Copilot round-1 comments zlib error logs (ZlibUtils.cpp:51, HttpDeflateCompression.cpp:47,83): zlib return codes are signed and frequently negative (Z_DATA_ERROR=-3, etc.). Logging them with %u misrepresented the value and was a format/type mismatch; use %d for both the step and the code. PAL.cpp generateUuidString (POSIX/Android): reduce std::random_device reads from 11 to 4 (random_device::max() spans the full unsigned int range, so 4x32 bits fills the 128-bit GUID), cutting per-event-ID entropy-source reads on the hot path. Also soften the comment: random_device is non-deterministic/CSPRNG-backed on our target platforms but the standard does not guarantee the backing source universally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/compression/HttpDeflateCompression.cpp | 4 +-- lib/pal/PAL.cpp | 34 +++++++++++++++------- lib/utils/ZlibUtils.cpp | 2 +- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/lib/compression/HttpDeflateCompression.cpp b/lib/compression/HttpDeflateCompression.cpp index 679cef080..93605ea89 100644 --- a/lib/compression/HttpDeflateCompression.cpp +++ b/lib/compression/HttpDeflateCompression.cpp @@ -44,7 +44,7 @@ namespace MAT_NS_BEGIN { int result = deflateInit2(&stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED, m_windowBits, 8 /*DEF_MEM_LEVEL*/, Z_DEFAULT_STRATEGY); if (result != Z_OK) { - LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 1, result, (stream.msg ? stream.msg : "(null)")); + LOG_WARN("HTTP request compressing failed, error=%d/%d (%s)", 1, result, (stream.msg ? stream.msg : "(null)")); compressionFailed(ctx); return false; } @@ -80,7 +80,7 @@ namespace MAT_NS_BEGIN { deflateEnd(&stream); if (result != Z_STREAM_END) { - LOG_WARN("HTTP request compressing failed, error=%u/%u (%s)", 2, result, (stream.msg ? stream.msg : "(null)")); + LOG_WARN("HTTP request compressing failed, error=%d/%d (%s)", 2, result, (stream.msg ? stream.msg : "(null)")); compressionFailed(ctx); return false; } diff --git a/lib/pal/PAL.cpp b/lib/pal/PAL.cpp index 770056f11..3e667653f 100644 --- a/lib/pal/PAL.cpp +++ b/lib/pal/PAL.cpp @@ -369,20 +369,32 @@ namespace PAL_NS_BEGIN { std::transform(uuidStr.begin(), uuidStr.end(), uuidStr.begin(), ::tolower); return uuidStr; #else - // Use a CSPRNG-backed source (std::random_device reads from /dev/urandom - // on Linux/Android) rather than std::rand()/srand(time(0)), so the session - // and event identifiers built from this are not predictable. - // thread_local so the entropy source is opened once per thread instead of - // on every call (generateUuidString is on the event logging hot path); - // each operator() call still draws fresh CSPRNG bytes. + // Use std::random_device -- a non-deterministic, CSPRNG-backed source on + // the platforms we target (glibc/bionic/libc++ draw from getrandom or + // /dev/urandom) -- instead of std::rand()/srand(time(0)), so the session + // and event identifiers built from it are not predictable. It is + // thread_local so the backing source is opened once per thread rather than + // on every call (generateUuidString is on the event logging hot path), and + // the 128 bits are drawn with four operator() calls instead of eleven + // (random_device::max() is guaranteed to span the full unsigned int range). thread_local std::random_device rd; GUID_t uuid; - uuid.Data1 = static_cast(rd()); - uuid.Data2 = static_cast(rd()); - uuid.Data3 = static_cast(rd()); - for (size_t i = 0; i < sizeof(uuid.Data4); i++) - uuid.Data4[i] = static_cast(rd()); + const uint32_t r0 = rd(); + const uint32_t r1 = rd(); + const uint32_t r2 = rd(); + const uint32_t r3 = rd(); + uuid.Data1 = r0; + uuid.Data2 = static_cast(r1); + uuid.Data3 = static_cast(r1 >> 16); + uuid.Data4[0] = static_cast(r2); + uuid.Data4[1] = static_cast(r2 >> 8); + uuid.Data4[2] = static_cast(r2 >> 16); + uuid.Data4[3] = static_cast(r2 >> 24); + uuid.Data4[4] = static_cast(r3); + uuid.Data4[5] = static_cast(r3 >> 8); + uuid.Data4[6] = static_cast(r3 >> 16); + uuid.Data4[7] = static_cast(r3 >> 24); // TODO: [MG] - replace this sprintf by more robust GUID to string converter char buf[40] = { 0 }; diff --git a/lib/utils/ZlibUtils.cpp b/lib/utils/ZlibUtils.cpp index e45ef35f1..993aad2a8 100644 --- a/lib/utils/ZlibUtils.cpp +++ b/lib/utils/ZlibUtils.cpp @@ -48,7 +48,7 @@ namespace MAT_NS_BEGIN } while (ret == Z_OK); if (ret != Z_STREAM_END) { - LOG_WARN("Inflate failed, error=%u/%u (%s)", 2, ret, (zs.msg ? zs.msg : "(null)")); + LOG_WARN("Inflate failed, error=%d/%d (%s)", 2, ret, (zs.msg ? zs.msg : "(null)")); result = false; } inflateEnd(&zs); From 11555569761ef1d511270b022e12c00c4e8844db Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 1 Jul 2026 01:40:51 -0500 Subject: [PATCH 4/5] Name UUID test magic numbers as constexpr constants Address review feedback on PalTests UuidGeneration: replace the repeated literals 36 (canonical UUID string length) and 1000 (uniqueness-check batch size) with named constexpr constants UuidStringLength and UuidBatchSize so the test's intent is clear at each use site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/PalTests.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 9a967b228..24aaf435c 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -43,9 +43,14 @@ class PalTests : public Test {}; TEST_F(PalTests, UuidGeneration) { + // Canonical UUID string length ("8-4-4-4-12") and the number of UUIDs + // generated for the uniqueness check below. + constexpr size_t UuidStringLength = 36; + constexpr size_t UuidBatchSize = 1000; + std::string uuid0 = PAL::generateUuidString(); - EXPECT_THAT(uuid0.length(), 36u); + EXPECT_THAT(uuid0.length(), UuidStringLength); std::string mask = uuid0; for (char& ch : mask) { @@ -62,10 +67,10 @@ TEST_F(PalTests, UuidGeneration) std::string uuid1 = PAL::generateUuidString(); - EXPECT_THAT(uuid1.length(), 36u); + EXPECT_THAT(uuid1.length(), UuidStringLength); size_t diff = 0; - for (size_t i = 0; i < 36; i++) { + for (size_t i = 0; i < UuidStringLength; i++) { diff += (uuid0[i] != uuid1[i]); } EXPECT_THAT(diff, Gt(20u)); @@ -73,12 +78,12 @@ TEST_F(PalTests, UuidGeneration) // A batch of generated UUIDs must all be distinct (guards against a stuck // or low-entropy generator). std::set uuids; - for (size_t i = 0; i < 1000; i++) { + for (size_t i = 0; i < UuidBatchSize; i++) { std::string u = PAL::generateUuidString(); - EXPECT_THAT(u.length(), 36u); + EXPECT_THAT(u.length(), UuidStringLength); uuids.insert(u); } - EXPECT_THAT(uuids.size(), 1000u); + EXPECT_THAT(uuids.size(), UuidBatchSize); } TEST_F(PalTests, PseudoRandomGenerator) From a775a075e1e2ccbc23eafc5e6aa7158e12c20b74 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Wed, 1 Jul 2026 17:40:58 -0500 Subject: [PATCH 5/5] Unify PalTests local constants on constexpr Convert the remaining size_t const constants (NumQueries, NumBuckets) in the PseudoRandomGenerator test to constexpr so all local constants in PalTests.cpp use the same form. NumBuckets is used as an array bound, so constexpr also documents that it must be a compile-time constant. No behavior or codegen change (const-integral literals already fold to immediates). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/PalTests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unittests/PalTests.cpp b/tests/unittests/PalTests.cpp index 24aaf435c..9f20205fd 100644 --- a/tests/unittests/PalTests.cpp +++ b/tests/unittests/PalTests.cpp @@ -90,8 +90,8 @@ TEST_F(PalTests, PseudoRandomGenerator) { PAL::PseudoRandomGenerator prg; - size_t const NumQueries = 1000; - size_t const NumBuckets = 11; + constexpr size_t NumQueries = 1000; + constexpr size_t NumBuckets = 11; size_t buckets[NumBuckets] = {}; for (size_t i = 0; i < NumQueries; i++) {