Re: (ITS#4816) slurpd generates invalid ldapadd/modify requests

[gael.roualland@oleane.net]

This is a multi-part message in MIME format.
--------------090002080605050903080106
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit

Hello,

>> As far as I can see, slurpd merely processes the changes in the replog. If it 
>> is generating requests out of sequence, then they must have been recorded 
>> out-of-sequence in the log. And yet I don't see any reason why slapd would 
>> generate the modifications out of sequence.
> 
> That's right for changetype=modify, since the original code aggregates
> multiple values of the same attributes in a signle LDAPmod if they're
> given in order in the replog file.
> But it doesn't do that for changetype=add, which toggled the bug. I
> choosed to use the same code path for both for simplicity when fixing add.
> 
>> What are the circumstances that cause these improper sequences to get into 
>> the replog in the first place?
> 
> In our case, it was just "add" that was problematic. Although I could
> imagine the replog to be generated by something else than slapd, and I'd
> expect slurpd to be able to process it as long as it is valid ldif ?

The second patch I uploaded has an issue with deleting whole attributes.
I have a fixed version of it, but I also wrote another patch to fix the
initial issue only on the problematic case (entry adding), so that's a
less intrusive change : instead of adding a new LDAPMod for each
attribute/value pair, the previous LDAPMod (if any) is reused and the
current value added to it if it's for the same attribute.

The new patch is attached (no space left on ftp). Would it be possible
for such a fix to be in mainline ?
Thanks,

-- 
Gaël Roualland -+- gael.roualland@oleane.net

--------------090002080605050903080106
Content-Type: text/x-patch;
 name="openldap2.3-slurpd-ldapadd.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="openldap2.3-slurpd-ldapadd.diff"

--- openldap-2.3.33/servers/slurpd/ldap_op.c.orig	2007-01-25 12:01:46.000000000 +0100
+++ openldap-2.3.33/servers/slurpd/ldap_op.c	2007-03-01 18:05:44.315232000 +0100
@@ -52,7 +52,6 @@
 #include "slurp.h"
 
 /* Forward references */
-static struct berval **make_singlevalued_berval LDAP_P(( char	*, int ));
 static int op_ldap_add LDAP_P(( Ri *, Re *, char **, int * ));
 static int op_ldap_modify LDAP_P(( Ri *, Re *, char **, int * ));
 static int op_ldap_delete LDAP_P(( Ri *, Re *, char **, int * ));
@@ -183,28 +182,42 @@
 )
 {
     Mi		*mi;
-    int		nattrs, rc = 0, i;
+    int		nattrs, nvals, rc = 0, i;
     LDAPMod	*ldm, **ldmarr;
     int		lderr = 0;
 
     nattrs = i = 0;
     ldmarr = NULL;
+    ldm = NULL;
 
     /*
      * Construct a null-terminated array of LDAPMod structs.
      */
     mi = re->re_mods;
     while ( mi[ i ].mi_type != NULL ) {
-	ldm = alloc_ldapmod();
-	ldmarr = ( LDAPMod ** ) ch_realloc( ldmarr,
-		( nattrs + 2 ) * sizeof( LDAPMod * ));
-	ldmarr[ nattrs ] = ldm;
-	ldm->mod_op = LDAP_MOD_BVALUES;
-	ldm->mod_type = mi[ i ].mi_type;
-	ldm->mod_bvalues =
-		make_singlevalued_berval( mi[ i ].mi_val, mi[ i ].mi_len );
+	if (ldm == NULL || strcmp(ldm->mod_type, mi[ i ].mi_type) != 0) {
+		/* not the same attribute, add a new ldm */
+		ldm = alloc_ldapmod();
+		ldmarr = ( LDAPMod ** ) ch_realloc( ldmarr,
+			( nattrs + 2 ) * sizeof( LDAPMod * ));
+		ldmarr[ nattrs ] = ldm;
+		nattrs++;
+		ldm->mod_op = LDAP_MOD_BVALUES;
+		ldm->mod_type = mi[ i ].mi_type;
+		nvals = 0;
+	}
+
+	ldm->mod_bvalues = ( struct berval ** )
+		ch_realloc( ldm->mod_bvalues,
+		( nvals + 2 ) * sizeof( struct berval * ));
+	ldm->mod_bvalues[ nvals + 1 ] = NULL;
+	ldm->mod_bvalues[ nvals ] = ( struct berval * )
+		ch_malloc( sizeof( struct berval ));
+	ldm->mod_bvalues[ nvals ]->bv_val = mi[ i ].mi_val;
+	ldm->mod_bvalues[ nvals ]->bv_len = mi[ i ].mi_len;
+	nvals++;
+
 	i++;
-	nattrs++;
     }
 
     if ( ldmarr != NULL ) {
@@ -579,25 +592,6 @@
 
 
 /*
- * Create a berval with a single value. 
- */
-static struct berval **
-make_singlevalued_berval( 
-char	*value,
-int	len )
-{
-    struct berval **p;
-
-    p = ( struct berval ** ) ch_malloc( 2 * sizeof( struct berval * ));
-    p[ 0 ] = ( struct berval * ) ch_malloc( sizeof( struct berval ));
-    p[ 1 ] = NULL;
-    p[ 0 ]->bv_val = value;
-    p[ 0 ]->bv_len = len;
-    return( p );
-}
-
-
-/*
  * Given a modification type (string), return an enumerated type.
  * Avoids ugly copy in op_ldap_modify - lets us use a switch statement
  * there.

--------------090002080605050903080106--