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

Assertion failure when inserting into arrays with JSON_DIAGNOSTICS set #2838

Closed
2 of 5 tasks
carlsmedstad opened this issue Jun 23, 2021 · 6 comments · Fixed by #2866
Closed
2 of 5 tasks

Assertion failure when inserting into arrays with JSON_DIAGNOSTICS set #2838

carlsmedstad opened this issue Jun 23, 2021 · 6 comments · Fixed by #2866
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

@carlsmedstad
Copy link
Contributor

What is the issue you have?

The assertion checking parent correctness in assert_invariants() fails for arrays when using JSON_DIAGNOSTICS. It seems parents of objects being inserted into arrays are not being updated.

Please describe the steps to reproduce the issue.

To reproduce compile and run the following:

#define JSON_DIAGNOSTICS 1

#include <nlohmann/json.hpp>

using json = nlohmann::json;

int main()
{
    json j_arr = json::array();
    j_arr.push_back(json::object());
    j_arr.push_back(json::object());
    json j_obj = json::object();
    j_obj["key"] = j_arr;
}

This produces the following assertion failure:

assert_failure: single_include/nlohmann/json.hpp:18032: void nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>::assert_invariant(bool) const [with ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer; BinaryType = std::vector<unsigned char>]: Assertion `!check_parents || !is_structured() || std::all_of(begin(), end(), [this](const basic_json & j) { return j.m_parent == this; })' failed.
Aborted (core dumped)

Note that this does not happen when there is only one push_back, i.e. only a single element in the array.

This seems to have been the case since JSON_DIAGNOSTICS was introduced in 176d8e2.

Can you provide a small but working code example?

Disabling JSON_DIAGNOSTICS or commenting out the push_back lines will make this code run.

What is the expected behavior?

Assertion failure due to incorrect parents.

And what is the actual behavior instead?

No assert should fail and parents should be correct.

Which compiler and operating system are you using?

  • Compiler: GCC 11.1.0 and Clang 12.0.0
  • Operating system: Linux

Which version of the library did you use?

  • latest release version 3.9.1
  • other release - please state the version: ___
  • the develop branch

If you experience a compilation error: can you compile and run the unit tests?

  • yes
  • no - please copy/paste the error message below
@nlohmann
Copy link
Owner

Oh dear, thanks for reporting! I can reproduce the issue and will try to find the missing parent update.

@gregmarr
Copy link
Contributor

@nlohmann I'm thinking that the m_value.array->push_back() is resizing the vector and that after the resize the objects in the resized vector don't have their parent set. I remember that at one point we had the move and copy constructors copying the parent, but that wasn't correct because it could be happening for other reasons than an array resize. Maybe push_back(basic_json...) needs to detect when m_value.array->capacity() is changed and call set_parents() instead of just set_parent(...back()).

@nlohmann
Copy link
Owner

@gregmarr Great analysis! I added a quick fix and the assertion does not occur any more.

Before:

        m_value.array->push_back(std::move(val));
        set_parent(m_value.array->back());

After:

        auto capacity = m_value.array->capacity();
        m_value.array->push_back(std::move(val));
        if (capacity == m_value.array->capacity())
        {
            set_parent(m_value.array->back());
        }
        else
        {
            for (auto& element : *m_value.array)
            {
                set_parent(element);
            }
        }

I'll need some more time to prepare a PR with proper tests.

@nlohmann nlohmann self-assigned this Jun 26, 2021
@gregmarr
Copy link
Contributor

        for (auto& element : *m_value.array)
        {
            set_parent(element);
        }

This is the same as set_parents();, right?

@nlohmann
Copy link
Owner

I fixed the issue and also found some more member functions that could have triggered the assertion, see #2866. Feedback welcome!

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jul 12, 2021
@nlohmann nlohmann added this to the Release 3.9.2 milestone Jul 14, 2021
harry75369 pushed a commit to harry75369/json that referenced this issue Aug 8, 2021
@MHebes
Copy link

MHebes commented Aug 11, 2021

I fixed the issue and also found some more member functions that could have triggered the assertion, see #2866. Feedback welcome!

@nlohmann Do you also need to track capacity in the external construct functions, e.g.

    template < typename BasicJsonType, typename CompatibleArrayType,
               enable_if_t < !std::is_same<CompatibleArrayType, typename BasicJsonType::array_t>::value,
                             int > = 0 >
    static void construct(BasicJsonType& j, const CompatibleArrayType& arr)

?

I'm getting a very hard to reproduce write access violation with JSON_DIAGNOSTICS 1 when implicitly serializing an std::array of serializable structs. Can't get a minimal example to throw it though, not sure why. It seems related to this bug though.

Thrown here:

image

One stack up:

image

Edit: It's actually an array of std::optionals with a custom serializer per https://nlohmann.github.io/json/features/arbitrary_types/#how-do-i-convert-third-party-types. This works fine with JSON_DIAGNOSTICS 0.

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.

4 participants