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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
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.

LS

[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=20322524

0001-ldb-Fix-index-out-of-bound-in-ldb_msg_find_common_va.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Hello Lukas,

> 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.

What does valgrind say on x86_64?

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On (05/07/17 15:37), Stefan Metzmacher via samba-technical wrote:

>Hello Lukas,
>
>> 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.
>
>What does valgrind say on x86_64?
>

Almost the same it just failed in different test on x86_64

[ RUN      ] test_ldb_msg_find_common_values
--340-- REDIR: 0x606cda0 (libc.so.6:__strcmp_ssse3) redirected to 0x4c33cb0 (strcmp)
==340== Invalid read of size 8
==340==    at 0x5068E74: ldb_val_cmp (ldb_msg.c:95)
==340==    by 0x5068E74: ldb_msg_find_common_values (ldb_msg.c:266)
==340==    by 0x109849: test_ldb_msg_find_common_values (ldb_msg.c:265)
==340==    by 0x58E1978: ??? (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x58E2260: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x108C9F: main (ldb_msg.c:352)
==340==  Address 0x62fb8a8 is 8 bytes after a block of size 96 alloc'd
==340==    at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
==340==    by 0x5CFEC5B: _talloc_array (in /usr/lib64/libtalloc.so.2.1.9)
==340==    by 0x5068DA0: ldb_msg_find_common_values (ldb_msg.c:245)
==340==    by 0x109849: test_ldb_msg_find_common_values (ldb_msg.c:265)
==340==    by 0x58E1978: ??? (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x58E2260: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x108C9F: main (ldb_msg.c:352)
==340==
==340== Invalid read of size 8
==340==    at 0x5068E74: ldb_val_cmp (ldb_msg.c:95)
==340==    by 0x5068E74: ldb_msg_find_common_values (ldb_msg.c:266)
==340==    by 0x1098A1: test_ldb_msg_find_common_values (ldb_msg.c:269)
==340==    by 0x58E1978: ??? (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x58E2260: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x108C9F: main (ldb_msg.c:352)
==340==  Address 0x62fbc68 is 8 bytes after a block of size 96 alloc'd
==340==    at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
==340==    by 0x5CFEC5B: _talloc_array (in /usr/lib64/libtalloc.so.2.1.9)
==340==    by 0x5068DA0: ldb_msg_find_common_values (ldb_msg.c:245)
==340==    by 0x1098A1: test_ldb_msg_find_common_values (ldb_msg.c:269)
==340==    by 0x58E1978: ??? (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x58E2260: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.1)
==340==    by 0x108C9F: main (ldb_msg.c:352)
==340==
[       OK ] test_ldb_msg_find_common_values
[==========] 2 test(s) run.
[  PASSED  ] 2 test(s).

BTW when I was debugging it on i686 chroot I could not see failure in test.
Therefore I tried valgrind. I was really lucky that it failed in fedora build
system(koji) on i686. Because armv7hl in 32 bit as well but unit test did not
fail.

LS

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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.

We also need something like the attached patch. And a test or two, which
I'll get onto.

thanks,
Douglas

> - /*
> -  We have walked past the end of the second
> -  list, meaning the remainder of the first
> -  list cannot collide and we're done.
> - */
> - break;
> - }
>   } else {
>   /* we have a collision */
>   if (! remove_duplicates) {
> -- 2.13.0
>


0001-ldb-avoid-searching-empty-lists.patch (862 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 06/07/17 10:05, Douglas Bagnall via samba-technical wrote:

> On 06/07/17 01:33, Lukas Slebodnik wrote:
>> - 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.
>
> We also need something like the attached patch. And a test or two, which
> I'll get onto.
>
Well, here I've added tests of zero length elements that exercises this
path, but they don't usually fail without the fix because out-of-bounds
reads are like that.

Can we get another reviewer?

cheers,
Douglas

ldb-msg.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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 <[hidden email]>
>> 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 <[hidden email]>
>> ---
>>  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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On (06/07/17 16:38), Douglas Bagnall via samba-technical wrote:

>On 06/07/17 10:05, Douglas Bagnall via samba-technical wrote:
>> On 06/07/17 01:33, Lukas Slebodnik wrote:
>>> - 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.
>>
>> We also need something like the attached patch. And a test or two, which
>> I'll get onto.
>>
>
>Well, here I've added tests of zero length elements that exercises this
>path, but they don't usually fail without the fix because out-of-bounds
>reads are like that.
>
>Can we get another reviewer?
>
>cheers,
>Douglas

>From 2fbf68379f0f6e7b917f91a6eb99fa8fd6b31800 Mon Sep 17 00:00:00 2001
>From: Douglas Bagnall <[hidden email]>
>Date: Thu, 6 Jul 2017 10:01:24 +1200
>Subject: [PATCH 2/3] ldb: avoid searching empty lists in
> ldb_msg_find_common_values
>
>Signed-off-by: Douglas Bagnall <[hidden email]>
>---
> lib/ldb/common/ldb_msg.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c
>index 8e4047b..c2782db 100644
>--- a/lib/ldb/common/ldb_msg.c
>+++ b/lib/ldb/common/ldb_msg.c
>@@ -207,6 +207,9 @@ int ldb_msg_find_common_values(struct ldb_context *ldb,
> if (strcmp(el->name, el2->name) != 0) {
> return LDB_ERR_INAPPROPRIATE_MATCHING;
> }
>+ if (el->num_values == 0 || el2->num_values == 0) {
>+ return LDB_SUCCESS;
>+ }
> /*
>   With few values, it is better to do the brute-force search than the
>   clever search involving tallocs, memcpys, sorts, etc.
>--
>2.7.4
>

I think it is reasonable performance optimisation :-)

>
>From c1d0ce9f010525e80ecb1dbcbae105da33edc902 Mon Sep 17 00:00:00 2001
>From: Douglas Bagnall <[hidden email]>
>Date: Thu, 6 Jul 2017 12:41:07 +1200
>Subject: [PATCH 3/3] ldb/tests: more thoroughly test empty ldb_msg elements
>
>Signed-off-by: Douglas Bagnall <[hidden email]>
>---
> lib/ldb/tests/ldb_msg.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
>diff --git a/lib/ldb/tests/ldb_msg.c b/lib/ldb/tests/ldb_msg.c
>index e665d55..b3361dc 100644
>--- a/lib/ldb/tests/ldb_msg.c
>+++ b/lib/ldb/tests/ldb_msg.c
>@@ -87,6 +87,11 @@ static void test_ldb_msg_find_duplicate_val(void **state)
> ret = ldb_msg_add_empty(msg, "el1", 0, &el);
> assert_int_equal(ret, LDB_SUCCESS);
>
>+ /* An empty message contains no duplicates */
>+ ret = ldb_msg_find_duplicate_val(NULL, test_ctx, el, &dupe, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ assert_null(dupe);
>+
> for (i = 0; i < 5; i++) {
> add_uint_value(test_ctx, msg, "el1", i);
> }
>@@ -176,19 +181,19 @@ static void _assert_element_equal(struct ldb_message_element *a,
> static void test_ldb_msg_find_common_values(void **state)
> {
> /* we only use the state as a talloc context */
>- struct ldb_message_element *el, *el2, *el3, *el4, *el2b;
>+ struct ldb_message_element *el, *el2, *el3, *el4, *el2b, *empty;
> struct ldb_message_element *orig, *orig2, *orig3, *orig4;
> int ret;
> const uint32_t remove_dupes = LDB_MSG_FIND_COMMON_REMOVE_DUPLICATES;
> el = new_msg_element(*state, "test", 0, 4);
> el2 = new_msg_element(*state, "test", 4, 4);
> el3 = new_msg_element(*state, "test", 6, 4);
>+ empty = new_msg_element(*state, "test", 0, 0);
> orig = new_msg_element(*state, "test", 0, 4);
> orig2 = new_msg_element(*state, "test", 4, 4);
> orig3 = new_msg_element(*state, "test", 6, 4);
>
> /* first round is with short value arrays, using quadratic method */
>-
> /* we expect no collisions here */
> ret = ldb_msg_find_common_values(NULL, *state, el, el2, 0);
> assert_int_equal(ret, LDB_SUCCESS);
>@@ -256,7 +261,7 @@ static void test_ldb_msg_find_common_values(void **state)
> assert_element_equal(el2, orig2);
> assert_int_equal(el3->num_values, 0);
>
>- /* seeing as we have an empty element, try permutations therewith.
>+ /* permutations involving empty elements.
>   everything should succeed. */
> ret = ldb_msg_find_common_values(NULL, *state, el3, el2, 0);
> assert_int_equal(ret, LDB_SUCCESS);
>@@ -268,6 +273,12 @@ static void test_ldb_msg_find_common_values(void **state)
> assert_int_equal(ret, LDB_SUCCESS);
> ret = ldb_msg_find_common_values(NULL, *state, el2, el3, remove_dupes);
> assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, el3, empty, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, empty, empty, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, empty, el3, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>
> assert_element_equal(el2, orig2);
> assert_element_equal(el, orig);
>@@ -329,6 +340,18 @@ static void test_ldb_msg_find_common_values(void **state)
> orig2 = new_msg_element(*state, "test", 12, 2);
> assert_element_equal(el2, orig2);
>
>+ /* test the empty el against the full elements */
>+ ret = ldb_msg_find_common_values(NULL, *state, el, empty, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, empty, el, 0);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, el, empty, remove_dupes);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ ret = ldb_msg_find_common_values(NULL, *state, empty, el, remove_dupes);
>+ assert_int_equal(ret, LDB_SUCCESS);
>+ assert_element_equal(el, orig);
>+ assert_element_equal(empty, el3);
>+
> /* make sure an identical element with a different name is rejected */
> el2 = new_msg_element(*state, "fish", 12, 2);
> ret = ldb_msg_find_common_values(NULL, *state, el2, el, remove_dupes);
>--
>2.7.4
>

Have you tried to run unit test with valgrind / ore some address/memory
sanitizer?

Otherwise looks good to me. But it would be good to fix also comment from
different mail in this thread about remove_dupes.

Thank you very much for unit tests. They helped us to find bug :-)
I really appreciate it.

LS

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 06/07/17 21:48, Lukas Slebodnik wrote:
>
> Have you tried to run unit test with valgrind / ore some address/memory
> sanitizer?

No. I think I have lost that habit because I am so used to writing python
tests that act as clients, where valgrind tells you nothing at all useful.

> Otherwise looks good to me. But it would be good to fix also comment from
> different mail in this thread about remove_dupes.
>
> Thank you very much for unit tests. They helped us to find bug :-)
> I really appreciate it.

You are welcome.

Douglas
 
> LS
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On (06/07/17 22:20), Douglas Bagnall via samba-technical wrote:
>On 06/07/17 21:48, Lukas Slebodnik wrote:
>>
>> Have you tried to run unit test with valgrind / ore some address/memory
>> sanitizer?
>
>No. I think I have lost that habit because I am so used to writing python
>tests that act as clients, where valgrind tells you nothing at all useful.
>

I ran then and it passed.
For record
   LD_LIBRARY_PATH=bin/shared/ valgrind -v bin/ldb_msg_test
or running everything with valgrind
   valgrind -v --trace-children=yes make check

I was not running it from samba git but after extracting ldb from tarball
+ patching source code.


>> Otherwise looks good to me. But it would be good to fix also comment from
>> different mail in this thread about remove_dupes.
>>
>> Thank you very much for unit tests. They helped us to find bug :-)
>> I really appreciate it.
>
>You are welcome.
>

BTW I checked unit test one more time and you ere right.
It was caused by "el3->num_values == 0" because el3 was changed
after 1st initialisation.

So my patch is not needed; your solution is more elegant
and my patch can be dropped from patch set.

LS

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 07/07/17 00:18, Lukas Slebodnik wrote:

> On (06/07/17 22:20), Douglas Bagnall via samba-technical wrote:
>> On 06/07/17 21:48, Lukas Slebodnik wrote:
>>>
>>> Have you tried to run unit test with valgrind / ore some address/memory
>>> sanitizer?
>>
>> No. I think I have lost that habit because I am so used to writing python
>> tests that act as clients, where valgrind tells you nothing at all useful.
>>
>
> I ran then and it passed.
> For record
>    LD_LIBRARY_PATH=bin/shared/ valgrind -v bin/ldb_msg_test
> or running everything with valgrind
>    valgrind -v --trace-children=yes make check
>
> I was not running it from samba git but after extracting ldb from tarball
> + patching source code.
>
>
>>> Otherwise looks good to me. But it would be good to fix also comment from
>>> different mail in this thread about remove_dupes.
>>>
>>> Thank you very much for unit tests. They helped us to find bug :-)
>>> I really appreciate it.
>>
>> You are welcome.
>>
>
> BTW I checked unit test one more time and you ere right.
> It was caused by "el3->num_values == 0" because el3 was changed
> after 1st initialisation.
>
> So my patch is not needed; your solution is more elegant
> and my patch can be dropped from patch set.
I think your first patch is still correct. It makes that loop more
succinct and safe at no cost.

I have added a few more asserts about element size in the tests.

Douglas


>
> LS
>


ldb-msg.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On (07/07/17 09:07), Douglas Bagnall wrote:

>On 07/07/17 00:18, Lukas Slebodnik wrote:
>> On (06/07/17 22:20), Douglas Bagnall via samba-technical wrote:
>>> On 06/07/17 21:48, Lukas Slebodnik wrote:
>>>>
>>>> Have you tried to run unit test with valgrind / ore some address/memory
>>>> sanitizer?
>>>
>>> No. I think I have lost that habit because I am so used to writing python
>>> tests that act as clients, where valgrind tells you nothing at all useful.
>>>
>>
>> I ran then and it passed.
>> For record
>>    LD_LIBRARY_PATH=bin/shared/ valgrind -v bin/ldb_msg_test
>> or running everything with valgrind
>>    valgrind -v --trace-children=yes make check
>>
>> I was not running it from samba git but after extracting ldb from tarball
>> + patching source code.
>>
>>
>>>> Otherwise looks good to me. But it would be good to fix also comment from
>>>> different mail in this thread about remove_dupes.
>>>>
>>>> Thank you very much for unit tests. They helped us to find bug :-)
>>>> I really appreciate it.
>>>
>>> You are welcome.
>>>
>>
>> BTW I checked unit test one more time and you ere right.
>> It was caused by "el3->num_values == 0" because el3 was changed
>> after 1st initialisation.
>>
>> So my patch is not needed; your solution is more elegant
>> and my patch can be dropped from patch set.
>
>I think your first patch is still correct. It makes that loop more
>succinct and safe at no cost.
>
>I have added a few more asserts about element size in the tests.
>

Thank you.

ACK

LS

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Friday, 7 July 2017 09:21:11 CEST Lukas Slebodnik via samba-technical
wrote:

> On (07/07/17 09:07), Douglas Bagnall wrote:
> >On 07/07/17 00:18, Lukas Slebodnik wrote:
> >> On (06/07/17 22:20), Douglas Bagnall via samba-technical wrote:
> >>> On 06/07/17 21:48, Lukas Slebodnik wrote:
> >>>> Have you tried to run unit test with valgrind / ore some address/memory
> >>>> sanitizer?
> >>>
> >>> No. I think I have lost that habit because I am so used to writing
> >>> python
> >>> tests that act as clients, where valgrind tells you nothing at all
> >>> useful.
> >>
> >> I ran then and it passed.
> >> For record
> >>
> >>    LD_LIBRARY_PATH=bin/shared/ valgrind -v bin/ldb_msg_test
> >>
> >> or running everything with valgrind
> >>
> >>    valgrind -v --trace-children=yes make check
> >>
> >> I was not running it from samba git but after extracting ldb from tarball
> >> + patching source code.
> >>
> >>>> Otherwise looks good to me. But it would be good to fix also comment
> >>>> from
> >>>> different mail in this thread about remove_dupes.
> >>>>
> >>>> Thank you very much for unit tests. They helped us to find bug :-)
> >>>> I really appreciate it.
> >>>
> >>> You are welcome.
> >>
> >> BTW I checked unit test one more time and you ere right.
> >> It was caused by "el3->num_values == 0" because el3 was changed
> >> after 1st initialisation.
> >>
> >> So my patch is not needed; your solution is more elegant
> >> and my patch can be dropped from patch set.
> >
> >I think your first patch is still correct. It makes that loop more
> >succinct and safe at no cost.
> >
> >I have added a few more asserts about element size in the tests.
>
> Thank you.
>
> ACK
>
> LS

RB+

Pushed to autobuild!

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Loading...