From c254bb234afe58b36129a7c6c2849e6ce55a035f Mon Sep 17 00:00:00 2001
From: Rich Megginson <rmeggins@redhat.com>
Date: Mon, 29 Jul 2013 15:06:03 -0600
Subject: [PATCH 1/2] Ticket #47455 - valgrind - value mem leaks, uninit mem usage

https://fedorahosted.org/389/ticket/47455
Reviewed by: ???
Branch: master
Fix Description: Make sure to use slapi_valueset_done to free valuesets to
clean up everything.  Make sure to free the values in valueset_set_valueset
before setting them.  The calculation of the size of the values to write
to the changelog was wrong.  It was finding the size of the length by doing
sizeof(bv.bv_len) which is 8 bytes on 64-bit platforms, but using PRInt32 which
is 4 bytes on all platforms to actually store the length.  So each length
calculation was using an extra 4 bytes, and writing this as uninitialized
memory to the changelog db.
In slapi_valueset_add_attr_valuearray_ext(), if an error is returned, and
the PASSIN flag was used, set the vs->va[0] to NULL - this will allow the
called to use both slapi_valueset_done(vs) and valuearray_free(&addvals) -
addvals will still own all of the values.
Cleaned up some other memory leaks and uninitialized memory uses.
Platforms tested: RHEL6 x86_64
Flag Day: no
Doc impact: no
---
 ldap/servers/plugins/replication/cl5_api.c        |   10 ++++----
 ldap/servers/plugins/replication/repl5_protocol.c |    2 +-
 ldap/servers/slapd/back-ldbm/import.c             |    1 +
 ldap/servers/slapd/daemon.c                       |    3 ++
 ldap/servers/slapd/entry.c                        |    9 +++++--
 ldap/servers/slapd/valueset.c                     |   23 +++++++++++++++++++-
 6 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/ldap/servers/plugins/replication/cl5_api.c b/ldap/servers/plugins/replication/cl5_api.c
index c76cac6..995aefe 100644
--- a/ldap/servers/plugins/replication/cl5_api.c
+++ b/ldap/servers/plugins/replication/cl5_api.c
@@ -335,7 +335,7 @@ static int _cl5GetModSize (LDAPMod *mod);
 static void _cl5ReadBerval (struct berval *bv, char** buff);
 static void _cl5WriteBerval (struct berval *bv, char** buff);
 static int _cl5ReadBervals (struct berval ***bv, char** buff, unsigned int size);
-static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size);
+static int _cl5WriteBervals (struct berval **bv, char** buff, u_int32_t *size);
 
 /* replay iteration */
 #ifdef FOR_DEBUGGING
@@ -2731,7 +2731,7 @@ static int _cl5GetModSize (LDAPMod *mod)
 	{
 		while (mod->mod_bvalues != NULL && mod->mod_bvalues[i] != NULL)
 		{
-			size += mod->mod_bvalues[i]->bv_len + sizeof (mod->mod_bvalues[i]->bv_len); 			
+			size += (PRInt32)mod->mod_bvalues[i]->bv_len + sizeof (PRInt32);
 			i++;
 		}
 	}
@@ -2785,7 +2785,7 @@ static void _cl5WriteBerval (struct berval *bv, char** buff)
 	PRUint32 net_length = 0;
 
 	length = (PRUint32) bv->bv_len;
-    net_length = PR_htonl(length);
+	net_length = PR_htonl(length);
 
 	memcpy(*buff, &net_length, sizeof (net_length));
 	*buff += sizeof (net_length);
@@ -2835,7 +2835,7 @@ static int _cl5ReadBervals (struct berval ***bv, char** buff, unsigned int size)
 }
 
 /* data format: <value count> <value size> <value> <value size> <value> ..... */
