Repository navigation
fix double-free in CZString copy-assignment operator - #1706
SABITHSAHEB wants to merge 3 commits into
Conversation
|
any update? |
|
| Filename | Overview |
|---|---|
| src/lib_json/json_value.cpp | Reworks CZString copy assignment to use copy-and-swap, but the previously reported inactive-union-member access remains in the invoked swap implementation. |
| src/test_lib_json/main.cpp | Adds focused regression coverage confirming that both source and destination retain the copied string after assignment. |
Reviews (2): Last reviewed commit: "Merge branch 'master' into czstring-copy..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Coverage Report for CI Build 32808338607Coverage increased (+0.1%) to 90.022%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
دايما توصلني هالايميلات وواكثر من مستخدم على ايكلاودي ممكن الحل بدلت
الباسورد مافي فايده
…On Wed, Aug 26, 2026 at 8:43 PM greptile-apps[bot] ***@***.***> wrote:
***@***.***[bot]* commented on this pull request.
------------------------------
In src/lib_json/json_value.cpp
<#1706 (comment)>
:
> @@ -317,8 +317,11 @@ void Value::CZString::swap(CZString& other) {
}
Value::CZString& Value::CZString::operator=(const CZString& other) {
- cstr_ = other.cstr_;
- index_ = other.index_;
+ // Copy-and-swap so a duplicate-policy buffer is deep-copied and the old one
+ // released once. The prior shallow copy aliased other.cstr_, double-freeing
+ // an owned string and leaking the overwritten one.
+ CZString temp(other);
+ swap(temp);
You're right. swap() is pre-existing, and CZString already treats the
ArrayIndex/StringStorage union as a word-sized representation; the
existing destructor and copy constructor follow the same convention. Given
that invariant, swap(temp) at src/lib_json/json_value.cpp:320 preserves
the packed length/policy metadata and does not introduce a new issue in
this PR. The ASan/UBSan results and regression coverage also address the
actual ownership bug. I'll withdraw the inactive-union-member finding; no
further change is needed here.
Tip: You can customize Greptile's behavior for this repo with
.greptile/rules.md and .greptile/config.json
<https://www.greptile.com/docs/code-review/custom-standards>.
—
Reply to this email directly, view it on GitHub
<#1706?email_source=notifications&email_token=AIBOSVFC2QUFDMKDTFWFL3L5L4HSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBTGI4DOOJQGE42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r3864813651>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIBOSVAPIFGO4IC3J35GGWD5L4HSHAVCNFSNUABEKJSXA33TNF2G64TZHMZDCNJTGA4DMMJ3JFZXG5LFHM2DQMZYGQ3DAMRXGOQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AIBOSVED53ZDUSAYS3IZKHD5L4HSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBTGI4DOOJQGE42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/AIBOSVA645OX6N3TLI4IZTD5L4HSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBTGI4DOOJQGE42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
The Greptile summary at the top still reads "not yet safe to merge" over the inactive-union-member finding, but that finding was withdrawn in the inline thread, so nothing is outstanding from the bot review. Re-verified on the current tip, which now carries master through 3347a4b: drop the operator= hunk and the new case still trips ASan with a double free in ~CZString at json_value.cpp:309; keep it and the full suite is 131/131 clean under ASan and UBSan. Worth flagging the history since it is the obvious objection here: #321 tried |
This branch has not been deployed
4 participants
CZString is the key type behind Json::Value's object and array maps, and a key that owns its buffer (the duplicate policy) frees that buffer in its destructor. The copy-assignment operator only shallow-copied cstr_ and the packed index_/policy word, so after
a = bboth keys pointed at b's allocation while a's old buffer was never released. On scope exit both destructors free the same pointer, which AddressSanitizer reports as a double free, and the overwritten buffer leaks. I ran into it while filling out the CZString assignment coverage from #1654, which exercised the copy and move constructors and the move-assignment but not this operator. The move-assignment already releases then transfers correctly, so I made copy-assignment do the safe thing with copy-and-swap: the temporary deep-copies the source and takes our old buffer to be freed exactly once. Added the missing case to runCZStringTests, which double-frees under ASan before the change and passes after.