Closed milanlenco closed 7 years ago
Please verify the patch in devel branch (version 0.12.31), if there won't be any problem from your point of view, I will cherry pick it into the master.
Valgrind detected two issues:
Conditional jump or move depends on uninitialised value(s)
at 0x5D03AE1: lyd_diff_move_preprocess (tree_data.c:2656)
by 0x5D044DD: lyd_diff (tree_data.c:2906)
by 0x4B5067: rp_dt_generate_config_change_notification (rp_dt_edit.c:726)
by 0x4B6A8C: rp_dt_commit (rp_dt_edit.c:862)
by 0x47E108: rp_commit_req_process (request_processor.c:1460)
by 0x490DB8: rp_req_dispatch (request_processor.c:3248)
by 0x4921EF: rp_msg_dispatch (request_processor.c:3431)
by 0x492B71: rp_worker_thread_execute (request_processor.c:3531)
by 0x54596F9: start_thread (pthread_create.c:333)
by 0x606DB5C: clone (clone.S:109)
Uninitialised value was created by a heap allocation
at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D03401: diff_ordset_insert (tree_data.c:2479)
by 0x5D03652: lyd_diff_compare (tree_data.c:2544)
by 0x5D0415B: lyd_diff (tree_data.c:2821)
by 0x4B5067: rp_dt_generate_config_change_notification (rp_dt_edit.c:726)
by 0x4B6A8C: rp_dt_commit (rp_dt_edit.c:862)
by 0x47E108: rp_commit_req_process (request_processor.c:1460)
by 0x490DB8: rp_req_dispatch (request_processor.c:3248)
by 0x4921EF: rp_msg_dispatch (request_processor.c:3431)
by 0x492B71: rp_worker_thread_execute (request_processor.c:3531)
by 0x54596F9: start_thread (pthread_create.c:333)
by 0x606DB5C: clone (clone.S:109)
Conditional jump or move depends on uninitialised value(s)
at 0x5D03AE1: lyd_diff_move_preprocess (tree_data.c:2656)
by 0x5D047D3: lyd_diff (tree_data.c:2964)
by 0x4B5067: rp_dt_generate_config_change_notification (rp_dt_edit.c:726)
by 0x4B6A8C: rp_dt_commit (rp_dt_edit.c:862)
by 0x47E108: rp_commit_req_process (request_processor.c:1460)
by 0x490DB8: rp_req_dispatch (request_processor.c:3248)
by 0x4921EF: rp_msg_dispatch (request_processor.c:3431)
by 0x492B71: rp_worker_thread_execute (request_processor.c:3531)
by 0x54596F9: start_thread (pthread_create.c:333)
by 0x606DB5C: clone (clone.S:109)
Uninitialised value was created by a heap allocation
at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x5D03401: diff_ordset_insert (tree_data.c:2479)
by 0x5D03652: lyd_diff_compare (tree_data.c:2544)
by 0x5D0415B: lyd_diff (tree_data.c:2821)
by 0x4B5067: rp_dt_generate_config_change_notification (rp_dt_edit.c:726)
by 0x4B6A8C: rp_dt_commit (rp_dt_edit.c:862)
by 0x47E108: rp_commit_req_process (request_processor.c:1460)
by 0x490DB8: rp_req_dispatch (request_processor.c:3248)
by 0x4921EF: rp_msg_dispatch (request_processor.c:3431)
by 0x492B71: rp_worker_thread_execute (request_processor.c:3531)
by 0x54596F9: start_thread (pthread_create.c:333)
by 0x606DB5C: clone (clone.S:109)
Also one of the tests fails with SIGSEGV:
#0 0x00007ffff6f29c87 in lyd_diff (first=0x7fffe0005b40, second=0x7fffe0001060, options=1) at /home/mlenco/dev/libyang/src/tree_data.c:3100
#1 0x00000000004b6c8a in dm_commit_notify (dm_ctx=0x819840, session=0x7fffe800f1c0, ev=SR_EV_VERIFY, c_ctx=0x7fffe0000f40) at /home/mlenco/dev/sysrepo/src/data_manager.c:3580
#2 0x0000000000492f19 in rp_dt_commit (rp_ctx=0x833cc0, session=0x7fffe800ef90, c_ctx=0x0, errors=0x7ffff4d68cc0, err_cnt=0x7ffff4d68cc8) at /home/mlenco/dev/sysrepo/src/rp_dt_edit.c:842
#3 0x000000000045a968 in rp_commit_req_process (rp_ctx=0x833cc0, session=0x7fffe800ef90, msg=0x7fffe4010018, skip_msg_cleanup=0x7ffff4d68d63) at /home/mlenco/dev/sysrepo/src/request_processor.c:1460
#4 0x000000000046d618 in rp_req_dispatch (rp_ctx=0x833cc0, session=0x7fffe800ef90, msg=0x7fffe4010018, skip_msg_cleanup=0x7ffff4d68d63) at /home/mlenco/dev/sysrepo/src/request_processor.c:3248
#5 0x000000000046ea4f in rp_msg_dispatch (rp_ctx=0x833cc0, session=0x7fffe800ef90, msg=0x7fffe4010018) at /home/mlenco/dev/sysrepo/src/request_processor.c:3431
#6 0x000000000046f3d1 in rp_worker_thread_execute (rp_ctx_p=0x833cc0) at /home/mlenco/dev/sysrepo/src/request_processor.c:3531
#7 0x00007ffff77ae6fa in start_thread (arg=0x7ffff4d69700) at pthread_create.c:333
#8 0x00007ffff6becb5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
But other than that, the behaviour is now as expected in the reported case.
Hi, I wasn't able to reproduce the issue in sysrepo test. Please check it again after the last change (fixing that uninitialized variable) and if the test still fails, provide some more information (its name and more of the output).
To reproduce the crash yourselves you will need to apply this small change on to the devel branch of sysrepo:
diff --git a/tests/helpers/test_module_helper.c b/tests/helpers/test_module_helper.c
index 258e2a6..f70ca91 100644
--- a/tests/helpers/test_module_helper.c
+++ b/tests/helpers/test_module_helper.c
@@ -142,6 +142,23 @@ createDataTreeTestModule()
n = lyd_new_leaf(node, module, "union", "infinity");
assert_non_null(n);
+ /* user-ordered leaf-list items */
+ node = lyd_new_leaf(NULL, module, "ordered-numbers", "45");
+ assert_non_null(node);
+ assert_int_equal(0, lyd_insert_after(r, node));
+
+ node = lyd_new_leaf(NULL, module, "ordered-numbers", "12");
+ assert_non_null(node);
+ assert_int_equal(0, lyd_insert_after(r, node));
+
+ node = lyd_new_leaf(NULL, module, "ordered-numbers", "57");
+ assert_non_null(node);
+ assert_int_equal(0, lyd_insert_after(r, node));
+
+ node = lyd_new_leaf(NULL, module, "ordered-numbers", "0");
+ assert_non_null(node);
+ assert_int_equal(0, lyd_insert_after(r, node));
+
/* list + list of leafrefs */
node = lyd_new(NULL, module, "university");
assert_non_null(node);
Then if you run cl_notifications_test, it will fail with the crash. The source of the mis-behave of libyang may be caused by the user-ordered list-items being interleaved with other siblings at the time of lyd_diff:
...
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">0</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">57</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">12</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">45</ordered-numbers>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k2</key>
<id_ref>id_2</id_ref>
<union>infinity</union>
</list>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k1</key>
<id_ref>id_1</id_ref>
<union>42</union>
<wireless/>
</list>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">1</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">2</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">3</ordered-numbers>
(this is compared with the config where "3" is moved after "1" when if crashes).
Also with the change above applied, rp_dt_edit_test will also fail, but that is because the test itself requires update so just ignore it. Similarly for tests beyond cl_notifications_test.
I was able to reproduce the issue with the provided example and it should be fixed in the current devel (0.12.35). Please, verify that there isn't some other issue with this in sysrepo.
cl_test now crashes with:
(gdb) bt
#0 0x00007ffff6f297fd in lyd_diff (first=0x7fffe0026410, second=0x7fffe0021190, options=1) at /home/mlenco/dev/libyang/src/tree_data.c:2912
#1 0x00000000004b3ce0 in rp_dt_generate_config_change_notification (rp_ctx=0x835de0, session=0x7fffe4030d10, c_ctx=0x7fffe0021fa0) at /home/mlenco/dev/sysrepo/src/rp_dt_edit.c:726
#2 0x00000000004b546f in rp_dt_commit (rp_ctx=0x835de0, session=0x7fffe4030d10, c_ctx=0x0, errors=0x7ffff4d68cc0, err_cnt=0x7ffff4d68cc8) at /home/mlenco/dev/sysrepo/src/rp_dt_edit.c:849
#3 0x000000000047d71c in rp_commit_req_process (rp_ctx=0x835de0, session=0x7fffe4030d10, msg=0x7fffec022ec8, skip_msg_cleanup=0x7ffff4d68d63) at /home/mlenco/dev/sysrepo/src/request_processor.c:1493
#4 0x000000000048faa5 in rp_req_dispatch (rp_ctx=0x835de0, session=0x7fffe4030d10, msg=0x7fffec022ec8, skip_msg_cleanup=0x7ffff4d68d63) at /home/mlenco/dev/sysrepo/src/request_processor.c:3198
#5 0x0000000000490ebd in rp_msg_dispatch (rp_ctx=0x835de0, session=0x7fffe4030d10, msg=0x7fffec022ec8) at /home/mlenco/dev/sysrepo/src/request_processor.c:3378
#6 0x000000000049183f in rp_worker_thread_execute (rp_ctx_p=0x835de0) at /home/mlenco/dev/sysrepo/src/request_processor.c:3478
#7 0x00007ffff77ae6fa in start_thread (arg=0x7ffff4d69700) at pthread_create.c:333
#8 0x00007ffff6becb5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Also valgrind has found some memory leak, but that may be unrelated to your fix:
==10513== 320 (64 direct, 256 indirect) bytes in 4 blocks are definitely lost in loss record 3 of 3
==10513== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10513== by 0x5D0A1B3: ly_set_new (tree_data.c:5337)
==10513== by 0x5CFBD29: lys_find_xpath (tree_schema.c:3252)
==10513== by 0x5CA7F19: check_instid_ext_dep (resolve.c:6787)
==10513== by 0x5CA8C55: resolve_unres_data_item (resolve.c:7159)
==10513== by 0x5CA9720: resolve_unres_data (resolve.c:7425)
==10513== by 0x5D0C25C: lyd_defaults_add_unres (tree_data.c:6241)
==10513== by 0x5D0797B: lyd_validate (tree_data.c:4239)
==10513== by 0x417E1C: createDataTreeTestModule (test_module_helper.c:278)
==10513== by 0x415773: ac_test_setup (ac_test.c:47)
==10513== by 0x50451D0: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.3.1)
==10513== by 0x50453C7: cmocka_run_one_tests (in /usr/local/lib/libcmocka.so.0.3.1)
The memory leak should be fixed.
Regarding the crash - is there a way how to get know which data are passed to the lyd_diff()?
It crashes for example when I compare this:
<main xmlns="urn:ietf:params:xml:ns:yang:test-module">
<enum>maybe</enum>
<raw>SGVsbG8gd29ybGQh</raw>
<options>strict recursive</options>
<dec64>9.85</dec64>
<i8>8</i8>
<i16>16</i16>
<i32>32</i32>
<i64>64</i64>
<ui8>8</ui8>
<ui16>16</ui16>
<ui32>32</ui32>
<ui64>64</ui64>
<empty/>
<boolean>true</boolean>
<string>str</string>
<id_ref>id_1</id_ref>
<xml-data><p xmlns="http://www.w3.org/1999/xhtml">This is <em>very</em> cool.</p></xml-data>
<any-data><container><leaf1>value1</leaf1><leaf2>value2</leaf2></container></any-data>
<instance_id xmlns:tm="urn:ietf:params:xml:ns:yang:test-module">/tm:main/tm:i64</instance_id>
<numbers>1</numbers>
<numbers>2</numbers>
<numbers>42</numbers>
</main>
<dec64-in-union xmlns="urn:ietf:params:xml:ns:yang:test-module">-11.17</dec64-in-union>
<kernel-modules xmlns="urn:ietf:params:xml:ns:yang:test-module">
<kernel-module>
<name>netlink_diag.ko</name>
<location>/lib/modules/kernel/net/netlink</location>
<loaded>false</loaded>
</kernel-module>
<kernel-module>
<name>irqbypass.ko</name>
<location>/lib/modules/kernel/virt/lib</location>
<loaded>true</loaded>
</kernel-module>
<kernel-module>
<name>vboxvideo.ko</name>
<location>/lib/modules/kernel/misc</location>
<loaded>false</loaded>
</kernel-module>
</kernel-modules>
<leafref-chain xmlns="urn:ietf:params:xml:ns:yang:test-module">
<D>final-leaf</D>
<C>final-leaf</C>
</leafref-chain>
<university xmlns="urn:ietf:params:xml:ns:yang:test-module">
<students>
<student>
<name>nameA</name>
<age>19</age>
</student>
<student>
<name>nameB</name>
<age>17</age>
</student>
<student>
<name>nameC</name>
<age>18</age>
</student>
</students>
<classes>
<class>
<title>CCNA</title>
<student>
<name>nameB</name>
<age>17</age>
</student>
<student>
<name>nameC</name>
</student>
</class>
</classes>
</university>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">0</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">57</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">12</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">45</ordered-numbers>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k2</key>
<id_ref>id_2</id_ref>
<union>infinity</union>
</list>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k1</key>
<id_ref>id_1</id_ref>
<union>42</union>
<wireless/>
</list>
with this:
<main xmlns="urn:ietf:params:xml:ns:yang:test-module">
<enum>maybe</enum>
<raw>SGVsbG8gd29ybGQh</raw>
<options>strict recursive</options>
<dec64>9.85</dec64>
<i8>9</i8>
<i16>16</i16>
<i32>32</i32>
<i64>64</i64>
<ui8>8</ui8>
<ui16>16</ui16>
<ui32>32</ui32>
<ui64>64</ui64>
<empty/>
<boolean>true</boolean>
<string>str</string>
<id_ref>id_1</id_ref>
<xml-data><p xmlns="http://www.w3.org/1999/xhtml">This is <em>very</em> cool.</p></xml-data>
<any-data><container><leaf1>value1</leaf1><leaf2>value2</leaf2></container></any-data>
<instance_id xmlns:tm="urn:ietf:params:xml:ns:yang:test-module">/tm:main/tm:i64</instance_id>
<numbers>1</numbers>
<numbers>2</numbers>
<numbers>42</numbers>
</main>
<dec64-in-union xmlns="urn:ietf:params:xml:ns:yang:test-module">-11.17</dec64-in-union>
<kernel-modules xmlns="urn:ietf:params:xml:ns:yang:test-module">
<kernel-module>
<name>netlink_diag.ko</name>
<location>/lib/modules/kernel/net/netlink</location>
<loaded>false</loaded>
</kernel-module>
<kernel-module>
<name>irqbypass.ko</name>
<location>/lib/modules/kernel/virt/lib</location>
<loaded>true</loaded>
</kernel-module>
<kernel-module>
<name>vboxvideo.ko</name>
<location>/lib/modules/kernel/misc</location>
<loaded>false</loaded>
</kernel-module>
</kernel-modules>
<leafref-chain xmlns="urn:ietf:params:xml:ns:yang:test-module">
<D>final-leaf</D>
<C>final-leaf</C>
</leafref-chain>
<university xmlns="urn:ietf:params:xml:ns:yang:test-module">
<students>
<student>
<name>nameA</name>
<age>19</age>
</student>
<student>
<name>nameB</name>
<age>17</age>
</student>
<student>
<name>nameC</name>
<age>18</age>
</student>
</students>
<classes>
<class>
<title>CCNA</title>
<student>
<name>nameB</name>
<age>17</age>
</student>
<student>
<name>nameC</name>
</student>
</class>
</classes>
</university>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">0</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">57</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">12</ordered-numbers>
<ordered-numbers xmlns="urn:ietf:params:xml:ns:yang:test-module">45</ordered-numbers>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k2</key>
<id_ref>id_2</id_ref>
<union>infinity</union>
</list>
<list xmlns="urn:ietf:params:xml:ns:yang:test-module">
<key>k1</key>
<id_ref>id_1</id_ref>
<union>42</union>
<wireless/>
</list>
/test-module:main/i8
has only changed.
with the last patch (0.12.39) the cl_test
passes, but please do another round of checking, thanks.
All tests are passing now, thanks!
Hi, now master branch seems to be broken, our tests fail on this:
==10531== Thread 2:
==10531== Conditional jump or move depends on uninitialised value(s)
==10531== at 0x5AF7962: lyd_diff_move_preprocess (tree_data.c:2193)
==10531== by 0x5AF83E0: lyd_diff (tree_data.c:2459)
==10531== by 0x50B6FD: dm_commit_notify (data_manager.c:3164)
==10531== by 0x4D2A8E: rp_dt_commit (rp_dt_edit.c:735)
==10531== by 0x47519A: rp_commit_req_process (request_processor.c:1141)
==10531== by 0x49183D: rp_req_dispatch (request_processor.c:2571)
==10531== by 0x493ADE: rp_msg_dispatch (request_processor.c:2723)
==10531== by 0x494CC8: rp_worker_thread_execute (request_processor.c:2823)
==10531== by 0x5252183: start_thread (pthread_create.c:312)
==10531== by 0x5E5337C: clone (clone.S:111)
==10531==
==10531== Conditional jump or move depends on uninitialised value(s)
==10531== at 0x5AF7962: lyd_diff_move_preprocess (tree_data.c:2193)
==10531== by 0x5AF8779: lyd_diff (tree_data.c:2536)
==10531== by 0x50B6FD: dm_commit_notify (data_manager.c:3164)
==10531== by 0x4D2A8E: rp_dt_commit (rp_dt_edit.c:735)
==10531== by 0x47519A: rp_commit_req_process (request_processor.c:1141)
==10531== by 0x49183D: rp_req_dispatch (request_processor.c:2571)
==10531== by 0x493ADE: rp_msg_dispatch (request_processor.c:2723)
==10531== by 0x494CC8: rp_worker_thread_execute (request_processor.c:2823)
==10531== by 0x5252183: start_thread (pthread_create.c:312)
==10531== by 0x5E5337C: clone (clone.S:111)
==10531==
https://s3.amazonaws.com/archive.travis-ci.org/jobs/193032576/log.txt
...and during SR's installation:
-- Exec: /home/jkt/work/prog/sysrepo/build/src/sysrepocfg --import=/home/jkt/work/prog/sysrepo/examples/yang/turing-machine.data.xml --datastore=startup --format=xml --level=0 turing-machine
ASAN:DEADLYSIGNAL
=================================================================
==8871==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f3b72a36821 bp 0x7ffccd7fad10 sp 0x7ffccd7f9ea0 T0)
==8871==The signal is caused by a READ memory access.
==8871==Hint: address points to the zero page.
#0 0x7f3b72a36820 in lyd_diff /home/jkt/work/prog/libyang/src/tree_data.c:2912:109
#1 0x519c7a in srcfg_import_datastore /home/jkt/work/prog/sysrepo/src/executables/sysrepocfg.c:719:12
#2 0x51556c in srcfg_import_operation /home/jkt/work/prog/sysrepo/src/executables/sysrepocfg.c:849:11
#3 0x512589 in main /home/jkt/work/prog/sysrepo/src/executables/sysrepocfg.c:1446:18
#4 0x7f3b71367733 in __libc_start_main /var/tmp/portage/sys-libs/glibc-2.22-r4/work/glibc-2.22/csu/libc-start.c:289
#5 0x41ab78 in _start (/home/jkt/work/prog/sysrepo/build/src/sysrepocfg+0x41ab78)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/jkt/work/prog/libyang/src/tree_data.c:2912:109 in lyd_diff
Ugh, please disregard my last comment. I am unable to reproduce it despite quite a few bisections.
I'm really sorry - I have overlooked one of the patches when cherry-picking into master.
No problem, seems to be working now.
When I run
lyd_diff
to compare between this:and this configuration:
libyang tells me that there are no differences.
Definition of this leaf-list comes from our YANG file used for unit tests: https://github.com/sysrepo/sysrepo/blob/master/tests/yang/test-module.yang