From 7c4afc0e4ae7b6b4abfd7f38c7cee0b3cf4d1064 Mon Sep 17 00:00:00 2001
From: Rich Megginson <rmeggins@redhat.com>
Date: Fri, 3 Feb 2012 09:51:46 -0700
Subject: [PATCH] Ticket #280 - extensible binary filters do not work

https://fedorahosted.org/389/ticket/280
Resolves: Ticket #280
Bug Description: extensible binary filters do not work
Reviewed by: ???
Branch: master
Fix Description: The code was using strdup to copy the value when dup'ing
the filter.  We already had code that copied the contents of binary
bervals, so I just put slapi wrappers around them and created and
documented two new slapi functions - slapi_ber_bvcpy and slapi_ber_bvdone.
I found a few places that were manually copying the contents of bervals
and replaced them with slapi_ber_bvcpy and slapi_ber_bvdone.  With this
code the filter, I18n, and index tests pass at 100%.
Platforms tested: RHEL6 x86_64, Fedora 16
Flag Day: yes - slapi-plugin.h change
Doc impact: yes - two new slapi functions
---
 ldap/servers/plugins/collation/orfilter.c     |   19 ++++++-------
 ldap/servers/plugins/replication/repl5_agmt.c |   12 ++------
 ldap/servers/plugins/syntaxes/string.c        |    6 +---
 ldap/servers/slapd/filter.c                   |   10 ++-----
 ldap/servers/slapd/slapi-plugin.h             |   35 +++++++++++++++++++++++++
 ldap/servers/slapd/value.c                    |   12 ++++++++
 6 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/ldap/servers/plugins/collation/orfilter.c b/ldap/servers/plugins/collation/orfilter.c
index d947e02..2293baf 100644
--- a/ldap/servers/plugins/collation/orfilter.c
+++ b/ldap/servers/plugins/collation/orfilter.c
@@ -347,12 +347,7 @@ slapi_ch_bvdup0 (struct berval* val)
 {
     auto struct berval* result = (struct berval*)
       slapi_ch_malloc (sizeof (struct berval));
-    result->bv_len = val->bv_len;
-    result->bv_val = slapi_ch_malloc (result->bv_len + 1);
-    if (result->bv_len > 0) {
-	memcpy (result->bv_val, val->bv_val, result->bv_len);
-    }
-    result->bv_val[result->bv_len] = '\0';
+    slapi_ber_bvcpy(result, val);
     return result;
 }
 
@@ -398,12 +393,9 @@ ss_filter_values (struct berval* pattern, int* query_op)
     }
     if (n == 2) { /* no wildcards in pattern */
 	auto struct berval** pvec = (struct berval**) slapi_ch_malloc (sizeof (struct berval*) * 2);
-	auto struct berval* pv = (struct berval*) slapi_ch_malloc (sizeof (struct berval));
+	auto struct berval* pv = slapi_ch_bvdup(pattern);
 	pvec[0] = pv;
 	pvec[1] = NULL;
-	pv->bv_len = pattern->bv_len;
-	pv->bv_val = slapi_ch_malloc (pv->bv_len);
-	memcpy (pv->bv_val, pattern->bv_val, pv->bv_len);
 	ss_unescape (pv);
 	*query_op = SLAPI_OP_EQUAL;
 	return pvec;
@@ -448,6 +440,13 @@ ss_filter_key (indexer_t* ix, struct berval* val)
 	vals[1] = NULL;
 	keys = ix->ix_index (ix, vals, NULL);
 	if (keys && keys[0]) {
+	    /* why +1 in the len?  you need the +1 to old the trailing NULL,
+	       to guard against someone accidentally doing a strcmp or
+	       other str function, but a bvcmp is going to use the bv_len
+	       which includes the trailing NULL which the value being
+	       compared against might not have - not only are bervals
+	       not guaranteed to be properly NULL terminated, but they
+	       also contain binary data - see slapi_ber_bvcpy() */
 	    key->bv_len = keys[0]->bv_len + 1;
 	    key->bv_val = slapi_ch_malloc (key->bv_len);
 	    memcpy (key->bv_val, keys[0]->bv_val, keys[0]->bv_len);
diff --git a/ldap/servers/plugins/replication/repl5_agmt.c b/ldap/servers/plugins/replication/repl5_agmt.c
index f1f114b..1d9affa 100644
--- a/ldap/servers/plugins/replication/repl5_agmt.c
+++ b/ldap/servers/plugins/replication/repl5_agmt.c
@@ -292,10 +292,7 @@ agmt_new_from_entry(Slapi_Entry *e)
 			const struct berval *bv = slapi_value_get_berval(sval);
 			if (NULL != bv)
 			{
-				ra->creds->bv_val = slapi_ch_malloc(bv->bv_len + 1);
-				memcpy(ra->creds->bv_val, bv->bv_val, bv->bv_len);
-				ra->creds->bv_len = bv->bv_len;
-				ra->creds->bv_val[bv->bv_len] = '\0'; /* be safe */
+				slapi_ber_bvcpy(ra->creds, bv);
 			}
 		}
 	}
@@ -1007,8 +1004,7 @@ agmt_set_credentials_from_entry(Repl_Agmt *ra, const Slapi_Entry *e)
 	PR_ASSERT(NULL != ra);
 	slapi_entry_attr_find(e, type_nsds5ReplicaCredentials, &sattr);
 	PR_Lock(ra->lock);
-	slapi_ch_free((void **)&ra->creds->bv_val);
-	ra->creds->bv_len = 0;
+	slapi_ber_bvdone(ra->creds);
 	if (NULL != sattr)
 	{
 		Slapi_Value *sval = NULL;
@@ -1016,9 +1012,7 @@ agmt_set_credentials_from_entry(Repl_Agmt *ra, const Slapi_Entry *e)
 		if (NULL != sval)
 		{
 			const struct berval *bv = slapi_value_get_berval(sval);
-			ra->creds->bv_val = slapi_ch_calloc(1, bv->bv_len + 1);
-			memcpy(ra->creds->bv_val, bv->bv_val, bv->bv_len);
-			ra->creds->bv_len = bv->bv_len;
+			slapi_ber_bvcpy(ra->creds, bv);
 		}
 	}
 	/* If no credentials set, set to zero-length string */
diff --git a/ldap/servers/plugins/syntaxes/string.c b/ldap/servers/plugins/syntaxes/string.c
index 80a1a46..5c78d6a 100644
--- a/ldap/servers/plugins/syntaxes/string.c
+++ b/ldap/servers/plugins/syntaxes/string.c
@@ -76,13 +76,11 @@ string_filter_ava( struct berval *bvfilter, Slapi_Value **bvals, int syntax,
 	if (syntax & SYNTAX_NORM_FILT) {
 		pbvfilter_norm = bvfilter; /* already normalized */
 	} else {
-		bvfilter_norm.bv_val = slapi_ch_malloc( bvfilter->bv_len + 1 );
-		SAFEMEMCPY( bvfilter_norm.bv_val, bvfilter->bv_val, bvfilter->bv_len );
-		bvfilter_norm.bv_val[bvfilter->bv_len] = '\0';
+		slapi_ber_bvcpy(&bvfilter_norm, bvfilter);
 		/* 3rd arg: 1 - trim leading blanks */
 		value_normalize_ext( bvfilter_norm.bv_val, syntax, 1, &alt );
 		if (alt) {
-			slapi_ch_free_string(&bvfilter_norm.bv_val);
+			slapi_ber_bvdone(&bvfilter_norm);
 			bvfilter_norm.bv_val = alt;
 			alt = NULL;
 		}
diff --git a/ldap/servers/slapd/filter.c b/ldap/servers/slapd/filter.c
index 36bd419..ae87e5c 100644
--- a/ldap/servers/slapd/filter.c
+++ b/ldap/servers/slapd/filter.c
@@ -698,10 +698,7 @@ slapi_filter_dup(Slapi_Filter *f)
 	case LDAP_FILTER_LE:
 	case LDAP_FILTER_APPROX:
 		out->f_ava.ava_type = slapi_ch_strdup(f->f_ava.ava_type);
-		out->f_ava.ava_value.bv_val = slapi_ch_malloc(f->f_ava.ava_value.bv_len+1);
-		memcpy(out->f_ava.ava_value.bv_val,f->f_ava.ava_value.bv_val,f->f_ava.ava_value.bv_len);
-		out->f_ava.ava_value.bv_val[f->f_ava.ava_value.bv_len] = 0; /* terminate */
-		out->f_ava.ava_value.bv_len = f->f_ava.ava_value.bv_len;
+		slapi_ber_bvcpy(&out->f_ava.ava_value, &f->f_ava.ava_value);
 		break;
 
 	case LDAP_FILTER_SUBSTRINGS:
@@ -733,8 +730,7 @@ slapi_filter_dup(Slapi_Filter *f)
 	case LDAP_FILTER_EXTENDED:
 		out->f_mr_oid = slapi_ch_strdup(f->f_mr_oid);
 		out->f_mr_type = slapi_ch_strdup(f->f_mr_type);
-		out->f_mr_value.bv_val = slapi_ch_strdup(f->f_mr_value.bv_val);
-		out->f_mr_value.bv_len = f->f_mr_value.bv_len;
+		slapi_ber_bvcpy(&out->f_mr_value, &f->f_mr_value);
 		out->f_mr_dnAttrs = f->f_mr_dnAttrs;
 		if (f->f_mr.mrf_match) {
 			int rc = plugin_mr_filter_create(&out->f_mr);
@@ -796,7 +792,7 @@ slapi_filter_free( struct slapi_filter *f, int recurse )
 	case LDAP_FILTER_EXTENDED:
 		slapi_ch_free((void**)&f->f_mr_oid);
 		slapi_ch_free((void**)&f->f_mr_type);
-		slapi_ch_free((void **)&f->f_mr_value.bv_val );
+		slapi_ber_bvdone(&f->f_mr_value);
 		if (f->f_mr.mrf_destroy != NULL) {
 		    Slapi_PBlock pb;
 		    pblock_init (&pb);
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index ce68e76..36683a4 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -4468,6 +4468,41 @@ size_t slapi_value_get_length(const Slapi_Value *value);
  */
 int slapi_value_compare(const Slapi_Attr *a,const Slapi_Value *v1,const Slapi_Value *v2);
 
+/**
+ * Free and initialize the contents of the berval without freeing the
+ * struct berval * itself.  This is useful when you use struct berval
+ * which are allocated on the stack e.g.
+ * \code
+ * struct berval bv = {0, NULL};
+ * slapi_ber_bvcpy(&bv, someotherbvp);
+ * // use bv
+ * slapi_ber_bvdone(&bv);
+ * \endcode
+ * You must only call slapi_ber_bvdone() if the bv_val member has been 
+ * allocated with malloc() or is NULL.
+ *
+ * \param bvp A struct berval * you want to free the contents of
+ * \return \c void
+ */
+void slapi_ber_bvdone(struct berval *bvp);
+
+/**
+ * Copy the contents of the berval without allocating a new struct berval*
+ * This is useful when you use struct berval which are allocated on the stack e.g.
+ * \code
+ * struct berval bv = {0, NULL};
+ * slapi_ber_bvcpy(&bv, someotherbvp);
+ * // use bv
+ * slapi_ber_bvdone(&bv);
+ * \endcode
+ * The bvs source berval to copy from can either be allocated on the heap
+ * or the stack.
+ *
+ * \param bvd A struct berval * you want copy to
+ * \param bvs A struct berval * you want to copy from
+ * \return \c void
+ */
+void slapi_ber_bvcpy(struct berval *bvd, const struct berval *bvs);
 
 /*
  * Valueset functions.
diff --git a/ldap/servers/slapd/value.c b/ldap/servers/slapd/value.c
index 1edfe79..30301e2 100644
--- a/ldap/servers/slapd/value.c
+++ b/ldap/servers/slapd/value.c
@@ -78,6 +78,18 @@ static void ber_bvcpy(struct berval *bvd, const struct berval *bvs)
     return;
 }
 
+void
+slapi_ber_bvdone(struct berval *bvp)
+{
+    ber_bvdone(bvp);
+}
+
+void
+slapi_ber_bvcpy(struct berval *bvd, const struct berval *bvs)
+{
+    ber_bvcpy(bvd, bvs);
+}
+
 /* <=========================== Slapi_Value ==========================> */
 
 #ifdef VALUE_DEBUG
-- 
1.7.1

