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

src/tree_data_sorted.c:1160: LY_ERR lyds_create_metadata(struct lyd_node *, struct lyd_meta **): Assertion `leader && (!leader->prev->next || (leader->schema != leader->prev->schema))' failed. #2223

Closed
jktjkt opened this issue Apr 10, 2024 · 8 comments

Comments

@jktjkt
Copy link
Contributor

jktjkt commented Apr 10, 2024

velia's test suite started triggering an internal assert in libyang. This has worked prior that migration to the v2.2 of libyang., so it's a regression somewhere along the way, and I cannot bisect that due to API changes. Here's how to reproduce:

The backtrace looks like:

    #10 lyds_insert /home/jkt/work/cesnet/gerrit/github/CESNET/libyang/src/tree_data_sorted.c:1235:9 (libyang.so.3+0x8c3e6)
    #11 lyd_insert_node /home/jkt/work/cesnet/gerrit/github/CESNET/libyang/src/tree_data.c:764:15 (libyang.so.3+0x2e768)
    #12 lyd_insert_child /home/jkt/work/cesnet/gerrit/github/CESNET/libyang/src/tree_data.c:1046:9 (libyang.so.3+0x2ecea)
    #13 sr_lyd_insert_child /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/ly_wrap.c:1117:9 (libsysrepo.so.7+0x3445f)
    #14 sr_edit_diff_edit_merge_r /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/edit_diff.c:2933:24 (libsysrepo.so.7+0x4d404)
    #15 sr_edit_diff_edit_merge_r /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/edit_diff.c:2916:29 (libsysrepo.so.7+0x4d346)
    #16 sr_edit_mod_diff_edit_merge /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/edit_diff.c:2959:25 (libsysrepo.so.7+0x4d23d)
    #17 sr_modinfo_edit_merge /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/modinfo.c:611:44 (libsysrepo.so.7+0x407a8)
    #18 sr_apply_changes /home/jkt/work/cesnet/gerrit/github/sysrepo/sysrepo/src/sysrepo.c:3935:20 (libsysrepo.so.7+0x16d0b)
    #19 sysrepo::Session::applyChanges(std::chrono::duration<long, std::ratio<1l, 1000l>>) /home/jkt/work/cesnet/gerrit/CzechLight/sysrepo-cpp/src/Session.cpp:256:16 (libsysrepo-cpp.so+0xea64)
    #20 velia::utils::valuesPush(std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, sysrepo::Session) /home/jkt/work/cesnet/gerrit/CzechLight/velia/src/utils/sysrepo.cpp:134:17 (test-sysrepo_ietf-hardware+0x1c6760)
    #21 velia::utils::valuesPush(std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>> const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&, sysrepo::Session, sysrepo::Datastore) /home/jkt/work/cesnet/gerrit/CzechLight/velia/src/utils/sysrepo.cpp:121:5 (test-sysrepo_ietf-hardware+0x1c61de)
    #22 velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0::operator()() const /home/jkt/work/cesnet/gerrit/CzechLight/velia/src/ietf-hardware/sysrepo/Sysrepo.cpp:143:13 (test-sysrepo_ietf-hardware+0x15c1c8)
    #23 void std::__invoke_impl<void, velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>(std::__invoke_other, velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0&&) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/invoke.h:61:14 (test-sysrepo_ietf-hardware+0x15a87d)
    #24 std::__invoke_result<velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>::type std::__invoke<velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>(velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0&&) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/invoke.h:96:14 (test-sysrepo_ietf-hardware+0x15a87d)
    #25 void std::thread::_Invoker<std::tuple<velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>>::_M_invoke<0ul>(std::_Index_tuple<0ul>) /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_thread.h:292:13 (test-sysrepo_ietf-hardware+0x15a87d)
    #26 std::thread::_Invoker<std::tuple<velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>>::operator()() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_thread.h:299:11 (test-sysrepo_ietf-hardware+0x15a87d)
    #27 std::thread::_State_impl<std::thread::_Invoker<std::tuple<velia::ietf_hardware::sysrepo::Sysrepo::Sysrepo(sysrepo::Session, std::shared_ptr<velia::ietf_hardware::IETFHardware>, std::chrono::duration<long, std::ratio<1l, 1000000l>>)::$_0>>>::_M_run() /nix/store/j00nb8s5mwaxgi77h21i1ycb91yxxqck-gcc-13.2.0/include/c++/13.2.0/bits/std_thread.h:244:13 (test-sysrepo_ietf-hardware+0x15a87d)
    #28 execute_native_thread_routine <null> (libstdc++.so.6+0xe8682)

The edits that are being pushed are, going first:

{
  "ietf-hardware:hardware": {
    "last-change": "2024-04-10T01:51:04-00:00",
    "component": [
      {
        "name": "ne",
        "class": "iana-hardware:chassis",
        "mfg-name": "CESNET",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:power",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 0,
          "value-type": "watts",
          "value-scale": "micro",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:psu",
        "class": "iana-hardware:power-supply",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:psu:child",
        "class": "iana-hardware:sensor",
        "parent": "ne:psu",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 12000,
          "value-type": "volts-DC",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:temperature-cpu",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 41800,
          "value-type": "celsius",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      }
    ]
  }
}

and then this one as the second one:

{
  "ietf-hardware:hardware": {
    "last-change": "2024-04-10T01:51:05-00:00",
    "component": [
      {
        "name": "ne",
        "class": "iana-hardware:chassis",
        "mfg-name": "CESNET",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:power",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 0,
          "value-type": "watts",
          "value-scale": "micro",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:psu",
        "class": "iana-hardware:power-supply",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:psu:child",
        "class": "iana-hardware:sensor",
        "parent": "ne:psu",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 12000,
          "value-type": "volts-DC",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:temperature-cpu",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 41800,
          "value-type": "celsius",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      }
    ]
  }
}

and the thrid one:

{
  "ietf-hardware:hardware": {
    "last-change": "2024-04-10T01:51:05-00:00",
    "component": [
      {
        "name": "ne",
        "class": "iana-hardware:chassis",
        "mfg-name": "CESNET",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:power",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 0,
          "value-type": "watts",
          "value-scale": "micro",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:psu",
        "class": "iana-hardware:power-supply",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:psu:child",
        "class": "iana-hardware:sensor",
        "parent": "ne:psu",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 12000,
          "value-type": "volts-DC",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:temperature-cpu",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 41800,
          "value-type": "celsius",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      }
    ]
  }
}

which is followed by this one which crashes:

{
  "ietf-hardware:hardware": {
    "last-change": "2024-04-10T01:51:05-00:00",
    "component": [
      {
        "name": "ne",
        "class": "iana-hardware:chassis",
        "mfg-name": "CESNET",
        "state": {
          "oper-state": "enabled"
        }
      },
      {
        "name": "ne:power",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 11222333,
          "value-type": "watts",
          "value-scale": "micro",
          "value-precision": 0,
          "oper-status": "ok"
        }
      },
      {
        "name": "ne:psu",
        "class": "iana-hardware:power-supply",
        "parent": "ne",
        "state": {
          "oper-state": "disabled"
        }
      },
      {
        "name": "ne:psu:child"
      },
      {
        "name": "ne:temperature-cpu",
        "class": "iana-hardware:sensor",
        "parent": "ne",
        "state": {
          "oper-state": "enabled"
        },
        "sensor-data": {
          "value": 222,
          "value-type": "celsius",
          "value-scale": "milli",
          "value-precision": 0,
          "oper-status": "ok"
        }
      }
    ]
  },
  "sysrepo:discard-items": "/ietf-hardware:hardware/component[name='ne:psu:child']"
}

...maybe these will help you reproduce the bug without compiling our projects.

@lePici
Copy link
Collaborator

lePici commented Apr 10, 2024

I have tried to reproduce the problem but no luck. Your instructions make no sense to me.

get the latest devel of everything - so libyang and sysrepo? In that case libyang-cpp cannot be compiled even if I manually apply commit e1b046e. And if I follow the versions described in the README in the libyang-cpp and sysrepo-cpp projects, it cannot be compiled with commit e1b046e.

I also tried to reproduce the problem with just sysrepo, but I didn't get the assert error.

My initial suspicion is that the problem will not be with the lyds_insert function, but rather with lyd_find_sibling_schema . I would add the assert(leader) and assert(!leader->prev->next || (leader->schema != leader->prev->schema)) here, before calling the lyds_insert function. The leader should point to the first instance of the schema node.

@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 10, 2024

I have tried to reproduce the problem but no luck. Your instructions make no sense to me.

get the latest devel of everything - so libyang and sysrepo?

Sorry for that. Use the current devel of libyang and sysrepo.

In that case libyang-cpp cannot be compiled even if I manually apply commit e1b046e. And if I follow the versions described in the README in the libyang-cpp and sysrepo-cpp projects, it cannot be compiled with commit e1b046e.

Yes. We have a fair number of downstream projects, so it's essentially a "flag day" update for us, and we have a pre-merge CI to make sure that either everything works, or no breaking changes are merged.

Changes for libyang-cpp/sysrepo-cpp have not been merged to their respective master branches yet because of this regression (and the API changes that we have to adapt to). In short, you need to fetch these commits manually. Also, since we're splitting the work into many smaller commits, you cannot just apply a commit, it is necessary to checkout that commit. The easiest way is probably through a named ref, like this:

  • cd libyang-cpp
  • git fetch https://github.com/CESNET/libyang-cpp refs/changes/49/7049/1
  • git checkout -b bug-2223 FETCH_HEAD

For sysrepo-cpp, it's:

  • git fetch https://github.com/sysrepo/sysrepo-cpp refs/changes/33/7033/4

The SHAs are different because we've moved a bit since my original report; the old ones should work, but we're on these now.

I also tried to reproduce the problem with just sysrepo, but I didn't get the assert error.

My initial suspicion is that the problem will not be with the lyds_insert function, but rather with lyd_find_sibling_schema . I would add the assert(leader) and assert(!leader->prev->next || (leader->schema != leader->prev->schema)) here, before calling the lyds_insert function. The leader should point to the first instance of the schema node.

With the following patch in place:

diff --git a/src/tree_data.c b/src/tree_data.c
index 5158f939e..fc962bc39 100644
--- a/src/tree_data.c
+++ b/src/tree_data.c
@@ -761,6 +761,8 @@ lyd_insert_node(struct lyd_node *parent, struct lyd_node **first_sibling_p, stru
         lyd_insert_node_ordby_schema(parent, &first_sibling, node);
     } else if (lyds_is_supported(node) &&
             (lyd_find_sibling_schema(first_sibling, node->schema, &leader) == LY_SUCCESS)) {
+        assert(leader);
+        assert(!leader->prev->next || (leader->schema != leader->prev->schema));
         ret = lyds_insert(&first_sibling, &leader, node);
         if (ret) {
             /* The operation on the sorting tree unexpectedly failed due to some internal issue,

I'm now hitting that assert:

test-sysrepo_ietf-hardware: /home/jkt/work/cesnet/gerrit/github/CESNET/libyang/src/tree_data.c:765: void lyd_insert_node(struct lyd_node *, struct lyd_node **, struct lyd_node *, uint32_t): Assertion `!leader->prev->next || (leader->schema != leader->prev->schema)' failed.

