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! ==29701== Invalid write of size 1 ==29701== at 0x4A089AF: strcpy (vg_replace_strmem.c:458) ==29701== by 0x45ECC6: slap_bv2undef_ad (ad.c:772) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==29701== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29701== by 0x406C55: main (main.c:988) ==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd ==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296) ==29701== by 0x549A18: ber_memalloc_x (memory.c:228) ==29701== by 0x43901A: ch_malloc (ch_malloc.c:54) =979701== by 0x45EC93: slap_bv2undef_ad (ad.c:764) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==29701== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29701== by 0x406C55: main (main.c:988) ==29701== ==29701== Invalid read of size 1 ==29701== at 0x53BE9F: ldap_pvt_str2upper (string.c:116) ==29701== by 0x45ECCF: slap_bv2undef_ad (ad.c:775) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==291%1== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29701== by 0x406C55: main (main.c:988) ==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd ==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296) ==29701== by 0x549A18: ber_memalloc_x (memory.c:228) ==29701== by 0x43901A: ch_malloc (ch_malloc.c:54) ==29701== by 0x45EC93: slap_bv2undef_ad (ad.c:764) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==29701== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29703D%3= by 0x406C55: main (main.c:988) ==29701== Invalid read of size 1 ==29701== at 0x30E184812C: vfprintf (in /lib64/libc-2.12.so) ==29701== by 0x30E186FA51: vsnprintf (in /lib64/libc-2.12.so) ==29701== by 0x5498DA: lutil_debug (debug.c:67) ==29701== by 0x45ED34: slap_bv2undef_a(a8ad.c:785) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==29701== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29701== by 0x406C55: main (main.c:988) ==29701== Address 0x57d9187 is 0 bytes after a block of size 71 alloc'd ==29701== at 0x4A0720A: malloc (vg_replace_malloc.c:296) ==29701== by 0x549A18: ber_memalloc_x (memory.c:228) ==29701== by 0x43901A: ch_malloc (ch_malloc.c:54) ==29701== by 0x45EC93: slap_bv2undef_ad (ad.c:764) ==29701== by 0x4C3649: mdb_ad_read (attr.c:575) ==29701== by 0x4949D7: mdb_db_open (init.c:278) ==29701== by 0x482D86: over_db_open (backover.c:149) ==29701== by 0x42DA58: backend_startup_one (backend.c:224) ==29701== by 0x42DD22: backend_startup (backend.c:325) ==29701== by 0x44ABB0: slap_startup (init.c:219) ==29701== by 0x406C55: main (main.c:988)
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/
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.
Леонид Юрьев wrote: > (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 And the answer is no, back-mdb/attr.c mdb_ad_get() doesn't store the trailing NUL byte, > diff --git a/servers/slapd/ad.c b/servers/slapd/ad.c > index 246b900..fd60483 100644 > --- a/servers/slapd/ad.c > +++ b/servers/slapd/ad.c > @@ -118,7 +118,7 @@ AttributeDescription * ad_find_tags( > for (ad = type->sat_ad; ad; ad=ad->ad_next) > { > if (ad->ad_tags.bv_len == tags->bv_len && > - !strcasecmp(ad->ad_tags.bv_val, tags->bv_val)) > + !strncasecmp( ad->ad_tags.bv_val, tags->bv_val, ad->ad_tags.bv_len )) > break; > } Unnecessary. > ldap_pvt_thread_mutex_unlock( &type->sat_ad_mutex ); > @@ -742,14 +742,13 @@ int slap_bv2undef_ad( > /* use the appropriate type */ > if ( flags & SLAP_AD_PROXIED ) { > at = slap_schema.si_at_proxied; > - > } else { > at = slap_schema.si_at_undefined; > } > > for( desc = at->sat_ad; desc; desc=desc->ad_next ) { > if( desc->ad_cname.bv_len == bv->bv_len && > - !strcasecmp( desc->ad_cname.bv_val, bv->bv_val ) ) > + !strncasecmp( desc->ad_cname.bv_val, bv->bv_val, desc->ad_cname.bv_len) ) Unnecessary. We've already checked that the two lengths are equal; even if bv->bv_val is non-terminated the compare will stop because desc->ad_cname is terminated. > { > break; > } > @@ -769,7 +768,8 @@ int slap_bv2undef_ad( > > desc->ad_cname.bv_len = bv->bv_len; > desc->ad_cname.bv_val = (char *)(desc+1); > - strcpy(desc->ad_cname.bv_val, bv->bv_val); > + strncpy( desc->ad_cname.bv_val, bv->bv_val, desc->ad_cname.bv_len ) > + [desc->ad_cname.bv_len] = '\0'; This is a valid fix. > > /* canonical to upper case */ > ldap_pvt_str2upper( desc->ad_cname.bv_val ); > @@ -806,9 +806,10 @@ slap_bv2tmp_ad( > slap_sl_mfuncs.bmf_malloc( sizeof(AttributeDescription) + > bv->bv_len + 1, memctx ); > > - ad->ad_cname.bv_val = (char *)(ad+1); > - strncpy( ad->ad_cname.bv_val, bv->bv_val, bv->bv_len+1 ); > ad->ad_cname.bv_len = bv->bv_len; > + ad->ad_cname.bv_val = (char *)(ad+1); > + strncpy( ad->ad_cname.bv_val, bv->bv_val, ad->ad_cname.bv_len) > + [ad->ad_cname.bv_len] = '\0'; > ad->ad_flags = SLAP_DESC_TEMPORARY; > ad->ad_type = slap_schema.si_at_undefined; > Unnecessary. > @@ -887,7 +888,7 @@ an_find( > > for ( ; a->an_name.bv_val; a++ ) { > if ( a->an_name.bv_len != s->bv_len) continue; > - if ( strcasecmp( s->bv_val, a->an_name.bv_val ) == 0 ) { > + if ( strncasecmp( s->bv_val, a->an_name.bv_val, s->bv_len ) == 0 ) { > return( 1 ); > } > } Unnecessary. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
changed notes changed state Open to Test moved from Incoming to Software Bugs
changed notes changed state Test to Release
fixed in master fixed in RE25 fixed in RE24
changed notes changed state Release to Closed