fix(json_wrapper): fix undefined behavior in copy constructor#1715
Hidden character warning
fix(json_wrapper): fix undefined behavior in copy constructor#1715LHT129 wants to merge 1 commit intoantgroup:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes undefined behavior in the JsonWrapper copy constructor by using a member initializer list. The original implementation accessed uninitialized members, which could lead to crashes or memory corruption. The fix is sound and is accompanied by a good set of unit tests that verify the correctness of the copy operation, including deep copy semantics. I have one suggestion to make the copy constructor slightly more efficient and idiomatic.
src/json_wrapper.cpp
Outdated
| JsonWrapper::JsonWrapper(const JsonWrapper& other) : json_(new nlohmann::json()), owns_json_(true) { | ||
| if (other.json_ != nullptr) { | ||
| *json_ = *other.json_; | ||
| } |
There was a problem hiding this comment.
While the current implementation is correct, it can be made more efficient and concise. You can use the nlohmann::json copy constructor directly in the member initializer list. This avoids creating a default-constructed json object first and then assigning to it, performing the initialization in a single step.
JsonWrapper::JsonWrapper(const JsonWrapper& other)
: json_(other.json_ ? new nlohmann::json(*other.json_) : new nlohmann::json()),
owns_json_(true) {
}There was a problem hiding this comment.
Good suggestion! Updated the code to use ternary operator in the member initializer list. This is more concise and efficient, performing the initialization in a single step. Fixed in commit 44a25df.
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in JsonWrapper’s copy constructor that could delete uninitialized members, and adds regression tests to validate safe, deep-copy behavior.
Changes:
- Reworked
JsonWrappercopy constructor to use a member initializer list and removed invalid pre-initialization cleanup. - Added Catch2 unit tests covering several copy-constructor scenarios (empty, primitives, nested JSON, independence).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/json_wrapper.cpp |
Fixes copy-constructor UB by initializing members before use and removing erroneous deletion of uninitialized pointers. |
tests/test_json_wrapper.cpp |
Adds unit tests to exercise copy behavior across multiple JSON shapes and usage patterns. |
tests/test_json_wrapper.cpp
Outdated
| TEST_CASE("JsonWrapper Copy From Parsed With Exception Disabled", "[ft][json_wrapper]") { | ||
| auto w1 = vsag::JsonWrapper::Parse(R"({"valid": "json"})", false); | ||
| vsag::JsonWrapper w2 = w1; | ||
| CHECK(w2.Contains("valid")); | ||
| CHECK(w2["valid"].GetString() == "json"); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Addressed! I've renamed the test and updated it to test copying a non-owning wrapper obtained from operator[], which is a more meaningful scenario for the copy constructor.
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in vsag::JsonWrapper’s copy constructor (uninitialized member access during construction), reducing risk of crashes/memory corruption in JSON parameter/config handling.
Changes:
- Reworks
JsonWrappercopy constructor to initialize members via an initializer list. - Removes invalid “cleanup” logic from the copy constructor.
- Adds Catch2 functional tests covering several copy-construction scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/json_wrapper.cpp |
Updates JsonWrapper copy constructor implementation to avoid UB and deep-copy the underlying JSON. |
tests/test_json_wrapper.cpp |
Adds functional tests intended to validate copy-constructor behavior and copy independence. |
| JsonWrapper::JsonWrapper(const JsonWrapper& other) | ||
| : json_(other.json_ != nullptr ? new nlohmann::json(*other.json_) : new nlohmann::json()), | ||
| owns_json_(true) { |
There was a problem hiding this comment.
Good suggestion! I've simplified the copy constructor to use a more straightforward approach: initialize json_ to nullptr first, then conditionally allocate memory in the body. This keeps all lines under the 100-column limit and improves readability.
tests/test_json_wrapper.cpp
Outdated
| TEST_CASE("JsonWrapper Copy From Parsed With Exception Disabled", "[ft][json_wrapper]") { | ||
| auto w1 = vsag::JsonWrapper::Parse(R"({"valid": "json"})", false); | ||
| vsag::JsonWrapper w2 = w1; |
There was a problem hiding this comment.
Good point! I've renamed the test to "JsonWrapper Copy Non-owning Wrapper" and changed it to test copying a wrapper obtained from operator[] (which returns a non-owning wrapper). This provides more meaningful test coverage for the copy constructor with different ownership scenarios.
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
0cb28dc to
daf151b
Compare
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
daf151b to
6b71791
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses undefined behavior in vsag::JsonWrapper’s copy constructor to prevent crashes/memory corruption, and adds functional tests around copy behavior. It also includes refactors to backward-compatibility tooling (script + GitHub Actions workflow) that affect how compatibility checks and index artifacts are handled.
Changes:
- Fix
JsonWrappercopy constructor to avoid reading/deleting uninitialized members by properly initializing and allocatingjson_. - Add Catch2 functional tests covering copy-construction scenarios (including copying a non-owning wrapper).
- Refactor compatibility check script and the “generate old version index” workflow upload logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/json_wrapper.cpp |
Reworks copy constructor to eliminate UB and deep-copy JSON state safely. |
tests/test_json_wrapper.cpp |
Adds functional tests validating copy constructor behavior across several JSON shapes/usages. |
scripts/check_compatibility.sh |
Refactors iteration over old versions and simplifies output/flow for compatibility checks. |
.github/workflows/generate_old_version_index.yml |
Simplifies release upload/create logic for compatibility index artifacts. |
tests/test_json_wrapper.cpp
Outdated
| auto w1 = vsag::JsonWrapper::Parse(R"({"valid": "json"})"); | ||
| vsag::JsonWrapper sub = w1["valid"]; | ||
| vsag::JsonWrapper w2 = sub; |
scripts/check_compatibility.sh
Outdated
| #!/usr/bin/bash | ||
|
|
||
| old_version_indexes="v0.17.2_hgraph v0.17.2_hnsw \ | ||
| v0.16.14_hgraph v0.16.14_hnsw \ | ||
| v0.15.1_hgraph v0.15.1_hnsw \ | ||
| v0.14.8_hgraph v0.14.8_hnsw \ | ||
| v0.13.4_hgraph v0.13.4_hnsw \ | ||
| v0.13.0_hnsw \ | ||
| v0.12.0_hnsw \ | ||
| v0.11.14_hnsw \ | ||
| v0.10.0_hnsw" |
scripts/check_compatibility.sh
Outdated
| "v0.11.14_hnsw" | ||
| "v0.10.0_hnsw" | ||
| ) | ||
| #!/usr/bin/bash |
scripts/check_compatibility.sh
Outdated
| ./build-release/tools/check_compatibility/check_compatibility ${version} | ||
| if [ $? -ne 0 ]; then |
| gh release upload compatibility-indexes $files --repo antgroup/vsag --clobber || \ | ||
| gh release create compatibility-indexes $files \ | ||
| --repo antgroup/vsag \ | ||
| --title "VSAG Compatibility Test Indexes" \ | ||
| --notes "Index files for backward compatibility testing" |
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
6b71791 to
d45b470
Compare
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
d45b470 to
d22a1ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes undefined behavior in vsag::JsonWrapper’s copy constructor by removing pre-initialization cleanup and ensuring members are initialized before use, addressing crash/memory-corruption risk in JSON-heavy configuration paths.
Changes:
- Reworked
JsonWrappercopy constructor to avoid reading/deleting uninitialized members and to perform a safe deep copy. - Added functional tests covering multiple copy-constructor scenarios (empty, primitives, nested JSON, non-owning sub-wrapper).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/json_wrapper.cpp |
Fixes copy-constructor UB by initializing members and allocating/copying JSON safely. |
tests/test_json_wrapper.cpp |
Adds tests intended to cover copy-constructor behavior across common and edge ownership cases. |
| TEST_CASE("JsonWrapper Copy Non-owning Wrapper", "[ft][json_wrapper]") { | ||
| auto w1 = vsag::JsonWrapper::Parse(R"({"valid": "json"})"); | ||
| vsag::JsonWrapper sub = w1["valid"]; | ||
| vsag::JsonWrapper w2 = sub; | ||
| CHECK(w2.GetString() == "json"); | ||
| } |
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
65c1bd1 to
efb583b
Compare
Review ResponseI've addressed the key feedback:
The other Gemini/Copilot suggestions are optional improvements that don't affect correctness. |
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in JsonWrapper’s copy constructor by removing cleanup of uninitialized members and changing the constructor to allocate/copy JSON safely; adds unit tests to cover copy scenarios.
Changes:
- Reworked
JsonWrappercopy constructor to avoid deleting uninitialized members and to allocate/copynlohmann::json. - Added Catch2 tests covering copying empty, primitive, nested, and non-owning wrappers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/json_wrapper.cpp | Refactors copy ctor allocation/copy logic to avoid UB in member cleanup. |
| tests/test_json_wrapper.cpp | Adds unit tests validating copy behavior and independence. |
src/json_wrapper.cpp
Outdated
| JsonWrapper::JsonWrapper(const JsonWrapper& other) : owns_json_(true) { | ||
| if (other.json_ != nullptr) { | ||
| *json_ = *other.json_; | ||
| json_ = new nlohmann::json(*other.json_); | ||
| } else { | ||
| json_ = new nlohmann::json(); | ||
| } | ||
| } |
src/json_wrapper.cpp
Outdated
| JsonWrapper::JsonWrapper(const JsonWrapper& other) : owns_json_(true) { | ||
| if (other.json_ != nullptr) { | ||
| *json_ = *other.json_; | ||
| json_ = new nlohmann::json(*other.json_); | ||
| } else { | ||
| json_ = new nlohmann::json(); | ||
| } | ||
| } |
tests/test_json_wrapper.cpp
Outdated
| std::string copied_value; | ||
| { | ||
| auto w1 = vsag::JsonWrapper::Parse(R"({"key": "value"})"); | ||
| vsag::JsonWrapper sub = w1["key"]; | ||
| vsag::JsonWrapper w2 = sub; | ||
| copied_value = w2.GetString(); | ||
| } | ||
| CHECK(copied_value == "value"); | ||
| } |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include <catch2/catch_test_macros.hpp> |
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
efb583b to
7ffbb69
Compare
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
7ffbb69 to
70bc6d2
Compare
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in JsonWrapper’s copy constructor that could lead to crashes/memory corruption by ensuring members are initialized correctly before use, and adds tests for copy scenarios.
Changes:
- Replaced copy-constructor body with a member-initializer-list implementation (eliminates use of uninitialized members).
- Removed invalid cleanup logic from the copy constructor.
- Added Catch2 unit tests covering several copy scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_json_wrapper.cpp | Adds unit tests intended to validate copy behavior (including nested and non-owning cases). |
| src/json_wrapper.cpp | Rewrites copy constructor to allocate/initialize json_ safely and deep-copy from the source. |
| vsag::JsonWrapper w2 = vsag::JsonWrapper(); | ||
| { | ||
| auto w1 = vsag::JsonWrapper::Parse(R"({"key": "value"})"); | ||
| vsag::JsonWrapper sub = w1["key"]; | ||
| w2 = sub; | ||
| } |
There was a problem hiding this comment.
The test name/intent suggests validating the copy constructor from a non-owning wrapper, but this code primarily exercises copy assignment (w2 = sub) and sub may be constructed via move/copy elision from w1["key"]. To ensure the copy constructor is actually tested, construct w2 via copy construction from an lvalue sub (e.g., declare sub in-scope, then vsag::JsonWrapper w2(sub); outside the inner scope), and avoid vsag::JsonWrapper w2 = vsag::JsonWrapper(); in favor of a plain default construction.
| vsag::JsonWrapper w2 = vsag::JsonWrapper(); | |
| { | |
| auto w1 = vsag::JsonWrapper::Parse(R"({"key": "value"})"); | |
| vsag::JsonWrapper sub = w1["key"]; | |
| w2 = sub; | |
| } | |
| vsag::JsonWrapper sub; | |
| { | |
| auto w1 = vsag::JsonWrapper::Parse(R"({"key": "value"})"); | |
| sub = w1["key"]; | |
| } | |
| vsag::JsonWrapper w2(sub); |
src/json_wrapper.cpp
Outdated
| : json_(other.json_ ? new nlohmann::json(*other.json_) : new nlohmann::json()), | ||
| owns_json_(true) { |
There was a problem hiding this comment.
The conditional allocation duplicates the new nlohmann::json(...) expression, which makes the initializer harder to read/modify. Consider allocating once (e.g., initialize json_ to a fresh nlohmann::json and conditionally copy into it in the constructor body), which keeps the ownership semantics the same while reducing duplication.
| : json_(other.json_ ? new nlohmann::json(*other.json_) : new nlohmann::json()), | |
| owns_json_(true) { | |
| : json_(new nlohmann::json()), | |
| owns_json_(true) { | |
| if (other.json_ != nullptr) { | |
| *json_ = *other.json_; | |
| } |
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
70bc6d2 to
b20a3f7
Compare
Fix UB in JsonWrapper copy constructor where uninitialized members were checked before initialization. Use member initializer list and explicit nullptr comparison. Fixes antgroup#1714 Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
LHT129
force-pushed
the
2026-03-13-修复-jsonwrapper-拷贝构造函数-ub
branch
from
b20a3f7 to
19a1789
Compare
Summary
Fix undefined behavior in
JsonWrappercopy constructor that could cause random crashes or memory corruption.Changes
if (owns_json_) { delete json_; }) from copy constructorProblem
The copy constructor incorrectly checked
owns_json_and deletedjson_before they were initialized:Solution
Use member initializer list to properly initialize members:
Testing
Added 6 unit tests covering:
All tests pass.
Related Issues