Invalid comparison of strings in dict

[olivier-matz-6wind]

While reading dict's code, I was surprised to see the following test in lydict_val_eq():

if (strncmp(str1, str2, *(size_t *)cb_data) == 0) {
    return 1;
}

*cb_data contains the length of str1, which means that if we compare str1 with (str1 + "something"), it will return true.

This happens rarely because the hash table code already compares the hash, so the string must also have the same hash to trigger the issue. I've setup a unit test that points out this wrong behavior, I'm attaching it.

add_collision_test.patch.txt

I also have 2 draft fixes for this:

• idea 1: ensure that strings are always nul-terminated (this imply to duplicate string in lydict_insert() before calling dict_insert()), and use strcmp() instead of strncmp() in lydict_val_eq().
  This is maybe not ideal for the zero-copy use case.
  https://github.com/olivier-matz-6wind/libyang/tree/dict_idea1

• idea 2: also check that str2[*cb_data] == '\0' in lydict_val_eq(). Unfortunately I did not manage to solve this issue with this simple change: there are cases where we are called from a resize where it causes issues. I ended up with a quite big patch (on top of PR Optimize hash table insertion #2107) that removes the lyht_{insert|remove}_with_resize_cb() variants. While it passes 100% of unit tests, it probably needs a careful review.
  https://github.com/olivier-matz-6wind/libyang/tree/dict_idea2

Please let me know if I need to do a pull request, thanks!

[michalvasko]

Yes, it should be fixed in libyang devel.

[olivier-matz-6wind]

Thanks for the fix!

I was first surprised by your commit because I tried it myself and unit tests were broken after that.
So just for the record, both commits de17be1 ("dict BUGFIX check whole compared string") and c6a1fe6 ("dict UPDATE avoid comparison of the exact same strings") are actually needed.