Low-severity hardening: CSPRNG UUIDs (POSIX) and null-safe zlib error logs#1493
Open
bmehta001 wants to merge 4 commits into
Open
Low-severity hardening: CSPRNG UUIDs (POSIX) and null-safe zlib error logs#1493bmehta001 wants to merge 4 commits into
bmehta001 wants to merge 4 commits into
Conversation
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>
lalitb
reviewed
Jun 22, 2026
| // 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; |
Contributor
There was a problem hiding this comment.
Since generateUuidString() is on the event logging path, do we have any perf data for constructing std::random_device on every UUID, especially on Linux/Android? The security improvement makes sense; I just want to make sure we are not adding a noticeable hot-path cost.
Contributor
Author
There was a problem hiding this comment.
The old approach took ~115 ns, and this approach takes ~2.0 µs. I think generateUuidString() should be called once or twice per event.
std::random_device is thread_local , so it should only be constructed once per thread. If this is too much of a cost (17x slower), then I can close this PR.
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens telemetry identifier generation on non-Windows/non-Apple platforms by replacing predictable std::rand()-based UUID generation with std::random_device, and it makes zlib error logging null-safe when z_stream::msg is Z_NULL.
Changes:
- Update POSIX/Android UUID generation to draw bytes from
std::random_deviceinstead ofstd::rand()/time-based seeding. - Add a unit test that generates 1000 UUIDs and asserts they are all distinct.
- Guard zlib error log formatting so
%snever receives a null pointer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unittests/PalTests.cpp | Adds a 1000-UUID uniqueness test to catch stuck/low-entropy UUID generation. |
| lib/pal/PAL.cpp | Replaces std::rand() UUID byte sourcing with std::random_device for non-Windows/non-Apple builds. |
| lib/utils/ZlibUtils.cpp | Makes inflate error logging null-safe for zs.msg. |
| lib/compression/HttpDeflateCompression.cpp | Makes deflate error logging null-safe for stream.msg. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two low-severity hardening fixes from a repo-wide review.
1. POSIX/Android UUID generation used
std::rand()(predictable)PAL::generateUuidString()'s non-Windows/non-Apple fallback built the entire 128-bit UUID fromstd::rand(), seeded once withsrand(time(0) ^ nanos).std::rand()is a weak, predictable PRNG and a time-based seed is guessable, so identifiers derived from it (m_sessionId,SESSION_ID_LEGACY,sessionSDKUid, SignalsInstanceId, per-event ids) were predictable.These are correlation identifiers (not auth material — the upload credential is the tenant token), hence Low. Fix: source the bytes from
std::random_device(backed by/dev/urandomon Linux/Android), matching the existingCorrelationVector.cpp/PseudoRandomGeneratorusage. The Windows (CoCreateGuid) and Apple (CFUUIDCreate) paths are unchanged.Test:
PalTests.UuidGenerationnow also asserts a batch of 1000 generated UUIDs are all distinct (on top of the existing length / format / entropy checks).2. zlib error logs pass a possibly-NULL
msgto%sZlibUtils.cpp:51andHttpDeflateCompression.cpp:47,83loggedzs.msg/stream.msgvia%s. zlib leavesmsg == Z_NULLfor several error codes (Z_MEM_ERROR,Z_BUF_ERROR, after a faileddeflateInit2), andprintf("%s", NULL)is undefined behavior — benign(null)on glibc but not guaranteed across the MSVC/Android/Apple CRTs this SDK targets. Guard withmsg ? msg : "(null)".Validation
Built and ran
UnitTestson Linux (host):PalTests(incl. the new UUID-uniqueness check) andZlibUtilsTestspass.