Resolves: bug 458675

Bug Description: Memory leaks in valueset code
Reviewed by: nkinder,nhosoi (Thanks!)
Branch: HEAD
Fix Description: The first leak occurs when you are using replication and you add values to an
attribute that were previously deleted - that is, the values that you want to
add are on the attribute's deleted values list and are being "resurrected".
This leak is caused by an improper bit test (foo & bar|baz).  The or | has
higher precedence and is evaluated first.  The fix is to use parentheses (foo &
(bar|baz)).  Note that this issue was flagged by the compiler gcc with -Wall.
The second leak is caused when several values are being added to an attribute,
and the list contains non-sequential duplicate values (e.g. foo, bar, baz,
foo).  The code uses an array of Slapi_Value* called keyvals.  When a valid
value is found, the Slapi_Value* is moved from keyvals to valuetreep and the
keyvals array index is set to NULL.  This array is passed to valuearray_free to
free the individual Slapi_Value* and the array itself.  This works fine in the
non-error case because there are no Slapi_Value* elements to free, so it just
frees the array.  However, in the duplicate value case, some of the elements
have already been set to NULL, so those are skipped over by valuearray_free.
The fix is to introduce a new function valuearray_free_ext that takes an
additional argument which is the array index to start freeing from.  That way
the non-NULL Slapi_Value* elements can be freed along with the array itself.
Platforms tested: RHEL5, Fedora 8
Flag Day: no
Doc impact: no
QA impact: should be covered by regular nightly and manual testing
New Tests integrated into TET: none

1 files changed, 19 insertions, 9 deletions

diff --git a/ldap/servers/slapd/valueset.c b/ldap/servers/slapd/valueset.c
index 61e0724f..3df302f3 100644
--- a/ldap/servers/slapd/valueset.c
+++ b/ldap/servers/slapd/valueset.c
@@ -311,20 +311,25 @@ valuearray_get_bervalarray(Slapi_Value **cvals,struct berval ***bvals)
 }
 
 void
-valuearray_free(Slapi_Value ***va)
+valuearray_free_ext(Slapi_Value ***va, int idx)
 {
 	if(va!=NULL && *va!=NULL)
 	{
-		int i;
-		for(i=0; (*va)[i]!=NULL; i++)
+		for(; (*va)[idx]!=NULL; idx++)
 		{
-			slapi_value_free(&(*va)[i]);
+			slapi_value_free(&(*va)[idx]);
 		}
 		slapi_ch_free((void **)va);
 		*va = NULL;
 	}
 }
 
+void
+valuearray_free(Slapi_Value ***va)
+{
+    valuearray_free_ext(va, 0);
+}
+
 int
 valuearray_next_value( Slapi_Value **va, int index, Slapi_Value **v)
 {
@@ -663,7 +668,12 @@ valuetree_add_valuearray( const char *type, struct slapdplugin *pi, Slapi_Value
 					}
 				}
 			}
-			valuearray_free( &keyvals );
+			/* start freeing at index i - the rest of them have already
+			   been moved into valuetreep
+			   the loop iteration will always do the +1, so we have
+			   to remove it if so */
+			i = (i > 0) ? i-1 : 0;
+			valuearray_free_ext( &keyvals, i );
 		}
 	}
 	if(rc!=0)
@@ -1121,8 +1131,8 @@ valueset_remove_valuearray(Slapi_ValueSet *vs, const Slapi_Attr *a, Slapi_Value
 							if ( va_out )
 							{
 								if (vs->va[index]->v_csnset &&
-									(flags & SLAPI_VALUE_FLAG_PRESERVECSNSET|
-											 SLAPI_VALUE_FLAG_USENEWVALUE))
+									(flags & (SLAPI_VALUE_FLAG_PRESERVECSNSET|
+                                              SLAPI_VALUE_FLAG_USENEWVALUE)))
 								{
 									valuestodelete[i]->v_csnset = csnset_dup (vs->va[index]->v_csnset);
 								}
@@ -1192,8 +1202,8 @@ valueset_remove_valuearray(Slapi_ValueSet *vs, const Slapi_Attr *a, Slapi_Value
 					if ( va_out )
 					{
 						if (found->v_csnset &&
-							(flags & SLAPI_VALUE_FLAG_PRESERVECSNSET|
-							 		 SLAPI_VALUE_FLAG_USENEWVALUE))
+							(flags & (SLAPI_VALUE_FLAG_PRESERVECSNSET|
+                                      SLAPI_VALUE_FLAG_USENEWVALUE)))
 						{
 							valuestodelete[i]->v_csnset = csnset_dup (found->v_csnset);
 						}