[Patch] Fix the ";binary" suffix behavior

[Jeremy Allison]

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 :-).

> >> +               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()).

> >> +	}
> >> +	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.

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 ?

Sorry for the pickyness but I think we're getting there !

Jeremy.