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

https://fedorahosted.org/389/ticket/47455
Reviewed by: lkrispenz, nhosoi (Thanks!)
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
(cherry picked from commit 5633e125a462fa174186d1a8adba3ee00c43e3cd)

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 */

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)

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);
     

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 */
 
@@ -1822,6 +1823,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 ) {

ldap/servers/slapd/entry.c

@@ -1106,11 +1106,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. */
@@ -1179,6 +1180,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,
@@ -1186,6 +1188,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);
@@ -1231,8 +1234,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)

ldap/servers/slapd/valueset.c

@@ -1044,6 +1044,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)
@@ -1116,8 +1125,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 {
@@ -1345,11 +1358,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))
         	{