Full_Name: Tom Riddle Version: 2.1.22 OS: Fedora Core 1.0 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (216.204.182.62) ldap_back_search() does not handle the case where base == NULL: #ifdef ENABLE_REWRITE switch ( rewrite_session( li->rwinfo, "searchBase", base->bv_val, conn, &mbase.bv_val ) ) { case REWRITE_REGEXEC_OK: if ( mbase.bv_val == NULL ) { mbase = *base; } #ifdef NEW_LOGGING LDAP_LOG( BACK_LDAP, DETAIL1, "[rw] searchBase: \"%s\" -> \"%s\"\n", base->bv_val, mbase.bv_val, 0 ); #else /* !NEW_LOGGING */ Debug( LDAP_DEBUG_ARGS, "rw> searchBase: \"%s\" -> \"%s\"\n%s", base->bv_val, mbase.bv_val, "" ); #endif /* !NEW_LOGGING */
I assumed "base" would always be set; however, code has been completely reworked in HEAD. 2.1.X will need a dedicated fix, I'll take care of it asap. Thanks, p. > Full_Name: Tom Riddle > Version: 2.1.22 > OS: Fedora Core 1.0 > URL: ftp://ftp.openldap.org/incoming/ > Submission from: (NULL) (216.204.182.62) > > > > ldap_back_search() does not handle the case where base == NULL: > > #ifdef ENABLE_REWRITE > switch ( rewrite_session( li->rwinfo, "searchBase", > base->bv_val, conn, &mbase.bv_val ) ) { > case REWRITE_REGEXEC_OK: > if ( mbase.bv_val == NULL ) { > mbase = *base; > } > #ifdef NEW_LOGGING > LDAP_LOG( BACK_LDAP, DETAIL1, > "[rw] searchBase: \"%s\" -> \"%s\"\n", > base->bv_val, mbase.bv_val, 0 ); > #else /* !NEW_LOGGING */ > Debug( LDAP_DEBUG_ARGS, "rw> searchBase: \"%s\" -> > \"%s\"\n%s", > base->bv_val, mbase.bv_val, "" ); > #endif /* !NEW_LOGGING */ -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
Pierangelo Masarati wrote: >I assumed "base" would always be set; however, > slap_sasl_match(), slap_sasl2dn() and two places in sasl.c call it explicitly with NULL: vvvvv (*be->be_search)( be, conn, &op, NULL, &dn, scope, LDAP_DEREF_NEVER, 1, 0, filter, NULL, NULL, 1 ); I tried applying the attached patch, but there are many other places further down that make this same assumption. -- Tom Riddle HighStreet Networks www.highstreetnetworks.com
I suggest this patch for OPENLDAP_REL_ENG_2_1 and next 2.1 release. ftp://ftp.openldap.org/incoming/its-2825.patch Ando.
Pierangelo Masarati wrote: >I suggest this patch for OPENLDAP_REL_ENG_2_1 and next 2.1 release. > >ftp://ftp.openldap.org/incoming/its-2825.patch > >Ando. > > Why do you say that it should not happen ?? Is it not allowed to do SASL bind on subordinate back-ldap ? Am I doing something wrong ? What about filterstr ? it too is passed as NULL by slap_sasl2dn() and derefenenced unconditionally. -- Tom Riddle HighStreet Networks www.highstreetnetworks.com
Looks like the sasl code assumed only nbase was ever needed by the backends. The fix is to provide a non-NULL base in sasl.c. I'll take care of this. -- Howard Chu Chief Architect, Symas Corp. Director, Highland Sun http://www.symas.com http://highlandsun.com/hyc Symas: Premier OpenSource Development and Support > -----Original Message----- > From: owner-openldap-bugs@OpenLDAP.org > [mailto:owner-openldap-bugs@OpenLDAP.org]On Behalf Of > ftr@highstreetnetworks.com > Sent: Thursday, November 13, 2003 6:34 AM > To: openldap-its@OpenLDAP.org > Subject: Re: segfault in ldap_back_search() with ENABLE_REWRITE > (ITS#2825) > > > This is a multi-part message in MIME format. > --------------040507020600070408090206 > Content-Type: text/plain; charset=ISO-8859-1; format=flowed > Content-Transfer-Encoding: 7bit > > Pierangelo Masarati wrote: > > >I assumed "base" would always be set; however, > > > slap_sasl_match(), slap_sasl2dn() and two places in sasl.c call it > explicitly with NULL: > vvvvv > (*be->be_search)( be, conn, &op, NULL, &dn, > scope, LDAP_DEREF_NEVER, 1, 0, > filter, NULL, NULL, 1 ); > > I tried applying the attached patch, but there are many other places > further down that make this same assumption. > > -- > Tom Riddle > HighStreet Networks > www.highstreetnetworks.com > > > --------------040507020600070408090206 > Content-Type: text/plain; > name="hsn1.patch" > Content-Transfer-Encoding: 7bit > Content-Disposition: inline; > filename="hsn1.patch" > > --- openldap-2.1.22-orig/servers/slapd/back-ldap/search.c > 2003-03-12 17:27:57.000000000 -0500 > +++ openldap-2.1.22/servers/slapd/back-ldap/search.c > 2003-11-12 09:53:32.000000000 -0500 > @@ -157,18 +157,18 @@ > */ > #ifdef ENABLE_REWRITE > switch ( rewrite_session( li->rwinfo, "searchBase", > - base->bv_val, conn, &mbase.bv_val ) ) { > + base ? base->bv_val : NULL, conn, > &mbase.bv_val ) ) { > case REWRITE_REGEXEC_OK: > - if ( mbase.bv_val == NULL ) { > + if ( mbase.bv_val == NULL && base) { > mbase = *base; > } > #ifdef NEW_LOGGING > LDAP_LOG( BACK_LDAP, DETAIL1, > "[rw] searchBase: \"%s\" -> \"%s\"\n", > - base->bv_val, mbase.bv_val, 0 ); > + base ? base->bv_val : "", mbase.bv_val, 0 ); > #else /* !NEW_LOGGING */ > Debug( LDAP_DEBUG_ARGS, "rw> searchBase: \"%s\" > -> \"%s\"\n%s", > - base->bv_val, mbase.bv_val, "" ); > + base ? base->bv_val : "", mbase.bv_val, "" ); > #endif /* !NEW_LOGGING */ > break; > > > --------------040507020600070408090206-- >
> Pierangelo Masarati wrote: > >>I suggest this patch for OPENLDAP_REL_ENG_2_1 and next 2.1 release. >> >>ftp://ftp.openldap.org/incoming/its-2825.patch >> >>Ando. >> >> > Why do you say that it should not happen ?? > Is it not allowed to do SASL bind on subordinate back-ldap ? Am I doing > something wrong ? In my opinion, an empty DN is represented by a berval with 0 length pointing to a "". Everything else is wrong. Besides, it's unclear to me what a base has to do with sasl auth{c|z} > What about filterstr ? it too is passed as NULL by slap_sasl2dn() and > derefenenced unconditionally. where? In any case, one of the reasons of using bervals instead of char*s was exactly to avoid having to check for NULL pointers and calling strlen() all the times. Now all struct berval*s should be assert()ed, and a null berval'ed field should be tested as bv->bv_val == NULL while an empty one should be tested as bv->bv_len != 0. In my opinion, a NULL base is totally meaningless; an empty base makes sense. p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
changed notes changed state Open to Test moved from Incoming to Software Bugs
> where? In any case, one of the reasons of using bervals > instead of char*s was exactly to avoid having to check for > NULL pointers and calling strlen() all the times. Now > all struct berval*s should be assert()ed, and a null > berval'ed field should be tested as bv->bv_val == NULL while > an empty one should be tested as bv->bv_len != 0. In my > opinion, a NULL base is totally meaningless; an empty base > makes sense. Ando, Is it your point that we should change bv->bv_val == NULL to bv->bv_len == 0 along with the related codes sometime ? Currently, berval library codes are also checking bv->bv_val when it could check bv->bv_len, and there are also some cases where bv->bv_len is left uninitialized, making its user codes unable to rely on bv->bv_len. - Jong
>> where? In any case, one of the reasons of using bervals >> instead of char*s was exactly to avoid having to check for >> NULL pointers and calling strlen() all the times. Now >> all struct berval*s should be assert()ed, and a null >> berval'ed field should be tested as bv->bv_val == NULL while >> an empty one should be tested as bv->bv_len != 0. In my >> opinion, a NULL base is totally meaningless; an empty base >> makes sense. > > Ando, > Is it your point that we should change bv->bv_val == NULL > to bv->bv_len == 0 along with the related codes sometime ? > Currently, berval library codes are also checking bv->bv_val > when it could check bv->bv_len, and there are also some > cases where bv->bv_len is left uninitialized, making its user > codes unable to rely on bv->bv_len. > - Jong I wouldn't be so strict; however, the two things have different meanings: bv_val == NULL means there is no value; bv_len == 0 means the value is empty, but it is a legal value. So bv_len == 0 should be allowed to coexist with bv_val != NULL, e.g. in the rootDSE, while bv_val == NULL should imply that bv_len must be ignored. The use of either form should be consistent with the meaning of each berval. For instance, a search base of { 0, NULL } has little meaning, while a search base of { 0, "" } makes sense. This has nothing to do with the original posting, because in that case the berval pointer was null; I'd consider this occurrence undesirable, because it defeats the purpose of using bervals as arguments. Note that all this discussion is outdated since the ABI change. p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
I misinterpreted your previous posting. Thanks. - Jong > I wouldn't be so strict; however, the two things have different > meanings: bv_val == NULL means there is no value; bv_len == 0 > means the value is empty, but it is a legal value. So bv_len == 0 > should be allowed to coexist with bv_val != NULL, e.g. in the > rootDSE, while bv_val == NULL should imply that bv_len must be > ignored. > > The use of either form should be consistent with the meaning > of each berval. For instance, a search base of { 0, NULL } > has little meaning, while a search base of { 0, "" } makes > sense. > > This has nothing to do with the original posting, because > in that case the berval pointer was null; I'd consider this > occurrence undesirable, because it defeats the purpose of > using bervals as arguments. Note that all this discussion > is outdated since the ABI change. > > p. > > -- > Pierangelo Masarati > mailto:pierangelo.masarati@sys-net.it
changed state Test to Release
changed state Release to Closed
moved from Software Bugs to Archive.Software Bugs
fixed in HEAD, RE21