On 27/07/2010 01:08, masarati@aero.polimi.it wrote:

...

Full_Name: Jonathan CLARKE Version: RE24 OS: URL: ftp://ftp.openldap.org/incoming/jonathan-clarke-pcache-100723.patch Submission from: (NULL) (80.13.86.63)

While checking it's configuration, the pcache overlay verifies each configured attribute set (pcacheAttrset), to ensure that all attributes in the set are defined, via slap_str2ad. Given an attribute set with a non-existant attribute, an error is logged and slapd refuses to start (as expected):

line 117 (pcacheAttrset 0 nonexistantAttr) /etc/ldap/slapd.conf: line 117: attribute type undefined.

However, when a search request comes in, the requested attributes list is not checked by the pcache overlay to ensure that attributes are properly defined. In effect, slapd just ignores the non-existant attributes, and returns other attributes (or behaves as if 1.1 was requested if all requested attributes are invalid).

This causes pcache's attribute set matching to fail for some requests, since it counts invalid attributes. If it were to ignore them, configured attribute sets might match, and successfully cache the search. The patch above implements this behaviour. Would you consider it for inclusion in OpenLDAP?

I realize this may be considered "repairing bad requests", but sometimes one can't (easily) control what clients are requesting. Furthermore, it seems to make sense to have matching behavior all over (since slapd ignores invalid requested attributes, pcache should too, IMHO).

Yours looks like a good catch; however, your patch looks a bit like an overkill, since the an_desc field of the attribute list that is passed to get_attr_set() should already be set if the attribute was recognized by slapd, so you don't need to go through slap_bv2ad() once more.

Thanks for your feedback.

Indeed, looking at it with your comments, definitely overkill. I meant to ask about this approach in the bug description but forgot, sorry.

Here is a simpler patch which works the same for my tests: ftp://ftp.openldap.org/incoming/jonathan-clarke-pcache-100727.patch

Jonathan