Ticket #47455 - valgrind - value mem leaks, uninit mem usage

https://fedorahosted.org/389/ticket/47455
Reviewed by: nkinder (Thanks!)
Branch: master
Fix Description: The problem was that slapi_valueset_add_attr_valuearray_ext
was assuming the caller was going to free the entire given vs upon failure.
This is fine for the value replace case but not for the add 1 value case.
Callers of slapi_valueset_add_attr_valuearray_ext must provide
the dup_index parameter if using SLAPI_VALUE_FLAG_PASSIN and
SLAPI_VALUE_FLAG_DUPCHECK, and if there is more than one value.  The caller
needs to know which of the values from addvals is in vs to properly clean up
with no memory leaks.
Platforms tested: RHEL6 x86_64
Flag Day: no
Doc impact: no

ldap/servers/slapd/slapi-private.h

@@ -846,6 +846,11 @@ void valuearray_add_valuearray_fast( Slapi_Value ***vals, Slapi_Value **addvals,
 Slapi_Value * valueset_find_sorted (const Slapi_Attr *a, const Slapi_ValueSet *vs, const Slapi_Value *v, int *index);
 int valueset_insert_value_to_sorted(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value *vi, int dupcheck);
 void valueset_array_to_sorted (const Slapi_Attr *a, Slapi_ValueSet *vs);
+/* NOTE: if the flags include SLAPI_VALUE_FLAG_PASSIN and SLAPI_VALUE_FLAG_DUPCHECK
+ * THE CALLER MUST PROVIDE THE dup_index PARAMETER in order to know where in addval
+ * the un-copied values start e.g. to free them for cleanup
+ * see valueset_replace_valuearray_ext() for an example
+ */
 int slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value **addval, int nvals, unsigned long flags, int *dup_index);
 int valuearray_find(const Slapi_Attr *a, Slapi_Value **va, const Slapi_Value *v);
 int valuearray_dn_normalize_value(Slapi_Value **vals);

ldap/servers/slapd/valueset.c

@@ -1147,8 +1147,9 @@ slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs,
 					rc = LDAP_TYPE_OR_VALUE_EXISTS;
 					if (dup_index) *dup_index = i;
 					if (passin) {
-						/* we have to NULL out the first value so valuearray_free won't delete values in addvals */
-						(vs->va)[0] = NULL;
+						PR_ASSERT((i == 0) || dup_index);
+						/* caller must provide dup_index to know how far we got in addvals */
+						(vs->va)[vs->num] = NULL;
 					} else {
 						slapi_value_free(&(vs->va)[vs->num]);
 					}
@@ -1391,8 +1392,9 @@ valueset_replace_valuearray_ext(Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value *
     } else {
 	/* verify the given values are not duplicated.  */
 	unsigned long flags = SLAPI_VALUE_FLAG_PASSIN|SLAPI_VALUE_FLAG_DUPCHECK;
+	int dupindex = 0;
 	Slapi_ValueSet *vs_new = slapi_valueset_new();
-	rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, flags, NULL);
+	rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, flags, &dupindex);
 
 	if ( rc == LDAP_SUCCESS )
 	{
@@ -1420,8 +1422,11 @@ valueset_replace_valuearray_ext(Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value *
 	{
 	        /* caller expects us to own valstoreplace - since we cannot
 	           use them, just delete them */
+		/* using PASSIN, some of the Slapi_Value* are in vs_new, and the rest
+		 * after dupindex are in valstoreplace
+		 */
         	slapi_valueset_free(vs_new);
-        	valuearray_free(&valstoreplace);
+        	valuearray_free_ext(&valstoreplace, dupindex);
 		PR_ASSERT((vs->sorted == NULL) || (vs->num == 0) || ((vs->sorted[0] >= 0) && (vs->sorted[0] < vs->num)));
 	}
     }