@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 12, 2024

Do you need anything else to reproduce and/or fix this? It's one of the blockers for us switching to the new 2.2/3/whatever SW stack.

@lePici
Copy link
Collaborator

lePici commented Apr 12, 2024

Sorry I haven't replied yet. I'll try to look into it on Monday. I'll be sure to ask if I get stuck on something.

@lePici
Copy link
Collaborator

lePici commented Apr 15, 2024

I have successfully compiled the projects. When running the tests using make test, I see a failure due to assert in the log, which is great. I would like to debug now, but when I run ./test-sysrepo_ietf-hardware I get the error Module "ietf-hardware" was not found in sysrepo. I assume that the tests are somehow related and I can't run this test separately or I'm doing it wrong overall. Could you please write an example of how to debug this test?

@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 15, 2024

These tests are using CMake test fixtures to install the required YANG modules and env variables to run against a non-standard sysrepo repository/SHM locations. Running them via ctest -R test-sysrepo_ietf-hardware --output-on-failure will do the trick. If you want to attach a debugger, then this scripts which extracts fixtures from CMake/CTest might be handy, but you can also use brute force and just run it manually; it should be something like:

sysrepoctl \
    --install yang/iana-hardware@2018-03-13.yang \
    --install yang/ietf-hardware@2018-03-13.yang \
        --enable-feature hardware-sensor \
        --enable-feature hardware-state \
    --install yang/ietf-alarms@2019-09-11.yang \
        --enable-feature alarm-shelving \
        --enable-feature alarm-summary \
    --install yang/velia-alarms@2022-07-12.yang \
    --install tests/yang/sysrepo-ietf-alarms@2022-02-17.yang

(Here is the relevant fixture setup in CMake.)

Note -- this will bypass the init/cleanup stuff we have in the test setup, so do not mix this direct invocation with running through ctest (which is also what make test would do), and purge your sysrepo config before and after running this. The tests are designed to rely on CTest features for this.

lePici added a commit to lePici/libyang that referenced this issue Apr 15, 2024
A hash must be re-inserted on additional sorting, otherwise the parent
will have the wrong reference to the first instance of the schema.

Refs CESNET#2223
lePici added a commit that referenced this issue Apr 15, 2024
A hash must be re-inserted on additional sorting, otherwise the parent
will have the wrong reference to the first instance of the schema.

Refs #2223
@lePici
Copy link
Collaborator

lePici commented Apr 15, 2024

The tests should pass. Update the libyang devel branch.

@jktjkt
Copy link
Contributor Author

jktjkt commented Apr 15, 2024

thanks

@jktjkt jktjkt closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants