From e8aad9ca20736607390e05006c270f230d2adf60 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 22 Jun 2026 11:33:44 -0500 Subject: [PATCH 1/3] 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/3] 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/3] 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);