-static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size)
+static int _cl5WriteBervals (struct berval **bv, char** buff, u_int32_t *size)
 {
     PRInt32 count, net_count;
     char *pos;
@@ -2847,7 +2847,7 @@ static int _cl5WriteBervals (struct berval **bv, char** buff, unsigned int *size
     *size = sizeof (count); 
     for (count = 0; bv[count]; count ++)
     {
-        *size += sizeof (bv[count]->bv_len) + bv[count]->bv_len;
+        *size += (u_int32_t)(sizeof (PRInt32) + (PRInt32)bv[count]->bv_len);
     }
 
     /* allocate buffer */
diff --git a/ldap/servers/plugins/replication/repl5_protocol.c b/ldap/servers/plugins/replication/repl5_protocol.c
index 8373ba6..34fe8a0 100644
--- a/ldap/servers/plugins/replication/repl5_protocol.c
+++ b/ldap/servers/plugins/replication/repl5_protocol.c
@@ -91,7 +91,7 @@ Repl_Protocol *
 prot_new(Repl_Agmt *agmt, int protocol_state)
 {
 	Slapi_DN *replarea_sdn = NULL;
-	Repl_Protocol *rp = (Repl_Protocol *)slapi_ch_malloc(sizeof(Repl_Protocol));
+	Repl_Protocol *rp = (Repl_Protocol *)slapi_ch_calloc(1, sizeof(Repl_Protocol));
 
 	rp->prp_incremental = rp->prp_total = rp->prp_active_protocol = NULL;
 	if (protocol_state == STATE_PERFORMING_TOTAL_UPDATE)
diff --git a/ldap/servers/slapd/back-ldbm/import.c b/ldap/servers/slapd/back-ldbm/import.c
index d341ea7..04da5a4 100644
--- a/ldap/servers/slapd/back-ldbm/import.c
+++ b/ldap/servers/slapd/back-ldbm/import.c
@@ -1558,6 +1558,7 @@ error:
     instance_set_not_busy(job->inst);
     
     import_free_job(job);
+    FREE(job);
     if (producer)
         FREE(producer);
     
diff --git a/ldap/servers/slapd/daemon.c b/ldap/servers/slapd/daemon.c
index 62c263e..9119dfb 100644
--- a/ldap/servers/slapd/daemon.c
+++ b/ldap/servers/slapd/daemon.c
@@ -1210,6 +1210,7 @@ void slapd_daemon( daemon_ports_t *ports )
 		}
 
 	}
+	slapi_ch_free((void **)&listener_idxs);
 	/* We get here when the server is shutting down */
 	/* Do what we have to do before death */
 
@@ -1819,6 +1820,8 @@ compute_idletimeout( slapdFrontendConfig_t *fecfg, Connection *conn )
 				}
 
 				slapi_sdn_free(&anon_sdn);
+			} else {
+				idletimeout = fecfg->idletimeout;
 			}
 			slapi_ch_free_string(&anon_dn);
 		} else if ( conn->c_isroot ) {
diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c
index d6f9fdd..bf4583c 100644
--- a/ldap/servers/slapd/entry.c
+++ b/ldap/servers/slapd/entry.c
@@ -1127,11 +1127,12 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo )
 		{
 		    /* Failure adding to value tree */
 		    LDAPDebug( LDAP_DEBUG_ANY, "str2entry_dupcheck: unexpected failure %d adding value\n", rc, 0, 0 );
+		    slapi_value_free(&value); /* value not consumed - free it */
 		    slapi_entry_free( e ); e = NULL;
 		    goto free_and_return;
 		}
 
-		slapi_value_free(&value);
+		slapi_value_free(&value); /* if rc is error, value was not consumed - free it */
 	}
 
     /* All done with parsing.  Now create the entry. */
@@ -1200,6 +1201,7 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo )
 					sa->sa_present_values.num,
 					SLAPI_VALUE_FLAG_PASSIN, NULL);
 				sa->sa_present_values.num= 0; /* The values have been consumed */
+				slapi_ch_free((void **)&sa->sa_present_values.va);
 				slapi_valueset_add_attr_valuearray_ext(
 					*a,
 					&(*a)->a_deleted_values,
@@ -1207,6 +1209,7 @@ str2entry_dupcheck( const char *rawdn, char *s, int flags, int read_stateinfo )
 					sa->sa_deleted_values.num,
 					SLAPI_VALUE_FLAG_PASSIN, NULL);
 				sa->sa_deleted_values.num= 0; /* The values have been consumed */
+				slapi_ch_free((void **)&sa->sa_deleted_values.va);
 				if(sa->sa_attributedeletioncsn!=NULL)
 				{
 					attr_set_deletion_csn(*a,sa->sa_attributedeletioncsn);
@@ -1252,8 +1255,8 @@ free_and_return:
     for ( i = 0; i < nattrs; i++ )
     {
 		slapi_ch_free((void **) &(attrs[ i ].sa_type));
-		slapi_ch_free((void **) &(attrs[ i ].sa_present_values.va));
-		slapi_ch_free((void **) &(attrs[ i ].sa_deleted_values.va));
+		slapi_valueset_done(&attrs[ i ].sa_present_values);
+		slapi_valueset_done(&attrs[ i ].sa_deleted_values);
 		attr_done( &attrs[ i ].sa_attr );
     }
 	if (tree_attr_checking)
diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c
index 2a664a1..e473497 100644
--- a/ldap/servers/slapd/valueset.c
+++ b/ldap/servers/slapd/valueset.c
@@ -1046,6 +1046,15 @@ valueset_insert_value_to_sorted(const Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_V
 		
 }
 
+/*
+ * If this function returns an error, it is safe to do both
+ * slapi_valueset_done(vs);
+ * and
+ * valuearray_free(&addvals);
+ * if there is an error and the PASSIN flag is used, the addvals array will own all of the values
+ * vs will own none of the values - so you should do both slapi_valueset_done(vs) and valuearray_free(&addvals)
+ * to clean up
+ */
 int
 slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs, 
 					Slapi_Value **addvals, int naddvals, unsigned long flags, int *dup_index)
@@ -1118,8 +1127,12 @@ slapi_valueset_add_attr_valuearray_ext(const Slapi_Attr *a, Slapi_ValueSet *vs,
 				if (dup < 0 ) {
 					rc = LDAP_TYPE_OR_VALUE_EXISTS;
 					if (dup_index) *dup_index = i;
-					if ( !passin) 
+					if (passin) {
+						/* we have to NULL out the first value so valuearray_free won't delete values in addvals */
+						(vs->va)[0] = NULL;
+					} else {
 						slapi_value_free(&(vs->va)[vs->num]);
+					}
 					break;
 				}
 			} else {
@@ -1348,11 +1361,17 @@ valueset_replace_valuearray_ext(Slapi_Attr *a, Slapi_ValueSet *vs, Slapi_Value *
 	PR_ASSERT((vs->sorted == NULL) || (vs->num == 0) || ((vs->sorted[0] >= 0) && (vs->sorted[0] < vs->num)));
     } else {
 	/* verify the given values are not duplicated.  */
+	unsigned long flags = SLAPI_VALUE_FLAG_PASSIN|SLAPI_VALUE_FLAG_DUPCHECK;
 	Slapi_ValueSet *vs_new = slapi_valueset_new();
-	rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, 0, NULL);
+	rc = slapi_valueset_add_attr_valuearray_ext (a, vs_new, valstoreplace, vals_count, flags, NULL);
 
 	if ( rc == LDAP_SUCCESS )
 	{
+		/* used passin, so vs_new owns all of the Slapi_Value* in valstoreplace
+		 * so tell valuearray_free_ext to start at index vals_count, which is
+		 * NULL, then just free valstoreplace
+		 */
+        	valuearray_free_ext(&valstoreplace, vals_count);
 		/* values look good - replace the values in the attribute */
         	if(!valuearray_isempty(vs->va))
         	{
-- 
1.7.1

