[Patch] Fix the ";binary" suffix behavior

[Niklas Abel]

Hey Jeremy

On 08/05/2016 09:57 PM, Jeremy Allison wrote:
> On Fri, Aug 05, 2016 at 12:56:35PM +0200, Niklas Abel wrote:
>> Hey Jeremy,
>>
>> here is the new patch version.
> Closer - comments below.
>
>>>> +const static char * strip_suffix (const void *mem_ctx, const char *attr, const char *suffix)
>>>> +{
>>>> +       size_t attr_length = 0;
>>>> +       size_t suffix_length = 0;
>>>> +       size_t new_attr_size = 0;
>>>> +       const char *tmp = NULL;
>>>> +       const static char *result = NULL;
>>>> +
>>>> +       if (!attr || !*attr || !suffix || !*suffix) {
>>>> +               return attr;
>>>> +       }
>>>> +       attr_length = strlen(attr);
>>>> +       suffix_length = strlen(suffix);
>>>> +       if (attr_length < suffix_length) {
>>>> +               return attr;
>>>> +       }
>>>> +       new_attr_size = (attr_length - suffix_length);
>>>> +       tmp = attr + new_attr_size;
>>>> +       if (0==strcasecmp(suffix, tmp)) {
> Convention in Samba is 'if (strcasecmp(suffix, tmp) == 0)'
> (yeah, picky I know :-).
Changed
>>>> +               result = talloc_strndup(mem_ctx, attr, new_attr_size);
>>>> +               if (result) {
>>>> +                       return result;
>>>> +               }
>>>> +       }
>>>> +       return attr;
>>>> +}
>>>> +*/
>>>> +const char **str_list_copy_const_clean_suffix(TALLOC_CTX *mem_ctx,
>>>> +					  const char **list)
>>>> +{
>>>> +	int i;
>>>> +	const char **ret;
>>>> +
>>>> +	if (list == NULL)
>>>> +		return NULL;
>>>> +
>>>> +	ret = talloc_array(mem_ctx, const char *, str_list_length(list)+1);
>>>> +	if (ret == NULL)
>>>> +		return NULL;
>>>> +
>>>> +	for (i=0;list && list[i];i++) {
>>>> +		ret[i] = strip_binary_attribute(mem_ctx, list[i]);
> Add an early break here if ret[i] == NULL (talloc fail inside
> strip_binary_attribute()).
Changed
>>>> +	}
>>>> +	ret[i] = NULL;
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  static int resolve_oids_search(struct ldb_module *module, struct ldb_request *req)
>>>>  {
>>>>  	struct ldb_context *ldb;
>>>> @@ -449,6 +505,7 @@ static int resolve_oids_search(struct ldb_module *module, struct ldb_request *re
>>>>  	struct resolve_oids_context *ac;
>>>>  	int ret;
>>>>  	bool needed = false;
>>>> +	bool needclean = false;
>>>>  	const char * const *attrs1;
>>>>  	const char **attrs2;
>>>>  	unsigned int i;
>>>> @@ -479,6 +536,10 @@ static int resolve_oids_search(struct ldb_module *module, struct ldb_request *re
>>>>  		const char *p;
>>>>  		const struct dsdb_attribute *a;
>>>>  
>>>> +		p = strchr(attrs1[i], ';');
>>>> +		if (p != NULL) {
>>>> +			needclean = true;
>>>> +		}
>>>>  		p = strchr(attrs1[i], '.');
>>>>  		if (p == NULL) {
>>>>  			continue;
>>>> @@ -492,6 +553,10 @@ static int resolve_oids_search(struct ldb_module *module, struct ldb_request *re
>>>>  		needed = true;
>>>>  		break;
>>>>  	}
>>>> +	if (needclean) {
>>>> +		req->op.search.attrs = str_list_copy_const_clean_suffix(ldb,
>>>> +					      discard_const_p(const char *, req->op.search.attrs));
> You're using ldb as the talloc context here. I think that's
> not right - too long lived.
>
> What about using req instead ? That seems the right
> object.
Changed, thanks :-)
>
> Also - do you need to do this before the:
>
>         if (!needed) {
>                 return ldb_next_request(module, req);
>         }
>
> of after ? I'm not sure - ldb_next_request() calls the
> next request in the chain - shouldn't it get the unmodified
> attribute list or should it get the stripped list ?
Yes we need it before, my tests are failing if I change the order.
So it needs the cleaned list.
>
> Sorry for the pickyness but I think we're getting there !
No problem, thanks for the review and the fast response.
>
> Jeremy.
Bye,
Niklas

-- 
Niklas Abel (IT Security Consultant), http://www.lsexperts.de
LSE Leading Security Experts GmbH, Postfach 100121, 64201 Darmstadt
Tel: +49 6151 86086-261, Fax: -299, Mobil: +49 151 26467737
Unternehmenssitz: Weiterstadt, Amtsgericht Darmstadt: HRB8649
Geschäftsführer: Nils Manegold, Oliver Michel, Arved Graf von Stackelberg, Sven Walther

-------------- next part --------------
A non-text attachment was scrubbed...
Name: binary_suffix.patch
Type: text/x-patch
Size: 4462 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160808/080604a1/binary_suffix.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3958 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160808/080604a1/smime.bin>