[Patch] Fix the ";binary" suffix behavior

[Jeremy Allison]

On Mon, Jun 27, 2016 at 06:59:07PM +0200, Niklas Abel wrote:
> -- 
> 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

Sorry for the delay, here's the review !

Getting there, but still needs some work I think.

> From 61bc531dcdc799114dda367355038b799f9526c3 Mon Sep 17 00:00:00 2001
> From: Niklas Abel <niklas.abel at lsexperts.de>
> Date: Mon, 27 Jun 2016 18:26:54 +0200
> Subject: [PATCH] Decription:
>  Fixed ";binary" suffix problem.
>  Search requests with the ";binary" suffix can now be searched with samba.
>  The suffix will now be ignored as it is meaningless to samba.
> 
> BUG: https://lists.samba.org/archive/samba-technical/2016-May/114242.html
> Signed-off-by: Niklas Abel <niklas.abel at lsexperts.de>
> ---
>  source4/dsdb/samdb/ldb_modules/resolve_oids.c | 65 +++++++++++++++++++++++++++
>  source4/dsdb/tests/python/ldap_syntaxes.py    |  7 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/source4/dsdb/samdb/ldb_modules/resolve_oids.c b/source4/dsdb/samdb/ldb_modules/resolve_oids.c
> index b5c5f8e..fd23866 100644
> --- a/source4/dsdb/samdb/ldb_modules/resolve_oids.c
> +++ b/source4/dsdb/samdb/ldb_modules/resolve_oids.c
> @@ -440,6 +440,62 @@ static int resolve_oids_callback(struct ldb_request *req, struct ldb_reply *ares
>  	return LDB_SUCCESS;
>  }

1). Tabs rather than spaces please ! If conditions always
must use { } framing even if only one executed clause.
Check out the Samba coding standards doc.

> +/**
> + * Strips ";binary" suffix from an attribute,
> + * if there is any because its meaningless to us.
> +*/

2). This function should take the suffix to remove as a parameter.
That way we can re-use it if we ever want to strip something else.

> +const static char * strip_binary_attribute (const void *mem_ctx, const char *attr)
> +{
> +    /* suffix which has to be removed */
> +    const char *suffix = ";binary";
> +    size_t attr_length = 0;
> +    size_t suffix_length = strlen(suffix);
> +    size_t new_attr_size = 0;
> +    const char *tmp = NULL;
> +    const static char *result = NULL;
> +
> +    if (!attr || !*attr) {

if !attr || *attr return attr, not NULL.
That's the default (no change) case.

> +        return NULL;
> +    }
> +    attr_length = strlen(attr);
> +    if (attr_length <= suffix_length) {

Doesn't the above case break if attr == ";binary" ?
Shouldn't we return "" in this case ?

> +        return attr;
> +    }
> +    new_attr_size = (attr_length - suffix_length);
> +    tmp = attr + new_attr_size;
> +    if (0==strcasecmp(suffix, tmp)) {

Boundary condition (mentioned above) where new_attr_size == 0.
Does this handle it right ?

> +        result = talloc_strndup(mem_ctx, attr, new_attr_size);
> +        if (result) {
> +            return result;
> +        }
> +    }
> +    return attr;
> +}
> +
> +/**
> + * Modified version of str_list_copy_const() which creates the new list without
> + * entries with a ";binary" tail.
> +*/
> +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]);
> +	}
> +	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));

Error condition if str_list_copy_const_clean_suffix returns NULL ?

> +	}
>  
>  	if (!needed) {
>  		return ldb_next_request(module, req);
> diff --git a/source4/dsdb/tests/python/ldap_syntaxes.py b/source4/dsdb/tests/python/ldap_syntaxes.py
> index 56a1755..762f66e 100755
> --- a/source4/dsdb/tests/python/ldap_syntaxes.py
> +++ b/source4/dsdb/tests/python/ldap_syntaxes.py
> @@ -82,6 +82,13 @@ systemOnly: FALSE
>          self.assertEquals(res[0]["lDAPDisplayName"][0], attr_ldap_display_name)
>          self.assertTrue("schemaIDGUID" in res[0])
>  
> +        # search for created attribute with ";binary" suffix
> +        res = []
> +        res = self.ldb.search("cn=%s,%s" % (attr_name+";binary", self.schema_dn), scope=SCOPE_BASE, attrs=["*"])
> +        self.assertEquals(len(res), 1)
> +        self.assertEquals(res[0]["lDAPDisplayName"][0], attr_ldap_display_name)
> +        self.assertTrue("schemaIDGUID" in res[0])
> +
>          class_name = "test-Class-DN-String" + time.strftime("%s", time.gmtime())
>          class_ldap_display_name = class_name.replace("-", "")
>  
> -- 
> 2.7.4
>