openldap-bugs

openldap-bugs@openldap.org

1 participants
20964 discussions

Re: (ITS#6504) Corrupted control value when using pagedresults with subordinate *ldap* database
by masarati＠aero.polimi.it 05 Apr '10

05 Apr '10

>> Well, I've just fixed it. When changing database, I modified the paged >> results control value by setting the page size to pagesize - >> rs->sr_nentries, with an empty cookie. I'll commit tomorrow, as now I >> only have http access. > > Speedy patch - thank you! Just tried it out, and it works great for the > issue I reported. > > Testing further shows a limitation in crossing databases though. A > search that spans over two glued ldap databases, returns n < pagesize > results from the first, and continues over to the second, returning > pagesize-n results from it, making pagesize results all together. So > far, so good. > > However, the subsequent search with the returned pagedresults cookie > just starts a new search over, returning the same set of results. So it > works fine for one page max. > > But it doesn't look simple to solve this issue, unless backglue starts > recording pagedresults cookies, etc... > > Anyway, great improvement on not sending back free'd cookie :-) I see the issue, and it seems to be not related to my patch. Don't know how to fix it (yet). It'll probably need a separate ITS. p.

1 0

Re: (ITS#6504) Corrupted control value when using pagedresults with subordinate *ldap* database
by jonathan＠phillipoux.net 05 Apr '10

05 Apr '10

Le 02/04/2010 06:44, masarati(a)aero.polimi.it a écrit : >> masarati(a)aero.polimi.it wrote: >>>> masarati(a)aero.polimi.it wrote: >>>>>> Also, I note that the glued paged response seems to work incorrectly. >>>>>> I >>>>>> made a simple test system, where the root database contains exactly >>>>>> one >>>>>> entry (the suffix) and a back-ldap is glued on top. If I request >>>>>> entries >>>>>> with a page size of 2, searching the suffix return 3 entries; >>>>>> subsequent >>>>>> searches return 2 entries from the proxy. I haven't figured out yet >>>>>> where >>>>>> the issue is. >>>>> >>>>> I figured out how to fix the issue you reported. It is related to the >>>>> fact that slapd internally assumes that ldctl_oid values are constant >>>>> strings, so it doesn't duplicate nor free them. However, control OIDs >>>>> returned by back-ldap (and friends) have been decoded from the wire by >>>>> the >>>>> client library, so they are actually malloc'ed. That's why backglue >>>>> ends >>>>> up with a dangling pointer. >>>>> >>>>> I'm fixing it by having backglue duplicate (and free) control OIDs >>>>> consistently. Another option would be to replace those values with >>>>> constant strings for known control values. This would prevent gluing >>>>> for >>>>> unknown proxied controls. >>>> >>>> Or just tmpalloc them and don't bother to clean them up. >>> >>> Yes, I've reworked all the temporary controls allocation using tmpalloc. >>> Too bad OIDs are char* and not bervals. >>> >>> With respect to the other issue, to honor the requested pagesize we'd >>> need >>> to intercept pagedresult requests, modify the page size in the request >>> when a page crosses two databases, requesting the original page size >>> minus >>> entries already returned. I wonder whether it's worth the effort, >>> though. >> >> IMO pagedresults should be a (global) overlay, and backends shouldn't do >> anything about it. Note that the sssvlv overlay already intercepts >> pagedresults, it's impossible to handle things correctly otherwise. > > Well, I've just fixed it. When changing database, I modified the paged > results control value by setting the page size to pagesize - > rs->sr_nentries, with an empty cookie. I'll commit tomorrow, as now I > only have http access. Speedy patch - thank you! Just tried it out, and it works great for the issue I reported. Testing further shows a limitation in crossing databases though. A search that spans over two glued ldap databases, returns n < pagesize results from the first, and continues over to the second, returning pagesize-n results from it, making pagesize results all together. So far, so good. However, the subsequent search with the returned pagedresults cookie just starts a new search over, returning the same set of results. So it works fine for one page max. But it doesn't look simple to solve this issue, unless backglue starts recording pagedresults cookies, etc... Anyway, great improvement on not sending back free'd cookie :-) -- -------------------------------------------------------------- Jonathan Clarke - jonathan(a)phillipoux.net -------------------------------------------------------------- Ldap Synchronization Connector (LSC) - http://lsc-project.org --------------------------------------------------------------

1 0

RE: (ITS#6505) ldap_search_* functions missing const qualifiers
by EHASZLA＠transunion.com 05 Apr '10

05 Apr '10

>From: masarati(a)aero.polimi.it [mailto:masarati@aero.polimi.it] > >> hmm... I'm not quite sure how to attach a file to a ticket through the >> web interface, > >In general, upload to ftp.openldap.org according to contribution >instructions, and post a message with the URL; the way you provided it >makes it pretty unusable. Fwiw, I uploaded the two patches as ehaszla-ldap.h-20100405.patch and ehaszla-search.c-20100405.patch eric

1 0

RE: (ITS#6505) ldap_search_* functions missing const qualifiers
by masarati＠aero.polimi.it 05 Apr '10

05 Apr '10

> Anyway, if the change doesn't get accepted it won't be the end of the > world, but obviously I'm hoping you find the above convincing. Just to clarify: if I were to design the API from scratch, I'd CONST those args... so I agree with your suggestion. p.

1 0

RE: (ITS#6505) ldap_search_* functions missing const qualifiers
by EHASZLA＠transunion.com 04 Apr '10

04 Apr '10

This is a multi-part message in MIME format. ------_=_NextPart_001_01CAD483.E1DB174F Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable -----Original Message----- >From: masarati(a)aero.polimi.it [mailto:masarati@aero.polimi.it] >In general, upload to ftp.openldap.org according to contribution >instructions, and post a message with the URL; the way you provided it >makes it pretty unusable. I saw the instructions about using the ftp site some time after I sent = the patches. Sorry. The patch was just the obvious, mechanical change to add const to the attrs parameter in the functions that have it. Maybe a reminder to keep reading the rest of the page in the section on "patches" would be helpful. I got to the part that started talking = about attribution of large changes, and assumed the rest of the page wasn't relevant to what I was trying to do. >However, I think your patch will not be considered, although in = principle >perfectly acceptable, because an API change of this type would probably >break a lot of code out there, without significant advantages. Well, a couple of observations: 1) At least one other ldap implementation already does this. 2) I consider being able to turn on the compiler warnings that tell you = about parameters that might be changed when do don't expect them to be = a significant advantage. For many of the source trees I've worked on, = doing so (and treating warnings as errors, to force them to be fixed)=20 has helped uncover a variety of bugs. 3) If you're not being pedantic about warnings, and causing them to be=20 treated as errors, this API change just results in an extra warning,=20 which wouldn't actually break any code. 4) Adding "const" effectively and simply documents the fact that those=20 parameters aren't going to be changed. If the API doesn't say so, it would be nice if that was at least explicitly mentions in the man = page. At the moment, I had to go read an analyze the ldap code to figure that out. I'd rather spend my time working on my own code instead, = and be able to simply use the great library you guys have created. :) Anyway, if the change doesn't get accepted it won't be the end of the world, but obviously I'm hoping you find the above convincing. eric ------_=_NextPart_001_01CAD483.E1DB174F Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN"> <HTML> <HEAD> <META HTTP-EQUIV=3D"Content-Type" CONTENT=3D"text/html; = charset=3Diso-8859-1"> <META NAME=3D"Generator" CONTENT=3D"MS Exchange Server version = 6.5.7654.12"> <TITLE>RE: (ITS#6505) ldap_search_* functions missing const = qualifiers</TITLE> </HEAD> <BODY>  -----Original Message----- >From: masarati(a)aero.polimi.it [<A = HREF=3D"mailto:masarati@aero.polimi.it">mailto:masarati@aero.polimi.it</A= >] >In general, upload to ftp.openldap.org according to contribution >instructions, and post a message with the URL; the way you provided = it >makes it pretty unusable.   I saw the instructions about using the ftp site some time after I = sent the patches.  Sorry.  The patch was just the obvious, = mechanical change to add const to the attrs parameter in the functions that have it. Maybe a reminder to keep reading the rest of the page in the section = on "patches" would be helpful.  I got to the part that = started talking about attribution of large changes, and assumed the rest of the page = wasn't relevant to what I was trying to do. >However, I think your patch will not be considered, although in = principle >perfectly acceptable, because an API change of this type would = probably >break a lot of code out there, without significant advantages. Well, a couple of observations:  1) At least one other ldap implementation already does this.  2) I consider being able to turn on the compiler warnings that = tell you   about parameters that might be changed when do don't expect them = to be   a significant advantage.  For many of the source trees I've = worked on,   doing so (and treating warnings as errors, to force them to be = fixed)   has helped uncover a variety of bugs.  3) If you're not being pedantic about warnings, and causing them = to be   treated as errors, this API change just results in an extra = warning,   which wouldn't actually break any code.  4) Adding "const" effectively and simply documents the = fact that those   parameters aren't going to be changed.  If the API doesn't = say so, it   would be nice if that was at least explicitly mentions in the man = page.   At the moment, I had to go read an analyze the ldap code to = figure   that out.  I'd rather spend my time working on my own code = instead, and   be able to simply use the great library you guys have created. = :) Anyway, if the change doesn't get accepted it won't be the end of = the  world, but obviously I'm hoping you find the above convincing. eric </BODY> </HTML> ------_=_NextPart_001_01CAD483.E1DB174F--

1 0

Re: (ITS#6505) ldap_search_* functions missing const qualifiers
by masarati＠aero.polimi.it 02 Apr '10

02 Apr '10

> hmm... I'm not quite sure how to attach a file to a ticket through the > web interface, In general, upload to ftp.openldap.org according to contribution instructions, and post a message with the URL; the way you provided it makes it pretty unusable. However, I think your patch will not be considered, although in principle perfectly acceptable, because an API change of this type would probably break a lot of code out there, without significant advantages. p.

1 0

Re: (ITS#6505) ldap_search_* functions missing const qualifiers
by EHASZLA＠transunion.com 02 Apr '10

02 Apr '10

hmm... I'm not quite sure how to attach a file to a ticket through the web interface, so I'm just going to paste the patches in here: --- ldap.h.1.349.orig 2010-04-02 15:32:34.113126200 -0500 +++ ldap.h 2010-04-02 15:33:55.252793400 -0500 @@ -1841,7 +1841,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **serverctrls, LDAPControl **clientctrls, @@ -1855,7 +1855,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **serverctrls, LDAPControl **clientctrls, @@ -1870,7 +1870,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly )); =20 LDAP_F( int ) @@ -1879,7 +1879,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPMessage **res )); =20 @@ -1889,7 +1889,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, struct timeval *timeout, LDAPMessage **res )); --- search.c.1.88.orig 2010-04-02 15:26:34.268550800 -0500 +++ search.c 2010-04-02 15:32:36.939198200 -0500 @@ -47,7 +47,7 @@ * attrsonly 1 =3D> attributes only 0 =3D> attributes and values * * Example: - * char *attrs[] =3D { "mail", "title", 0 }; + * const char *attrs[] =3D { "mail", "title", 0 }; * ldap_search_ext( ld, "dc=3Dexample,dc=3Dcom", LDAP_SCOPE_SUBTREE, "cn~=3Dbob", * attrs, attrsonly, sctrls, ctrls, timeout, sizelimit, * &msgid ); @@ -58,7 +58,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -76,7 +76,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -139,7 +139,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -157,7 +157,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -210,14 +210,14 @@ * attrsonly 1 =3D> attributes only 0 =3D> attributes and values * * Example: - * char *attrs[] =3D { "mail", "title", 0 }; + * const char *attrs[] =3D { "mail", "title", 0 }; * msgid =3D ldap_search( ld, "dc=3Dexample,dc=3Dcom", LDAP_SCOPE_SUBTREE, "cn~=3Dbob", * attrs, attrsonly ); */ int ldap_search( LDAP *ld, LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, int attrsonly ) + LDAP_CONST char **attrs, int attrsonly ) { BerElement *ber; ber_int_t id; @@ -246,7 +246,7 @@ LDAP_CONST char *base, ber_int_t scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, ber_int_t attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -390,7 +390,7 @@ int ldap_search_st( LDAP *ld, LDAP_CONST char *base, int scope, - LDAP_CONST char *filter, char **attrs, + LDAP_CONST char *filter, LDAP_CONST char **attrs, int attrsonly, struct timeval *timeout, LDAPMessage **res ) { int msgid; @@ -419,7 +419,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPMessage **res ) {

1 0

(ITS#6505) ldap_search_* functions missing const qualifiers
by ehaszla＠transunion.com 02 Apr '10

02 Apr '10

Full_Name: Eric Haszlakiewicz Version: cvsweb OS: RHEL5 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (12.106.254.66) The ldap_search_ext, ldap_search_ext_s, ldap_search, ldap_search_s and ldap_search_st functions should use const for the attrs parameter. I checked revision 1.88 of libraries/libldap/search.c and it appears that those strings do not get modified so there is no reason that a list of string constants couldn't be passed in. I have a short patch that changes libraries/libldap/search.c and include/ldap.h (rev 1.349) to add the const qualifier.

1 0

Re: (ITS#6504) Corrupted control value when using pagedresults with subordinate *ldap* database
by masarati＠aero.polimi.it 01 Apr '10

01 Apr '10

> masarati(a)aero.polimi.it wrote: >>> masarati(a)aero.polimi.it wrote: >>>>> Also, I note that the glued paged response seems to work incorrectly. >>>>> I >>>>> made a simple test system, where the root database contains exactly >>>>> one >>>>> entry (the suffix) and a back-ldap is glued on top. If I request >>>>> entries >>>>> with a page size of 2, searching the suffix return 3 entries; >>>>> subsequent >>>>> searches return 2 entries from the proxy. I haven't figured out yet >>>>> where >>>>> the issue is. >>>> >>>> I figured out how to fix the issue you reported. It is related to the >>>> fact that slapd internally assumes that ldctl_oid values are constant >>>> strings, so it doesn't duplicate nor free them. However, control OIDs >>>> returned by back-ldap (and friends) have been decoded from the wire by >>>> the >>>> client library, so they are actually malloc'ed. That's why backglue >>>> ends >>>> up with a dangling pointer. >>>> >>>> I'm fixing it by having backglue duplicate (and free) control OIDs >>>> consistently. Another option would be to replace those values with >>>> constant strings for known control values. This would prevent gluing >>>> for >>>> unknown proxied controls. >>> >>> Or just tmpalloc them and don't bother to clean them up. >> >> Yes, I've reworked all the temporary controls allocation using tmpalloc. >> Too bad OIDs are char* and not bervals. >> >> With respect to the other issue, to honor the requested pagesize we'd >> need >> to intercept pagedresult requests, modify the page size in the request >> when a page crosses two databases, requesting the original page size >> minus >> entries already returned. I wonder whether it's worth the effort, >> though. > > IMO pagedresults should be a (global) overlay, and backends shouldn't do > anything about it. Note that the sssvlv overlay already intercepts > pagedresults, it's impossible to handle things correctly otherwise. Well, I've just fixed it. When changing database, I modified the paged results control value by setting the page size to pagesize - rs->sr_nentries, with an empty cookie. I'll commit tomorrow, as now I only have http access. p.

1 0

Re: (ITS#6504) Corrupted control value when using pagedresults with subordinate *ldap* database
by hyc＠symas.com 01 Apr '10

01 Apr '10

masarati(a)aero.polimi.it wrote: >> masarati(a)aero.polimi.it wrote: >>>> Also, I note that the glued paged response seems to work incorrectly. >>>> I >>>> made a simple test system, where the root database contains exactly one >>>> entry (the suffix) and a back-ldap is glued on top. If I request >>>> entries >>>> with a page size of 2, searching the suffix return 3 entries; >>>> subsequent >>>> searches return 2 entries from the proxy. I haven't figured out yet >>>> where >>>> the issue is. >>> >>> I figured out how to fix the issue you reported. It is related to the >>> fact that slapd internally assumes that ldctl_oid values are constant >>> strings, so it doesn't duplicate nor free them. However, control OIDs >>> returned by back-ldap (and friends) have been decoded from the wire by >>> the >>> client library, so they are actually malloc'ed. That's why backglue >>> ends >>> up with a dangling pointer. >>> >>> I'm fixing it by having backglue duplicate (and free) control OIDs >>> consistently. Another option would be to replace those values with >>> constant strings for known control values. This would prevent gluing >>> for >>> unknown proxied controls. >> >> Or just tmpalloc them and don't bother to clean them up. > > Yes, I've reworked all the temporary controls allocation using tmpalloc. > Too bad OIDs are char* and not bervals. > > With respect to the other issue, to honor the requested pagesize we'd need > to intercept pagedresult requests, modify the page size in the request > when a page crosses two databases, requesting the original page size minus > entries already returned. I wonder whether it's worth the effort, though. IMO pagedresults should be a (global) overlay, and backends shouldn't do anything about it. Note that the sssvlv overlay already intercepts pagedresults, it's impossible to handle things correctly otherwise. -- -- 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 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs