From b8601e47ac298370e3777dcd8f7426eccd738c63 Mon Sep 17 00:00:00 2001
From: Noriko Hosoi <nhosoi@redhat.com>
Date: Fri, 13 May 2016 15:15:09 -0700
Subject: [PATCH] Ticket #47506 - slapi_valueset_add_value_ext must return int

---
 ldap/servers/plugins/cos/cos_cache.c               |  5 +++-
 ldap/servers/plugins/memberof/memberof.c           | 27 +++++++++++++---------
 .../plugins/replication/windows_protocol_util.c    | 20 ++++++++++++----
 ldap/servers/slapd/back-ldbm/ldbm_attr.c           | 10 ++++----
 ldap/servers/slapd/pw.c                            | 12 ++++++----
 ldap/servers/slapd/slapi-plugin.h                  |  4 +++-
 ldap/servers/slapd/value.c                         | 13 +++++------
 ldap/servers/slapd/valueset.c                      |  5 ++--
 ldap/servers/slapd/vattr.c                         |  5 +++-
 9 files changed, 66 insertions(+), 35 deletions(-)

diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c
index 8a32630..de64f03 100644
--- a/ldap/servers/plugins/cos/cos_cache.c
+++ b/ldap/servers/plugins/cos/cos_cache.c
@@ -3234,7 +3234,10 @@ static int cos_cache_cos_2_slapi_valueset(cosAttributes *pAttr, Slapi_ValueSet *
 			Slapi_Value *val = slapi_value_new_string(pAttrVal->val);
 			if(val) {
 				if(!add_mode || !slapi_valueset_find(attr, *out_vs, val)) {
-					slapi_valueset_add_value_ext(*out_vs, val, SLAPI_VALUE_FLAG_PASSIN);
+					if (slapi_valueset_add_value_ext(*out_vs, val, SLAPI_VALUE_FLAG_PASSIN)) {
+						/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+						slapi_value_free(&val);
+					}
 				}
 				else {
 					slapi_value_free(&val);
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
index aad300f..0915685 100644
--- a/ldap/servers/plugins/memberof/memberof.c
+++ b/ldap/servers/plugins/memberof/memberof.c
@@ -2060,8 +2060,8 @@ int memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
 	Slapi_DN *group_sdn = slapi_entry_get_sdn(e);
 	char *group_ndn = slapi_entry_get_ndn(e);
 	char *group_dn = slapi_entry_get_dn(e);
-	Slapi_Value *group_ndn_val = 0;
-	Slapi_Value *group_dn_val = 0;
+	Slapi_Value *group_ndn_val = NULL;
+	Slapi_Value *group_dn_val = NULL;
 	Slapi_ValueSet *groupvals = *((memberof_get_groups_data*)callback_data)->groupvals;
 	Slapi_ValueSet *group_norm_vals = *((memberof_get_groups_data*)callback_data)->group_norm_vals;
 	MemberOfConfig *config = ((memberof_get_groups_data*)callback_data)->config;
@@ -2095,7 +2095,6 @@ int memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
 		slapi_log_error( SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,
 			"memberof_get_groups_callback: group recursion"
 			" detected in %s\n" ,group_ndn);
-		slapi_value_free(&group_ndn_val);
 		goto bail;
 
 	}
@@ -2114,25 +2113,31 @@ int memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
 		slapi_log_error( SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,
 			"memberof_get_groups_callback: possible group recursion"
 			" detected in %s\n" ,group_ndn);
-		slapi_value_free(&group_ndn_val);
 		goto bail;
 	}
 
 	/* if the group does not belong to an excluded subtree, adds it to the valueset */
 	if (memberof_entry_in_scope(config, group_sdn)) {
-			/* Push group_dn_val into the valueset.  This memory is now owned
-			 * by the valueset. */
-			group_dn_val = slapi_value_new_string(group_dn);
-			slapi_valueset_add_value_ext(groupvals, group_dn_val, SLAPI_VALUE_FLAG_PASSIN);
-			slapi_valueset_add_value_ext(group_norm_vals, group_ndn_val, SLAPI_VALUE_FLAG_PASSIN);
+		/* Push group_dn_val into the valueset.  This memory is now owned
+		 * by the valueset. */
+		group_dn_val = slapi_value_new_string(group_dn);
+		if (!slapi_valueset_add_value_ext(groupvals, group_dn_val, SLAPI_VALUE_FLAG_PASSIN)) {
+			/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+			group_dn_val = NULL;
+		}
+		if (!slapi_valueset_add_value_ext(group_norm_vals, group_ndn_val, SLAPI_VALUE_FLAG_PASSIN)) {
+			/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+			group_ndn_val = NULL;
+		}
 	}
 	if(!config->skip_nested || config->fixup_task){
 		/* now recurse to find parent groups of e */
-		memberof_get_groups_r(((memberof_get_groups_data*)callback_data)->config,
-			group_sdn, callback_data);
+		memberof_get_groups_r(((memberof_get_groups_data*)callback_data)->config, group_sdn, callback_data);
 	}
 
 bail:
+	slapi_value_free(&group_dn_val);
+	slapi_value_free(&group_ndn_val);
 	return rc;
 }
 
diff --git a/ldap/servers/plugins/replication/windows_protocol_util.c b/ldap/servers/plugins/replication/windows_protocol_util.c
index 084b520..90ee412 100644
--- a/ldap/servers/plugins/replication/windows_protocol_util.c
+++ b/ldap/servers/plugins/replication/windows_protocol_util.c
@@ -2105,7 +2105,10 @@ windows_create_remote_entry(Private_Repl_Protocol *prp,Slapi_Entry *original_ent
 						new_value = slapi_value_dup(value);
 						slapi_valueset_done(vs);
 						/* The below hands off the memory to the valueset */
-						slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN);
+						if (slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN)) {
+							/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+							slapi_value_free(&new_value);
+						}
 					}
 				}
 			}
@@ -2179,7 +2182,10 @@ windows_create_remote_entry(Private_Repl_Protocol *prp,Slapi_Entry *original_ent
 								new_value = slapi_value_dup(value);
 								slapi_valueset_done(vs);
 								/* The below hands off the memory to the valueset */
-								slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN);
+								if (slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN)) {
+									/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+									slapi_value_free(&new_value);
+								}
 							}
 						}
 					}
@@ -4802,7 +4808,10 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,
 								new_value = slapi_value_dup(value);
 								slapi_valueset_done(vs);
 								/* The below hands off the memory to the valueset */
-								slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN);
+								if (slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN)) {
+									/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+									slapi_value_free(&new_value);
+								}
 							}
 						}
 					} else if ((0 == slapi_attr_type_cmp(local_type, "initials",
@@ -4957,7 +4966,10 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,
 									new_value = slapi_value_dup(value);
 									slapi_valueset_done(vs);
 									/* The below hands off the memory to the valueset */
-									slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN);
+									if (slapi_valueset_add_value_ext(vs, new_value, SLAPI_VALUE_FLAG_PASSIN)) {
+										/* if slapi_valueset_add_value_ext is successful, val is consumed. */
+										slapi_value_free(&new_value);
+									}
 								}
 							}
 						}
diff --git a/ldap/servers/slapd/back-ldbm/ldbm_attr.c b/ldap/servers/slapd/back-ldbm/ldbm_attr.c
index 092b6b5..a59a0bb 100644
--- a/ldap/servers/slapd/back-ldbm/ldbm_attr.c
+++ b/ldap/servers/slapd/back-ldbm/ldbm_attr.c
@@ -326,12 +326,14 @@ attr_index_parse_idlistsize_values(Slapi_Attr *attr, struct index_idlistsizeinfo
 				if (!idlinfo->ai_values) {
 					idlinfo->ai_values = slapi_valueset_new();
 				}
-				slapi_valueset_add_value_ext(idlinfo->ai_values, ivals[ii], SLAPI_VALUE_FLAG_PASSIN);
+				if (!slapi_valueset_add_value_ext(idlinfo->ai_values, ivals[ii], SLAPI_VALUE_FLAG_PASSIN)) {
+					/* If slapi_valueset_add_value_ext fails, ivals[ii] should be freed. */
+					slapi_value_free(&ivals[ii]);
+				}
 			}
 		}
-		/* only free members of ivals that were not moved to ai_values */
-		valuearray_free_ext(&ivals, ii);
-		ivals = NULL;
+		/* each slapi_value is freed or consumed. */
+		slapi_ch_free((void **)&ivals);
 	}
 
 	slapi_filter_free(filt, 1);
diff --git a/ldap/servers/slapd/pw.c b/ldap/servers/slapd/pw.c
index 17938b5..a2ee760 100644
--- a/ldap/servers/slapd/pw.c
+++ b/ldap/servers/slapd/pw.c
@@ -1503,7 +1503,7 @@ check_trivial_words (Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Value **vals, char
 
 	slapi_pblock_get ( pb, SLAPI_PWPOLICY, &pwresponse_req );
 
-        /* Get a list of present values for attrtype in the existing entry, if there is one */
+	/* Get a list of present values for attrtype in the existing entry, if there is one */
 	if (e != NULL )
 	{
 		if ( (attr = attrlist_find(e->e_attrs, attrtype)) &&
@@ -1535,13 +1535,17 @@ check_trivial_words (Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Value **vals, char
 				{
 					/* Add new value to valueset */
 					valp = slapi_value_new_berval( bvp );
-                                        slapi_valueset_add_value_ext( vs, valp, SLAPI_VALUE_FLAG_PASSIN );
-					valp = NULL;
+					if (slapi_valueset_add_value_ext(vs, valp, SLAPI_VALUE_FLAG_PASSIN)) {
+						/* passin failed.  need to free it. */
+						slapi_value_free(&valp);
+					} else {
+						valp = NULL;
+					}
 				}
 			}
 		}
 		/* Free smod */
-        	slapi_mod_free(&smod);
+		slapi_mod_free(&smod);
 		smod = NULL;
 		smodp = NULL;
 	}
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index d13aae9..205223a 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -5002,8 +5002,10 @@ void slapi_valueset_add_value(Slapi_ValueSet *vs, const Slapi_Value *addval);
  * \see slapi_valueset_add_value()
  * \see slapi_valueset_first_value()
  * \see slapi_valueset_next_value()
+ * \return \c LDAP_SUCCESS if the addval is successfully set to valueset.
+ * \return \c LDAP_TYPE_OR_VALUE_EXISTS if the addval is already in valueset.
  */
-void slapi_valueset_add_value_ext(Slapi_ValueSet *vs, const Slapi_Value *addval, unsigned long flags);
+int slapi_valueset_add_value_ext(Slapi_ValueSet *vs, const Slapi_Value *addval, unsigned long flags);
 int slapi_valueset_add_attr_value_ext(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value *addval, unsigned long flags);
 
 /**
diff --git a/ldap/servers/slapd/value.c b/ldap/servers/slapd/value.c
index 51beff7..bc606fb 100644
--- a/ldap/servers/slapd/value.c
+++ b/ldap/servers/slapd/value.c
@@ -206,15 +206,14 @@ slapi_value_get_flags(Slapi_Value *v)
 void 
 slapi_value_free(Slapi_Value **v)
 {
-    if(v!=NULL && *v!=NULL)
-    {
+	if (v && *v) {
 		VALUE_DUMP(*v,"value_free");
 		value_done(*v);
-        slapi_ch_free((void **)v);
-		*v= NULL;
-        PR_INCREMENT_COUNTER(slapi_value_counter_deleted);
-	    PR_DECREMENT_COUNTER(slapi_value_counter_exist);
-    }
+		slapi_ch_free((void **)v);
+		*v = NULL;
+		PR_INCREMENT_COUNTER(slapi_value_counter_deleted);
+		PR_DECREMENT_COUNTER(slapi_value_counter_exist);
+	}
 }
 
 void
diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c
index 50c0e52..938cd90 100644
--- a/ldap/servers/slapd/valueset.c
+++ b/ldap/servers/slapd/valueset.c
@@ -855,16 +855,17 @@ valueset_add_valuearray(Slapi_ValueSet *vs, Slapi_Value **addvals)
 void
 slapi_valueset_add_value(Slapi_ValueSet *vs, const Slapi_Value *addval)
 {
+	/* Since no flag is set, there is no chance to return any error. */
 	slapi_valueset_add_value_ext(vs, addval, 0);
 }
 
-void
+int
 slapi_valueset_add_value_ext(Slapi_ValueSet *vs, const Slapi_Value *addval, unsigned long flags)
 {
 	Slapi_Value *oneval[2];
 	oneval[0]= (Slapi_Value*)addval;
 	oneval[1]= NULL;
-	slapi_valueset_add_attr_valuearray_ext(NULL, vs, oneval, 1, flags, NULL);
+	return slapi_valueset_add_attr_valuearray_ext(NULL, vs, oneval, 1, flags, NULL);
 }
 
 
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
index e707be2..5451cb5 100644
--- a/ldap/servers/slapd/vattr.c
+++ b/ldap/servers/slapd/vattr.c
@@ -2342,7 +2342,10 @@ int vattr_basic_sp_get_value(vattr_sp_handle *handle, vattr_context *c, Slapi_En
 	value = slapi_value_new_string("Hello Client");
 	*results = slapi_ch_calloc(1,sizeof(Slapi_ValueSet));
 	slapi_valueset_init(*results);
-	slapi_valueset_add_value_ext(*results,value,SLAPI_VALUE_FLAG_PASSIN);
+	if (slapi_valueset_add_value_ext(*results,value,SLAPI_VALUE_FLAG_PASSIN)) {
+		/* passin failed.  need to free it. */
+		slapi_value_free(&value);
+	}
 	return 0;
 }
 
-- 
2.4.11

