[PATCH] ldb: Fix index out of bound in ldb_msg_find_common_values

[Lukas Slebodnik]

On (06/07/17 10:05), Douglas Bagnall wrote:
>On 06/07/17 01:33, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> I noticed failure[1] when I was packaging libldb-1.2.0 to fedora
>> which was required for samba-4.7.0rc1. And I was quite lucky that
>> it failed at least for i386 :-)
>> 
>> I did not noticed it with 1.1.31 because unit tests were not executed as part
>> of build due to other issues.
>> 
>
>Thanks Lukas!
>
>
>> LS
>> 
>> [1] https://koji.fedoraproject.org/koji/taskinfo?taskID=20322524
>> 
>> 
>> 0001-ldb-Fix-index-out-of-bound-in-ldb_msg_find_common_va.patch
>> 
>> 
>> From 68e9da7bc4049b1a2080d07324cc26eebe5ee55b Mon Sep 17 00:00:00 2001
>> From: Lukas Slebodnik <lslebodn at redhat.com>
>> Date: Tue, 4 Jul 2017 15:46:49 +0200
>> Subject: [PATCH] ldb: Fix index out of bound in ldb_msg_find_common_values
>> 
>> cmocka unit test failed on i386
>> [==========] Running 2 test(s).
>> [ RUN      ] test_ldb_msg_find_duplicate_val
>> [       OK ] test_ldb_msg_find_duplicate_val
>> [ RUN      ] test_ldb_msg_find_common_values
>> [  FAILED  ] test_ldb_msg_find_common_values
>> [==========] 2 test(s) run.
>> [  ERROR   ] --- 0x14 != 0
>> [   LINE   ] --- ../tests/ldb_msg.c:266: error: Failure!
>> [  PASSED  ] 1 test(s).
>> [  FAILED  ] 1 test(s), listed below:
>> [  FAILED  ] test_ldb_msg_find_common_values
>>  1 FAILED TEST(S)
>> 
>> But we were just lucky on other platforms because there is
>> index out of bound according to valgrind error.
>> 
>> ==3298== Invalid read of size 4
>> ==3298==    at 0x486FCF6: ldb_val_cmp (ldb_msg.c:95)
>> ==3298==    by 0x486FCF6: ldb_msg_find_common_values (ldb_msg.c:266)
>> ==3298==    by 0x109A3D: test_ldb_msg_find_common_values (ldb_msg.c:265)
>> ==3298==    by 0x48E7490: ??? (in /usr/lib/libcmocka.so.0.4.1)
>> ==3298==    by 0x48E7EB0: _cmocka_run_group_tests (in /usr/lib/libcmocka.so.0.4.1)
>> ==3298==    by 0x1089B7: main (ldb_msg.c:352)
>> ==3298==  Address 0x4b07734 is 4 bytes after a block of size 48 alloc'd
>> ==3298==    at 0x483223E: malloc (vg_replace_malloc.c:299)
>> ==3298==    by 0x4907AA7: _talloc_array (in /usr/lib/libtalloc.so.2.1.9)
>> ==3298==    by 0x486FBF8: ldb_msg_find_common_values (ldb_msg.c:245)
>> ==3298==    by 0x109A3D: test_ldb_msg_find_common_values (ldb_msg.c:265)
>> ==3298==    by 0x48E7490: ??? (in /usr/lib/libcmocka.so.0.4.1)
>> ==3298==    by 0x48E7EB0: _cmocka_run_group_tests (in /usr/lib/libcmocka.so.0.4.1)
>> ==3298==    by 0x1089B7: main (ldb_msg.c:352)
>> 
>> Signed-off-by: Lukas Slebodnik <lslebodn at redhat.com>
>> ---
>>  lib/ldb/common/ldb_msg.c | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>> 
>> diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c
>> index abad5a8320551c09e64539b993b8c5408ccdd32a..8e4047b41beebcadeab9631bc820941f0eadc490 100644
>> --- a/lib/ldb/common/ldb_msg.c
>> +++ b/lib/ldb/common/ldb_msg.c
>> @@ -262,20 +262,12 @@ int ldb_msg_find_common_values(struct ldb_context *ldb,
>>  	n_values = el->num_values;
>>  	i = 0;
>>  	j = 0;
>> -	while (i != n_values) {
>> +	while (i != n_values && j < el2->num_values) {
>>  		int ret = ldb_val_cmp(&values[i], &values2[j]);
>>  		if (ret < 0) {
>>  			i++;
>>  		} else if (ret > 0) {
>>  			j++;
>> -			if (j == el2->num_values) {
>
>The problem was when el2 has no values, right? In which case we really
>don't want to be here to start with. Which I obviously failed to check
>and to test.
>

I do not think so.
I failed on line lib/ldb/tests/ldb_msg.c:265 and If I read test correctly
then both values has 4 elements
    el2 = new_msg_element(*state, "test", 4, 4);
    el3 = new_msg_element(*state, "test", 6, 4);

BTW test on line 261 passed ah there was just different order or values.
el2 and el3.


And when I was looking to unit test; I've jsut realized that following lines
(267-270) probably does not test what you wanted to test.

    ret = ldb_msg_find_common_values(NULL, *state, el3, el2, remove_dupes);
    assert_int_equal(ret, LDB_SUCCESS);
    ret = ldb_msg_find_common_values(NULL, *state, el2, el3, remove_dupes);
    assert_int_equal(ret, LDB_SUCCESS);

function ldb_msg_find_common_values has side effects in case of remove_dupes.
So you didn't test invocation of function with different order of el2 an el3.
because el3 was changed.

You will need to allocate new el3 after first test. And it would be good to
test also el{2,3}->num_values in assertions.

LS