Re: (ITS#7995) of-by-one error in schema

[leo@yuriev.ru]

--001a11c27ac2dcc7420509ced83c
Content-Type: text/plain; charset=UTF-8

2014-12-09 18:57 GMT+03:00 Howard Chu <hyc@symas.com>:
> leo@yuriev.ru wrote:
>>
>> Full_Name: Leonid Yuriev
>> Version: 2.4.40
>> OS: RHEL7
>> URL: ftp://ftp.openldap.org/incoming/
>> Submission from: (NULL) (31.130.36.33)
>>
>>
>> In some cases (presumably when a database contains more attributes than
>> defined
>> in the scheme) a heap error may be detected at stop of slapd.
>>
>> Below is the result of attempts to find a bug(s) with Valgrind.
>> It is enough to corrupt a malloc's heap!
>
>
> Please provide a test case. Unable to reproduce this on a local database
> with commented out schema. I get:
>
> violino:~/OD/o24/tests> valgrind ../servers/slapd/slapd -Tc -f
> /tmp/testr/slapd.1.conf > /tmp/out
> ==25499== Memcheck, a memory error detector
> ==25499== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
> ==25499== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright
> info
> ==25499== Command: ../servers/slapd/slapd -Tc -f /tmp/testr/slapd.1.conf
> ==25499==
> 54871b4f UNKNOWN attributeDescription "HOMEPOSTALADDRESS" inserted.
> 54871b4f UNKNOWN attributeDescription "DRINK" inserted.
> 54871b4f UNKNOWN attributeDescription "HOMEPHONE" inserted.
> 54871b4f UNKNOWN attributeDescription "PAGER" inserted.
> ==25499==
> ==25499== HEAP SUMMARY:
> ==25499==     in use at exit: 2,746 bytes in 78 blocks
> ==25499==   total heap usage: 10,335 allocs, 10,257 frees, 1,649,999 bytes
> allocated
> ==25499==
> ==25499== LEAK SUMMARY:
> ==25499==    definitely lost: 0 bytes in 0 blocks
> ==25499==    indirectly lost: 0 bytes in 0 blocks
> ==25499==      possibly lost: 0 bytes in 0 blocks
> ==25499==    still reachable: 2,746 bytes in 78 blocks
> ==25499==         suppressed: 0 bytes in 0 blocks
> ==25499== Rerun with --leak-check=full to see details of leaked memory
> ==25499==
> ==25499== For counts of detected and suppressed errors, rerun with: -v
> ==25499== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
>
> --
>   -- Howard Chu
>   CTO, Symas Corp.           http://www.symas.com
>   Director, Highland Sun     http://highlandsun.com/hyc/
>   Chief Architect, OpenLDAP  http://www.openldap.org/project/

(1)
Unfortunately it was reproduced only in production-like environment,
therefore a testcase or complete info is not available yet.

(2)
But let see to lines 570-575 of mdb-back/attr.c and lines 764-772 of slapd/ad.c:
- does the mdb-value include a NUL byte =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/back-mdb/attr.c#L570?
- if "YES" then "+1" at slapd/ad.c:764 is wrong =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L764
- but if "NO" then "strcpy()" at slapd/ad.c:772 (and more) is wrong =
https://github.com/leo-yuriev/openldap-lmdb-challenge/blob/2.4-devel/servers/slapd/ad.c#L772

(3)
Next, I had added a couple of assert-check into mdb-back/attr.c
and right away got "slapcat: attr.c:573: mdb_ad_read: Assertion
`bdata.bv_val[bdata.bv_len-1] == 0' failed"

diff --git a/servers/slapd/back-mdb/attr.c b/servers/slapd/back-mdb/attr.c
index 07b64a6..7979b9d 100644
--- a/servers/slapd/back-mdb/attr.c
+++ b/servers/slapd/back-mdb/attr.c
@@ -569,6 +569,8 @@ int mdb_ad_read( struct mdb_info *mdb, MDB_txn *txn )
        while ( rc == MDB_SUCCESS ) {
                bdata.bv_len = data.mv_size;
                bdata.bv_val = data.mv_data;
+               assert(bdata.bv_len > 0);
+               assert(bdata.bv_val[bdata.bv_len-1] == 0);
                ad = NULL;
                rc = slap_bv2ad( &bdata, &ad, &text );
                if ( rc ) {

(4)
Ok, this is a off-by-one bug.
Then I looked for "sizeof(AttributeDescription)" and has found only 3
places in slapd/ad.c
... and fix a few places of str-calls to n-form.

Patch is attached, please review it.

Leonid.

---

The attached files is derived from OpenLDAP Software. All of the modifications
to OpenLDAP Software represented in the following patch(es) were developed by
Peter-Service LLC, Moscow, Russia. Peter-Service LLC has not assigned rights
and/or interest in this work to any party. I, Leonid Yuriev am authorized by
Peter-Service LLC, my employer, to release this work under the following terms.

Peter-Service LLC hereby places the following modifications to OpenLDAP Software
(and only these modifications) into the public domain. Hence, these
modifications may be freely used and/or redistributed for any purpose
with or without attribution and/or other notice.

--001a11c27ac2dcc7420509ced83c
Content-Type: text/x-patch; charset=US-ASCII; name="its7995.patch"
Content-Disposition: attachment; filename="its7995.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_i3hr0n7l0

Y29tbWl0IDkxMzk4NmZjZGI3NGFlMjcyNjk5ZjI0NzA0MzBmMGRjNmM0NTkxM2YKQXV0aG9yOiBM
ZW8gWXVyaWV2IDxsZW9AeXVyaWV2LnJ1PgpEYXRlOiAgIDIwMTQtMTItMDkgMjM6Mzg6NDQgKzAz
MDAKCiAgICBJVFMjNzk5NSBoZWFwIGNvcnJ1cHRpb24gYW5kIG9mZi1ieS1vbmUgZXJyb3JzIGlu
IGF0dHJpYnV0ZS1kZXNjcmlwdGlvbiBoYW5kbGluZy4KCmRpZmYgLS1naXQgYS9zZXJ2ZXJzL3Ns
YXBkL2FkLmMgYi9zZXJ2ZXJzL3NsYXBkL2FkLmMKaW5kZXggMjQ2YjkwMC4uZmQ2MDQ4MyAxMDA2
NDQKLS0tIGEvc2VydmVycy9zbGFwZC9hZC5jCisrKyBiL3NlcnZlcnMvc2xhcGQvYWQuYwpAQCAt
MTE4LDcgKzExOCw3IEBAIEF0dHJpYnV0ZURlc2NyaXB0aW9uICogYWRfZmluZF90YWdzKAogCWZv
ciAoYWQgPSB0eXBlLT5zYXRfYWQ7IGFkOyBhZD1hZC0+YWRfbmV4dCkKIAl7CiAJCWlmIChhZC0+
YWRfdGFncy5idl9sZW4gPT0gdGFncy0+YnZfbGVuICYmCi0JCQkhc3RyY2FzZWNtcChhZC0+YWRf
dGFncy5idl92YWwsIHRhZ3MtPmJ2X3ZhbCkpCisJCQkhc3RybmNhc2VjbXAoIGFkLT5hZF90YWdz
LmJ2X3ZhbCwgdGFncy0+YnZfdmFsLCBhZC0+YWRfdGFncy5idl9sZW4gKSkKIAkJCWJyZWFrOwog
CX0KIAlsZGFwX3B2dF90aHJlYWRfbXV0ZXhfdW5sb2NrKCAmdHlwZS0+c2F0X2FkX211dGV4ICk7
CkBAIC03NDIsMTQgKzc0MiwxMyBAQCBpbnQgc2xhcF9idjJ1bmRlZl9hZCgKIAkvKiB1c2UgdGhl
IGFwcHJvcHJpYXRlIHR5cGUgKi8KIAlpZiAoIGZsYWdzICYgU0xBUF9BRF9QUk9YSUVEICkgewog
CQlhdCA9IHNsYXBfc2NoZW1hLnNpX2F0X3Byb3hpZWQ7Ci0KIAl9IGVsc2UgewogCQlhdCA9IHNs
YXBfc2NoZW1hLnNpX2F0X3VuZGVmaW5lZDsKIAl9CiAKIAlmb3IoIGRlc2MgPSBhdC0+c2F0X2Fk
OyBkZXNjOyBkZXNjPWRlc2MtPmFkX25leHQgKSB7CiAJCWlmKCBkZXNjLT5hZF9jbmFtZS5idl9s
ZW4gPT0gYnYtPmJ2X2xlbiAmJgotCQkgICAgIXN0cmNhc2VjbXAoIGRlc2MtPmFkX2NuYW1lLmJ2
X3ZhbCwgYnYtPmJ2X3ZhbCApICkKKwkJICAgICFzdHJuY2FzZWNtcCggZGVzYy0+YWRfY25hbWUu
YnZfdmFsLCBidi0+YnZfdmFsLCBkZXNjLT5hZF9jbmFtZS5idl9sZW4pICkKIAkJewogCQkgICAg
CWJyZWFrOwogCQl9CkBAIC03NjksNyArNzY4LDggQEAgaW50IHNsYXBfYnYydW5kZWZfYWQoCiAK
IAkJZGVzYy0+YWRfY25hbWUuYnZfbGVuID0gYnYtPmJ2X2xlbjsKIAkJZGVzYy0+YWRfY25hbWUu
YnZfdmFsID0gKGNoYXIgKikoZGVzYysxKTsKLQkJc3RyY3B5KGRlc2MtPmFkX2NuYW1lLmJ2X3Zh
bCwgYnYtPmJ2X3ZhbCk7CisJCXN0cm5jcHkoIGRlc2MtPmFkX2NuYW1lLmJ2X3ZhbCwgYnYtPmJ2
X3ZhbCwgZGVzYy0+YWRfY25hbWUuYnZfbGVuICkKKwkJCVtkZXNjLT5hZF9jbmFtZS5idl9sZW5d
ID0gJ1wwJzsKIAogCQkvKiBjYW5vbmljYWwgdG8gdXBwZXIgY2FzZSAqLwogCQlsZGFwX3B2dF9z
dHIydXBwZXIoIGRlc2MtPmFkX2NuYW1lLmJ2X3ZhbCApOwpAQCAtODA2LDkgKzgwNiwxMCBAQCBz
bGFwX2J2MnRtcF9hZCgKIAkJIHNsYXBfc2xfbWZ1bmNzLmJtZl9tYWxsb2MoIHNpemVvZihBdHRy
aWJ1dGVEZXNjcmlwdGlvbikgKwogCQkJYnYtPmJ2X2xlbiArIDEsIG1lbWN0eCApOwogCi0JYWQt
PmFkX2NuYW1lLmJ2X3ZhbCA9IChjaGFyICopKGFkKzEpOwotCXN0cm5jcHkoIGFkLT5hZF9jbmFt
ZS5idl92YWwsIGJ2LT5idl92YWwsIGJ2LT5idl9sZW4rMSApOwogCWFkLT5hZF9jbmFtZS5idl9s
ZW4gPSBidi0+YnZfbGVuOworCWFkLT5hZF9jbmFtZS5idl92YWwgPSAoY2hhciAqKShhZCsxKTsK
KwlzdHJuY3B5KCBhZC0+YWRfY25hbWUuYnZfdmFsLCBidi0+YnZfdmFsLCBhZC0+YWRfY25hbWUu
YnZfbGVuKQorCQlbYWQtPmFkX2NuYW1lLmJ2X2xlbl0gPSAnXDAnOwogCWFkLT5hZF9mbGFncyA9
IFNMQVBfREVTQ19URU1QT1JBUlk7CiAJYWQtPmFkX3R5cGUgPSBzbGFwX3NjaGVtYS5zaV9hdF91
bmRlZmluZWQ7CiAKQEAgLTg4Nyw3ICs4ODgsNyBAQCBhbl9maW5kKAogCiAJZm9yICggOyBhLT5h
bl9uYW1lLmJ2X3ZhbDsgYSsrICkgewogCQlpZiAoIGEtPmFuX25hbWUuYnZfbGVuICE9IHMtPmJ2
X2xlbikgY29udGludWU7Ci0JCWlmICggc3RyY2FzZWNtcCggcy0+YnZfdmFsLCBhLT5hbl9uYW1l
LmJ2X3ZhbCApID09IDAgKSB7CisJCWlmICggc3RybmNhc2VjbXAoIHMtPmJ2X3ZhbCwgYS0+YW5f
bmFtZS5idl92YWwsIHMtPmJ2X2xlbiApID09IDAgKSB7CiAJCQlyZXR1cm4oIDEgKTsKIAkJfQog
CX0K
--001a11c27ac2dcc7420509ced83c--