Resolves: bug 249366

Bug Description: rhds71 - search filters returns too many entries on interger attributes value greater than 231
Reviewed by: nhosoi (Thanks!)
Fix Description: The problem is that the current code uses atol() to convert the string value to an integer.  long is 4 bytes or 8 bytes depending on the underlying platform.  These binary values are stored in the index as 4 or 8 byte values.  Finally, the behavior of atol() is different on the platform in overflow cases.  Instead of dealing with binary values, we must store the values in string format, and perform string comparison, string normalization, and string key generation on INTEGER syntax values.  I added another syntax type to the list in syntax.h.  The code in string.c and value.c was mostly usable.  I had to add some code in value_normalize to handle cases like "    -00000001" -> "-1" to make it work like atol(), and I had to add some code to value_cmp to handle the sign (e.g. positive is always greater than negative, no other comparison is necessary) and magnitude (longer number is larger/smaller than shorter number, depending on sign).  Otherwise, strcmp() does the right thing (e.g. "50" > "49", the same as int(50) > int(49)).  One problem I ran into was that the value_normalize code takes just a char *, rather than a berval* or a char * + size_t length.  To be efficient, this function should return the new length of the normalized string.  Fortunately, none of the existing code cares about the length, but I needed the length for magnitude comparison, so I just used strlen for those cases.  Which should be fine.  value_normalize always produces a correctly null terminated string.  I rewrote the value_cmp code to use a simple if rather than the switch statement.  This makes it much clearer - if syntax is case insensitive, use slapi_utf8casecmp - if case sensitive, use strcmp - otherwise, error.

I also found a problem with the ldif2db code, which I uncovered because I added my integer indexes online and did an online import.  The db2index code will correctly clear the INDEX_OFFLINE bit after the index is completed, but the ldif2db code will not.
Platforms tested: RHEL5 x86_64
Flag Day: Yes, if you are upgrading and you have integer valued indexes, you will have to remove them and recreate them.
Doc impact: We will have to document this in the release notes.

ldap/servers/plugins/syntaxes/int.c

@@ -50,11 +50,10 @@
 static int int_filter_ava( Slapi_PBlock *pb, struct berval *bvfilter,
 		Slapi_Value **bvals, int ftype, Slapi_Value **retVal );
 static int int_values2keys( Slapi_PBlock *pb, Slapi_Value **val,
-		Slapi_Value ***ivals );
+		Slapi_Value ***ivals, int ftype );
 static int int_assertion2keys( Slapi_PBlock *pb, Slapi_Value *val,
 		Slapi_Value ***ivals, int ftype );
 static int int_compare(struct berval	*v1, struct berval	*v2);
-static long int_to_canonical( long num );
 
 /* the first name is the official one from RFC 2252 */
 static char *names[] = { "INTEGER", "int", INTEGER_SYNTAX_OID, 0 };
@@ -97,103 +96,22 @@ static int
 int_filter_ava( Slapi_PBlock *pb, struct berval *bvfilter,
     Slapi_Value **bvals, int ftype, Slapi_Value **retVal )
 {
-        int     i, rc;
-	long	flong, elong;
-
-	if ( ftype == LDAP_FILTER_APPROX ) {
-		return( LDAP_PROTOCOL_ERROR );
-	}
-	if(retVal) {
-		*retVal=NULL;
-	}
-	flong = atol( bvfilter->bv_val );
-        for ( i = 0; bvals[i] != NULL; i++ ) {
-		elong = atol ( slapi_value_get_string(bvals[i]) );
-		rc = elong - flong;
-                switch ( ftype ) {
-                case LDAP_FILTER_GE:
-                        if ( rc >= 0 ) {
-							    if(retVal) {
-									*retVal = bvals[i];
-								}
-                                return( 0 );
-                        }
-                        break;
-                case LDAP_FILTER_LE:
-                        if ( rc <= 0 ) {
-							    if(retVal) {
-									*retVal = bvals[i];
-								}
-                                return( 0 );
-                        }
-                        break;
-                case LDAP_FILTER_EQUALITY:
-                        if ( rc == 0 ) {
-							    if(retVal) {
-									*retVal = bvals[i];
-								}
-                                return( 0 );
-                        }
-                        break;
-                }
-        }
-
-        return( -1 );
+	return( string_filter_ava( bvfilter, bvals, SYNTAX_INT | SYNTAX_CES,
+                               ftype, retVal ) );
 }
 
 static int
-int_values2keys( Slapi_PBlock *pb, Slapi_Value **vals, Slapi_Value ***ivals )
+int_values2keys( Slapi_PBlock *pb, Slapi_Value **vals, Slapi_Value ***ivals, int ftype )
 {
-	long		num;
-	int		i;
-
-	for ( i = 0; vals[i] != NULL; i++ ) {
-		/* NULL */
-	}
-
-	*ivals = (Slapi_Value **) slapi_ch_malloc(( i + 1 ) * sizeof(Slapi_Value *) );
-
-	for ( i = 0; vals[i] != NULL; i++ )
-	{
-		num = atol( slapi_value_get_string(vals[i]) );
-		num = int_to_canonical( num );
-		(*ivals)[i] = slapi_value_new();
-		slapi_value_set((*ivals)[i],&num,sizeof(long));
-	}
-	(*ivals)[i] = NULL;
-
-	return( 0 );
+	return( string_values2keys( pb, vals, ivals, SYNTAX_INT | SYNTAX_CES,
+                                ftype ) );
 }
 
 static int
 int_assertion2keys( Slapi_PBlock *pb, Slapi_Value *val, Slapi_Value ***ivals, int ftype )
 {
-	long num;
-    size_t len;
-    unsigned char *b;
-    Slapi_Value *tmpval=NULL;
-
-	num = atol( slapi_value_get_string(val) );
-	num = int_to_canonical( num );
-    /* similar to string.c to optimize equality path: avoid malloc/free */
-    if(ftype == LDAP_FILTER_EQUALITY_FAST) {
-        len=sizeof(long);
-        tmpval=(*ivals)[0];
-        if ( len > tmpval->bv.bv_len) {
-            tmpval->bv.bv_val=(char *)slapi_ch_malloc(len);
-        }
-        tmpval->bv.bv_len=len;
-        b = (unsigned char *)&num;
-        memcpy(tmpval->bv.bv_val,b,len);
-    } else {
-        *ivals = (Slapi_Value **) slapi_ch_malloc( 2 * sizeof(Slapi_Value *) );
-        (*ivals)[0] = (Slapi_Value *) slapi_ch_malloc( sizeof(Slapi_Value) );
-        /* XXXSD initialize memory */
-        memset((*ivals)[0],0,sizeof(Slapi_Value));	
-        slapi_value_set((*ivals)[0],&num,sizeof(long));
-        (*ivals)[1] = NULL;
-    }
-	return( 0 );
+	return(string_assertion2keys_ava( pb, val, ivals,
+                                      SYNTAX_INT | SYNTAX_CES, ftype ));
 }
 
 static int int_compare(    
@@ -201,25 +119,5 @@ static int int_compare(
     struct berval	*v2
 )
 {
-	long value1 = atol(v1->bv_val);
-	long value2 = atol(v2->bv_val);
-
-	if (value1 == value2) {
-		return 0;
-	}
-	return ( ((value1 - value2) > 0) ? 1 : -1); 
-}
-
-static long
-int_to_canonical( long num )
-{
-	long ret = 0L;
-	unsigned char *b = (unsigned char *)&ret;
-
-	b[0] = (unsigned char)(num >> 24);
-	b[1] = (unsigned char)(num >> 16);
-	b[2] = (unsigned char)(num >> 8);
-	b[3] = (unsigned char)num;
-
-	return ret;
+	return value_cmp(v1, v2, SYNTAX_INT|SYNTAX_CES, 3 /* Normalise both values */);
 }

ldap/servers/plugins/syntaxes/string.c

@@ -73,6 +73,7 @@ string_filter_ava( struct berval *bvfilter, Slapi_Value **bvals, int syntax,
 	SAFEMEMCPY( bvfilter_norm.bv_val, bvfilter->bv_val, bvfilter->bv_len );
 	bvfilter_norm.bv_val[bvfilter->bv_len] = '\0';
 	value_normalize( bvfilter_norm.bv_val, syntax, 1 /* trim leading blanks */ );
+	bvfilter_norm.bv_len = strlen(bvfilter_norm.bv_val);
 
 	for ( i = 0; bvals[i] != NULL; i++ ) {
 		rc = value_cmp( (struct berval*)slapi_value_get_berval(bvals[i]), &bvfilter_norm, syntax, 1/* Normalise the first value only */ );

ldap/servers/plugins/syntaxes/syntax.h

@@ -56,6 +56,7 @@
 #define SYNTAX_TEL		4	/* telephone number: used with SYNTAX_CIS */
 #define SYNTAX_DN		8	/* distinguished name: used with SYNTAX_CIS */
 #define SYNTAX_SI		16	/* space insensitive: used with SYNTAX_CIS */
+#define SYNTAX_INT		32	/* INTEGER */
 
 #define SUBLEN	3
 

ldap/servers/plugins/syntaxes/value.c

@@ -78,6 +78,13 @@ utf8isspace_fast( char* s )
 ** of the form "cn=a* b*" (note the space) would be wrongly 
 ** normalized into "cn=a*b*", because this function is called
 ** once for "a" and once for " b".
+** richm 20070917 - added integer syntax - note that this implementation
+** of integer syntax tries to mimic the old implementation (atol) as much
+** as possible - leading spaces are ignored, then the optional hyphen for
+** negative numbers, then leading 0s.  That is
+** "    -0000000000001" should normalize to "-1" which is what atol() does
+** Also note that this deviates from rfc 4517 INTEGER syntax, but we must
+** support legacy clients for the time being
 */
 void
 value_normalize(
@@ -105,10 +112,34 @@ value_normalize(
 	      LDAP_UTF8INC(s);
 	  }
 	}
+
+	/* for int syntax, look for leading sign, then trim 0s */
+	/* have to do this after trimming spaces */
+	if (syntax & SYNTAX_INT) {
+		int foundsign = 0;
+		if (*s == '-') {
+			foundsign = 1;
+			LDAP_UTF8INC(s);
+		}
+
+		while (*s && (*s == '0')) {
+			LDAP_UTF8INC(s);
+		}
+
+		/* if there is a hyphen, make sure it is just to the left
+		   of the first significant (i.e. non-zero) digit e.g.
+		   convert -00000001 to -1 */
+		if (foundsign && (s > d)) {
+			*d = '-';
+			d++;
+		}
+		/* s should now point at the first significant digit/char */
+	}
+
 	/* handle value of all spaces - turn into single space */
-	/* unless space insensitive syntax - turn into zero length string */
+	/* unless space insensitive syntax or int - turn into zero length string */
 	if ( *s == '\0' && s != d ) {
-		if ( ! (syntax & SYNTAX_SI)) {
+		if ( ! (syntax & SYNTAX_SI) && ! (syntax & SYNTAX_INT) ) {
 			*d++ = ' ';
 		}
 		*d = '\0';
@@ -175,7 +206,7 @@ value_cmp(
     int			normalize
 )
 {
-	int		rc;
+	int		rc = 0;
 	struct berval bvcopy1;
 	struct berval bvcopy2;
 	char little_buffer[64];
@@ -183,6 +214,7 @@ value_cmp(
 	int buffer_offset = 0;
 	int free_v1 = 0;
 	int free_v2 = 0;
+	int v1sign = 1, v2sign = 1; /* default to positive */
 
 	/* This code used to call malloc up to four times in the copying
 	 * of attributes to be normalized. Now we attempt to keep everything
@@ -221,20 +253,42 @@ value_cmp(
 		value_normalize( v2->bv_val, syntax, 1 /* trim leading blanks */ );
 	}
 
-	switch ( syntax ) {
-	case SYNTAX_CIS:
-	case (SYNTAX_CIS | SYNTAX_TEL):
-	case (SYNTAX_CIS | SYNTAX_DN):
-	case (SYNTAX_CIS | SYNTAX_SI):
-		rc = slapi_utf8casecmp( (unsigned char *)v1->bv_val,
-					(unsigned char *)v2->bv_val );
-		break;
+	if (syntax & SYNTAX_INT) {
+		v1sign = v1->bv_val && (*v1->bv_val != '-');
+		v2sign = v2->bv_val && (*v2->bv_val != '-');
+		rc = v1sign - v2sign;
+		if (rc) { /* one is positive, one is negative */
+			goto done;
+		}
 
-	case SYNTAX_CES:
+		/* check magnitude */
+		/* unfortunately, bv_len cannot be trusted - bv_len is not
+		   updated during or after value_normalize */
+		rc = (strlen(v1->bv_val) - strlen(v2->bv_val));
+		if (rc) {
+			rc = (rc > 0) ? 1 : -1;
+			if (!v1sign && !v2sign) { /* both negative */
+				rc = 0 - rc; /* flip it */
+			}
+			goto done;
+		}
+	}
+
+	if (syntax & SYNTAX_CIS) {
+		rc = slapi_utf8casecmp( (unsigned char *)v1->bv_val,
+								(unsigned char *)v2->bv_val );
+	} else if (syntax & SYNTAX_CES) {
 		rc = strcmp( v1->bv_val, v2->bv_val );
-		break;
+	} else { /* error - unknown syntax */
+		LDAPDebug(LDAP_DEBUG_PLUGIN,
+				  "invalid syntax [%d]\n", syntax, 0, 0);
+	}
+
+	if ((syntax & SYNTAX_INT) && !v1sign && !v2sign) { /* both negative */
+		rc = 0 - rc; /* flip it */
 	}
 
+done:
 	if ( (normalize & 1) && free_v1) {
 		ber_bvfree( v1 );
 	}

ldap/servers/slapd/back-ldbm/import.c

@@ -1054,6 +1054,20 @@ static int import_all_done(ImportJob *job, int ret)
     }
 
     if (job->flags & FLAG_ONLINE) {
+        /* make sure the indexes are online as well */
+        /* richm 20070919 - if index entries are added online, they
+           are created and marked as INDEX_OFFLINE, in anticipation
+           of someone doing a db2index.  In this case, the db2index
+           code will correctly unset the INDEX_OFFLINE flag.
+           However, if import is used to create the indexes, the
+           INDEX_OFFLINE flag will not be cleared.  So, we do that
+           here
+        */
+        IndexInfo *index = job->index_list;
+        while (index != NULL) {
+            index->ai->ai_indexmask &= ~INDEX_OFFLINE;
+            index = index->next;
+        }
         /* start up the instance */
         ret = dblayer_instance_start(job->inst->inst_be, DBLAYER_NORMAL_MODE);
         if (ret != 0)