There comes a libyang ASAN issue when call ly_ctx_remove_module

[nickylba]

READ of size 1 at 0xec4d2a08 thread T..
#0 0xf727f834 in __interceptor_strncmp ../../../sanitizer_common/sanitizer_common_interceptors.inc:251
#1 0xedae6e07 in lys_get_import_module tree_schema.c:2562
#2 0xedac1325 in resolve_augment_schema_nodeid resolve.c:1647
#3 0xedaeaf78 in lys_switch_deviation tree_schema.c:4183
#4 0xedaeb3df in remove_dev tree_schema.c:4290
#5 0xedaeb7c2 in lys_sub_module_remove_devs_augs tree_schema.c:4363
#6 0xeda47897 in ly_ctx_remove_module context.c:1216

0xec4d2a08 is located 0 bytes inside of 12-byte region [0xec4d2a08,0xec4d2a14)
freed by thread T.. here:
#0 0xf7306cd4 in __interceptor_free ../../../asan/asan_malloc_linux.cc:28
#1 0xeda48a0d in lydict_remove dict.c:148
#2 0xedae751d in module_free_common tree_schema.c:2678
#3 0xedae960c in lys_free tree_schema.c:3430
#4 0xeda478c1 in ly_ctx_remove_module context.c:1218

previously allocated by thread T.. here:
#0 0xf7306e54 in _interceptor_malloc ../../../asan/asan_malloc_linux.cc:38
#1 0x80811b6 in malloc syslib/frame_malloc.c:357
#2 0xedaf0090 in parse_text xml.c:710
#3 0xedaf07ba in parse_attr xml.c:806
#4 0xedaf17f1 in lyxml_parse_elem xml.c:1066
#5 0xedaf1ba0 in lyxml_parse_mem xml.c:1165
#6 0xeda98a29 in yin_read_module parser_yin.c:7112
#7 0xedae1aa2 in lys_parse_mem tree_schema.c:961
#8 0xedae2164 in lys_parse_fd tree_schema.c:1115
#9 0xedae1e3f in lys_parse_path tree_schema.c:1052

The libyang version that I used is 0.12.199 with following log:
SHA-1: 843a8c6

• VERSION bump to version 0.12.199

It seems that when call ly_ctx_remove_module (which will then call lys_sub_module_remove_devs_augs), the inner function will try to read some module that have been freed by lys_free after that line, do we need move lys_free to another for-loop?

Logic seems same in the code of latest version and the devel branch (inner-most function name have been replaced by resolve_schema_nodeid and lyp_get_module)

/* free the modules */
    for (u = 0; u < mods->number; u++) {
        /* remove the applied deviations and augments */
        lys_sub_module_remove_devs_augs((struct lys_module *)mods->set.g[u]);
        /* remove the module */
        lys_free((struct lys_module *)mods->set.g[u], private_destructor, 0);
    }

[michalvasko]

Hi,
yes, there were a number of problems with removing models but numerous bugfixes were introduced since 0.12 so unless you can reproduce the problem in the current devel or master branch, we will not waste out time on it just to find out it was already fixed.

Regards,
Michal

[nickylba]

Hi, I already check the latest code in the devel and master brach, the root-cause of the ASAN is following code in ly_ctx_remove_module, which is still there:

    /* free the modules */
    for (u = 0; u < mods->number; u++) {
        /* remove the applied deviations and augments */
        lys_sub_module_remove_devs_augs((struct lys_module *)mods->set.g[u]);
        /* remove the module */
        lys_free((struct lys_module *)mods->set.g[u], private_destructor, 0);
    }

when there is some dependencies module to be unloaded (say module-b have some deviation for module-a), when call lys_sub_module_remove_devs_augs for module-b, it rely on the module name of module-a in function lyp_get_module, callstack is:

lys_sub_module_remove_devs_augs --> remove_dev --> lys_switch_deviation -->
resolve_schema_nodeid --> lyp_get_module

in lyp_get_module, it will try to read main-module's name, but main-module have been removed. I will try to upgrade to newest version to check whether it is exist, but it may consume some time, maybe u can help confirm the code.

The following code change can fix the ASAN in my version

    for (u = 0; u < mods->number; u++) {
        /* remove the applied deviations and augments */
        lys_sub_module_remove_devs_augs((struct lys_module *)mods->set.g[u]);
    }

    for (u = 0; u < mods->number; u++) {
        /* remove the module */
        lys_free((struct lys_module *)mods->set.g[u], private_destructor, 0);
    }

[michalvasko]

Hi,
like I said, we have been fixing some problems with the order of removing models but it is possible it is still not correct. Nevertheless, your modification seems fine and should cause no issues.

Regards,
Michal

[nickylba]

Thanks, will check new version and confirm whether the problem still exist