fix: make the JSONCPP_USE_SECURE_MEMORY build compile and pass - #1709
Conversation
Building with JSONCPP_USE_SECURE_MEMORY=1 fails in three separate ways, each hidden behind the previous one. No CI job builds this configuration, which is why they have accumulated. 1. allocator.h calls RtlSecureZeroMemory on the _WIN32 branch without including <windows.h>, so MSVC fails with error C3861. Removing that branch lets the portable volatile std::fill_n path handle Windows. The same fill was zeroing n bytes rather than n * sizeof(T), so for any T wider than a char, most of the allocation was never wiped. 2. jsontestrunner declares a std::string where the surrounding code uses Json::String. The two are the same type only in default builds, so the test runner does not compile under secure memory. 3. CZString's move-assignment released a key with releasePrefixedStringValue, but CZString keys come from duplicateStringValue and carry no length prefix. The destructor already uses the matching releaseStringValue. Under the default allocator this mismatch is survivable; under SecureAllocator the suite segfaults. Verified on MSVC 19.44, x64, C++17. Before: error C3861. With only the first two fixed: builds, then jsoncpp_test SEGFAULT. With all three: secure build compiles and 3/3 ctest suites pass. The default build is unaffected and also 3/3. Fixes open-source-parsers#1399
Filed JC-1 and JC-7 together with the CZString move-assignment fix as open-source-parsers/jsoncpp#1709, 4 files +32/-31, closing #1399 which has been open since March 2022 reporting both the portability problem and that the unit tests will not build. Two findings from preparing the PR, both recorded in the receipt. The CZString hunk looked like scope creep; reverting it proved the secure build compiles and then segfaults, so it is load-bearing and the layered failure is now the PR's strongest argument. The under-wipe claim was cut back: Json::String is SecureAllocator<char> so sizeof(T) is 1 and it is unreachable there, latent only for wider T via Json::Allocator<T>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015iiwS6T2sCVr6b1hEztqww
|
| Filename | Overview |
|---|---|
| include/json/allocator.h | Replaces the undeclared Windows-only zeroing call with the portable volatile fill path and corrects the wipe length for arbitrary allocator element sizes. |
| src/jsontestrunner/main.cpp | Uses Json::String for filename extraction so the runner remains allocator-compatible in secure-memory builds. |
| src/lib_json/json_value.cpp | Corrects CZString move-assignment cleanup to use the deallocator matching its non-prefixed key allocation. |
| src/test_lib_json/main.cpp | Converts affected test data, streams, helpers, and containers to allocator-aware JsonCpp string aliases and adds CZString move coverage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Secure-memory build enabled] --> B[Json::String uses SecureAllocator]
B --> C[Allocator deallocation]
C --> D[Volatile wipe of n times sizeof T bytes]
D --> E[Global delete]
B --> F[CZString owning key]
F --> G[Move assignment releases prior key]
G --> H[releaseStringValue with stored length]
H --> I[Take source key ownership]
Reviews (1): Last reviewed commit: "Merge branch 'master' into fix-secure-me..." | Re-trigger Greptile
Coverage Report for CI Build 32423582949Coverage remained the same at 89.907%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
|
We probably want to add a GitHub Action for this build configure to ensure that it does not regress. Added an issue: |
JSONCPP_USE_SECURE_MEMORY=1does not build. It fails in three separate ways, each hidden behind the previous one, so fixing only the first gets you to the second. No CI job builds this configuration, which is how they accumulated.Reproduce on master (
60de77f), MSVC 19.44 x64:1. The
_WIN32branch callsRtlSecureZeroMemorywithout including<windows.h>. Removing that branch lets the portablevolatile std::fill_npath handle Windows too. The same fill was zeroingnbytes rather thann * sizeof(T);Json::StringisSecureAllocator<char>so it is unaffected, but any widerTthrough the publicJson::Allocator<T>alias was leaving most of the allocation un-wiped.2.
jsontestrunnerdeclares astd::stringwhere the surrounding code usesJson::String. Identical types in default builds, different under secure memory, so the runner does not compile. One line.3.
CZString's move-assignment releases a key withreleasePrefixedStringValue, butCZStringkeys come fromduplicateStringValueand carry no length prefix. The destructor already uses the matchingreleaseStringValue. Under the default allocator the mismatch is survivable; underSecureAllocatorthe suite segfaults. This is the one that only appears after the first two are fixed.Verification. Before:
error C3861. With only 1 and 2 fixed: builds, thenjsoncpp_test (SEGFAULT). With all three: secure build compiles and 3/3 ctest suites pass. The default build is unaffected and also 3/3.Fixes #1399, which reported both the portability problem and that "the unit tests won't build".