Skip to content

Conversation

@DanielHwangbo
Copy link

@DanielHwangbo DanielHwangbo commented Nov 30, 2025

Description

This PR fixes a crash when using update() on an ordered_json object when JSON_DIAGNOSTICS is enabled.

The Issue

When update() inserts new elements into an ordered_json object, the underlying std::vector may reallocate. This invalidates the m_parent pointers of existing children, causing assert_invariant to fail immediately because the children point to the old memory address.

The Fix

I have updated the update() function to call set_parents() after insertion operations when diagnostics are enabled. This ensures that if the container moves in memory, all children are correctly updated to point to the new parent address.

Reproduction Code

#define JSON_DIAGNOSTICS 1
#include "nlohmann/json.hpp"
#include <iostream>

int main() {
    using json = nlohmann::ordered_json;
    json j1 = { {"numbers", {{"one", 1}}} };
    json j2 = { {"numbers", {{"two", 2}}}, {"string", "t"} };

    // This triggers reallocation and assertion failure without the fix
    j1.update(j2, true); 
    std::cout << j1.dump() << std::endl;
}

@coveralls
Copy link

coveralls commented Nov 30, 2025

Coverage Status

coverage: 99.176% (-0.02%) from 99.191%
when pulling f7e256e on DanielHwangbo:issue4813
into a0e9fb1 on nlohmann:develop.

@github-actions
Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @DanielHwangbo
Please read and follow the Contribution Guidelines.

@nlohmann
Copy link
Owner

Maybe related: #4813

Signed-off-by: Daniel Hwangbo <[email protected]>
@github-actions github-actions bot added L and removed M labels Dec 7, 2025
@DanielHwangbo
Copy link
Author

Hi nlohmann,

I have fixed the Amalgamation error and would like to request a code review again!

@github-actions
Copy link

github-actions bot commented Dec 8, 2025

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @DanielHwangbo
Please read and follow the Contribution Guidelines.

Copy link
Contributor

@gregmarr gregmarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a LOT of unrelated formatting changes in this file. Did your editor do an auto-format?

m_data.m_value.object->operator[](it.key()) = it.value();

// Insert/Overwrite logic
auto& ref = m_data.m_value.object->operator[](it.key());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

Copy link
Contributor

@gregmarr gregmarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend reverting all of the whitespace changes and the changes at line 3491 to introduce ref. Then you also need to amalgamate and add the file to the commit. I don't know if it didn't work before, or if you didn't add the file, but there are no changes to the amalgamated header.

{
it2->second.update(it.value(), true);

// Repair the child's subtree.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment should go inside the #if.

auto& ref = m_data.m_value.object->operator[](it.key());
ref = it.value();

// We must traverse the entire tree of 'this' and reset everyone's parent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment should go inside the #if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants