Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON_DIAGNOSTICS assertion for ordered_json #2962

Closed
iwubcode opened this issue Aug 20, 2021 · 7 comments · Fixed by #2963
Closed

JSON_DIAGNOSTICS assertion for ordered_json #2962

iwubcode opened this issue Aug 20, 2021 · 7 comments · Fixed by #2963
Assignees
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@iwubcode
Copy link

iwubcode commented Aug 20, 2021

What is the issue you have?

Assertion occurs with ordered_json and JSON_DIAGNOSTICS when none is expected

Please describe the steps to reproduce the issue.

Run the working example

Can you provide a small but working code example?

#define JSON_DIAGNOSTICS 1
#include "nlohmann/json.hpp"
using json = nlohmann::ordered_json;

#include <iostream>

int main()
{
  json j;
  json j2;
  const std::string value = "";
  j["first"] = value;
  j["second"] = value;
  j2["something"] = j;
  std::cout << j2 << std::endl;
  return 0;
}

What is the expected behavior?

Assertion should not occur

And what is the actual behavior instead?

Assertion occurs

Which compiler and operating system are you using?

Tested on Visual Studio 2019 latest release. Also tested on godbolt with gcc 9.3, 11.2..

Which version of the library did you use?

3.10.0

@iwubcode
Copy link
Author

iwubcode commented Aug 20, 2021

From what I can tell, the parent pointer for the first element changes when the second element gets added to the ordered "map" container. I'm not familiar with the json code but my wild guess is pointer invalidation due to using a std::vector as the underlying container type.

As an aside, thank you for this wonderful library!

@nlohmann
Copy link
Owner

I can reproduce the issue. Interestingly, it does not occur if nlohmann::json is used, but only for nlohmann::ordered_json.

@nlohmann
Copy link
Owner

From what I can tell, the parent pointer for the first element changes when the second element gets added to the ordered "map" container. I'm not familiar with the json code but my wild guess is pointer invalidation due to using a std::vector as the underlying container type.

You are right, thanks for the analysis! In #2838 we fixed the library to re-evaluate the parents in case the vector for an array changed its capacity and pointers are invalidated. We forgot that the same effect can occur inside ordered_map. Unfortunately, there is no capacity function, so we have to find a different way to fix this.

@nlohmann
Copy link
Owner

That would work, but would be a breaking change in the ordered_map class. I basically need to adjust this if inside the set_parent function to also execute set_parents() in case the type is ordered_json:

if (old_capacity != std::size_t(-1))
{
    // see https://github.com/nlohmann/json/issues/2838
    JSON_ASSERT(type() == value_t::array);
    if (JSON_HEDLEY_UNLIKELY(m_value.array->capacity() != old_capacity))
    {
        // capacity has changed: update all parents
        set_parents();
        return j;
    }
}

@nlohmann
Copy link
Owner

I found a fix, see #2963. Feedback welcome!

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Aug 20, 2021
@nlohmann nlohmann self-assigned this Aug 20, 2021
@nlohmann nlohmann added this to the Release 3.10.1 milestone Aug 20, 2021
@asmaloney
Copy link

@nlohmann #2963 fixed it for me!

Thank you!

@nlohmann
Copy link
Owner

Cool! I still have to fix some linter warnings, but the fix should be merged by Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants