changed notes moved from Incoming to Contrib
> This patch is an implementation of RFC2696, "LDAP Control Extension for Simple > Paged Results Manipulation" for your review. The patch is very interesting; however there is a problem in handling the cookie. I applied the patch with minor fixes and everything works fine for the first page; however, after the cookie is sent back to the client, the next submission results in an invalid cookie. MOreover, you're doing a lot of mallocs that can be easily replacd by stack allocation. I'm going to try to fix your patch if I can; otherwise I'll post a revised version of it so that you can take care of my remarks. Thanks again for the submission. Pierangelo.
changed notes changed state Open to Feedback
Full_Name: Lynn Moss Version: HEAD branch OS: Linux URL: ftp://ftp.openldap.org/incoming/pagecontrol-112002.patch Submission from: (NULL) (129.42.208.186) This patch is an implementation of RFC2696, "LDAP Control Extension for Simple Paged Results Manipulation" for your review.
Forget about my previous posting: I found the bug. You were doing some ber-decoding in place, freeing the ber before saving the contents. I'm committing. Pierangelo.
changed notes changed state Feedback to Test
Note that, before support for this control is released, the server side of the patch should be carefully reviewed to ensure it complies with chapter 6 of RFC 2696: > 6. Security Considerations > > Server implementors should consider the resources used when clients > send searches with the simple paged control, to ensure that a > client's misuse of this control does not lock out other legitimate > operations. > > Servers implementations may enforce an overriding sizelimit, to > prevent the retrieval of large portions of a publically-accessible > directory. > > Clients can, using this control, determine how many entries match a > particular filter, before the entries are returned to the client. > This may require special processing in servers which perform access > control checks on entries to determine whether the existence of the > entry can be disclosed to the client. Pierangelo.
Pierangelo, Thanks for debugging this for me so quickly! The paging worked fine on the second page for me and all subsequent pages for me, so I don't know why I didn't catch the error. Are there still mallocs that should be changed to stack allocation per your previous post? Lynn
Pierangelo, Regarding chapter 6, security considerations: Paragraph 1 seems to be satisfied, since one can have multiple searches (or other operations) happening concurrently with no problem. Paragraph 2: The size limit for the search is still respected, so I believe this condition is satisfied. Paragraph 3: I'm not really sure how to interpret this one, but the server doesn't give an indication of the number of entries that will match before returning the first entries, so I would say that this is not satisfied. Is this necessary to add? Lynn
> Pierangelo, > Regarding chapter 6, security considerations: > Paragraph 1 seems to be satisfied, since one can have multiple searches > (or other operations) happening concurrently with no problem. > Paragraph 2: The size limit for the search is still respected, so I > believe this condition is satisfied. sure. > Paragraph 3: I'm not really sure how to interpret this one, but the > server doesn't give an indication of the number of entries that will > match before returning the first entries, so I would say that this is > not satisfied. Is this necessary to add? I think what you're doing is fine: it seems conservative (accroding to RFC 2696) to return 0 as count of entries that still have to be processed; however, an estimate of how many entries will be returned next is given by the number of candidates (if we accept it as a conservative estimate, i.e. a worst case in terms of resources usage). The point is that we should not reveal this information unless we give the sysadmin the possibility to protect it if required. We would need to set up a sort of meta-ACL that protects the count of entries that are available; to reuse a big chunk of code we might introduce a new type of limit in the limits stuff (see slapd.conf(5) "limits" and servers/slapd/limits.c); I'm thinking of a limits anonymous size.pr=10 size.pr=noEntriesLeft to limit the size of the page a specific user might request and to hide the number of entries left. This would lead to a higher granularity of the page size (the RFC states it cannot exceed the sizelimit imposed by the server; since slapd enforces limits on a per user basis, in this way we may allow a higher granularity. Pierangelo. PS: regarding the question about unnecessary mallocs, I think I fixed the one I noticed; there's a FIXME in servers/slapd/controls.c where you initially malloc 3*sizeof(LDAPControl *); is it really necessary? -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
> > > > > > Pierangelo, > Thanks for debugging this for me so quickly! The paging worked fine > on > the second page for me and all subsequent pages for me, so I don't know > why I didn't catch the error. Typical of some libc implementations not to invalidate memory after free()ing it :) > Are there still mallocs that should be > changed to stack allocation per your previous post? See my previous reply. Pierangelo. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
> to reuse a big chunk of code we might introduce a new type > of limit in the limits stuff (see slapd.conf(5) "limits" > and servers/slapd/limits.c); I'm thinking of a > > limits anonymous size.pr=10 size.pr=noEntriesLeft > > to limit the size of the page a specific user might request > and to hide the number of entries left. > > This would lead to a higher granularity of the page size > (the RFC states it cannot exceed the sizelimit imposed by > the server; since slapd enforces limits on a per user basis, > in this way we may allow a higher granularity. I've committed a quick hack that implements this; moreover, I think I've found a leak in back-bdb/search.c where a ber was not freed after flattening. Pierangelo. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it
Pierangelo, > I fixed the one I noticed; there's a FIXME in > servers/slapd/controls.c > where you initially malloc 3*sizeof(LDAPControl *); is it > really necessary? I believe you are correct that this isn't necessary. Thanks! Lynn
changed notes changed state Test to Closed
moved from Contrib to Archive.Contrib
applied to HEAD with changes; please test targetted for 2.2.