Re: (ITS#6600) pcache overlay should ignore invalid attributes in search requests

[jonathan@phillipoux.net]

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
-- 
--------------------------------------------------------------
Jonathan Clarke - jonathan@phillipoux.net
--------------------------------------------------------------
Ldap Synchronization Connector (LSC) - http://lsc-project.org
--------------------------------------------------------------