Ticket #47960 - cookie_change_info returns random negative number if there was no change in a tree

Description: An additional fix for the type mismatch for the change
numbers.  Change Number is declared as "unsigned long" in the Retro
Changelog plugin, while cookie_change_info in the Content Sync plugin
is "int", which could end up with a negative number when the change
number passes (2^31 - 1).

Changing the type of cookie_change_info to "unsigned long".

https://fedorahosted.org/389/ticket/47960

Reviewed by [email protected] and [email protected] (Thank you,
Rich and Thierry!!)

ldap/servers/plugins/sync/sync.h

@@ -64,10 +64,12 @@
 #define CL_ATTR_NEWSUPERIOR "newsuperior"
 #define CL_SRCH_BASE "cn=changelog"
 
+#define SYNC_INVALID_CHANGENUM ((unsigned long)-1)
+
 typedef struct sync_cookie {
 	char *cookie_client_signature;
 	char *cookie_server_signature;
-	int cookie_change_info;
+	unsigned long cookie_change_info;
 } Sync_Cookie;
 
 typedef struct sync_update {
@@ -80,8 +82,8 @@ typedef struct sync_update {
 
 typedef struct sync_callback {
 	Slapi_PBlock *orig_pb;
-	int changenr;
-	int change_start;
+	unsigned long changenr;
+	unsigned long change_start;
 	int cb_err;
 	Sync_UpdateNode *cb_updates;
 } Sync_CallBackData;
@@ -112,6 +114,7 @@ int sync_cookie_isvalid (Sync_Cookie *testcookie, Sync_Cookie *refcookie);
 void sync_cookie_free (Sync_Cookie **freecookie);
 char * sync_cookie2str(Sync_Cookie *cookie);
 int sync_number2int(char *nrstr);
+unsigned long sync_number2ulong(char *nrstr);
 char *sync_nsuniqueid2uuid(const char *nsuniqueid);
 
 int sync_is_active (Slapi_Entry *e, Slapi_PBlock *pb);

ldap/servers/plugins/sync/sync_refresh.c

@@ -293,9 +293,9 @@ sync_refresh_update_content(Slapi_PBlock *pb, Sync_Cookie *client_cookie, Sync_C
 	cb_data.orig_pb = pb;
 	cb_data.change_start = client_cookie->cookie_change_info;
 
-	filter = slapi_ch_smprintf("(&(changenumber>=%d)(changenumber<=%d))",
-					client_cookie->cookie_change_info,
-					server_cookie->cookie_change_info);
+	filter = slapi_ch_smprintf("(&(changenumber>=%lu)(changenumber<=%lu))",
+	                           client_cookie->cookie_change_info,
+	                           server_cookie->cookie_change_info);
 	slapi_search_internal_set_pb(
 		seq_pb, 
 		CL_SRCH_BASE,
@@ -305,7 +305,7 @@ sync_refresh_update_content(Slapi_PBlock *pb, Sync_Cookie *client_cookie, Sync_C
 		0,
 		NULL, NULL, 
 		plugin_get_default_component_id(),
-		0);							  
+		0);
 
 	rc = slapi_search_internal_callback_pb (
 		seq_pb, &cb_data, NULL, sync_read_entry_from_changelog, NULL);
@@ -460,6 +460,7 @@ sync_read_entry_from_changelog( Slapi_Entry *cl_entry, void *cb_data)
 	int chg_req;
 	int prev = 0;
 	int index = 0;
+	unsigned long chgnum = 0;
 	Sync_CallBackData *cb = (Sync_CallBackData *) cb_data;
 
 	if (cb == NULL) {
@@ -470,13 +471,28 @@ sync_read_entry_from_changelog( Slapi_Entry *cl_entry, void *cb_data)
 	if (uniqueid == NULL) {
 		slapi_log_error (SLAPI_LOG_FATAL, SYNC_PLUGIN_SUBSYSTEM, 
 			"Retro Changelog does not provied nsuniquedid."
-			"Check RCL plugin configuration." );
+			"Check RCL plugin configuration.\n" );
 		return(1);
 	}
-	chgtype = sync_get_attr_value_from_entry (cl_entry, CL_ATTR_CHGTYPE);
 	chgnr = sync_get_attr_value_from_entry (cl_entry, CL_ATTR_CHANGENUMBER);
-
-	index = sync_number2int(chgnr) - cb->change_start;
+	chgnum = sync_number2ulong(chgnr);
+	if (SYNC_INVALID_CHANGENUM == chgnum) {
+		slapi_log_error (SLAPI_LOG_FATAL, SYNC_PLUGIN_SUBSYSTEM, 
+			"Change number provided by Retro Changelog is invalid: %s\n", chgnr);
+		slapi_ch_free_string(&chgnr);
+		slapi_ch_free_string(&uniqueid);
+		return(1);
+	}
+	if (chgnum < cb->change_start) {
+		slapi_log_error (SLAPI_LOG_FATAL, SYNC_PLUGIN_SUBSYSTEM, 
+			"Change number provided by Retro Changelog %s is less than the initial number %lu\n",
+			chgnr, cb->change_start);
+		slapi_ch_free_string(&chgnr);
+		slapi_ch_free_string(&uniqueid);
+		return(1);
+	}
+	index = chgnum - cb->change_start;
+	chgtype = sync_get_attr_value_from_entry (cl_entry, CL_ATTR_CHGTYPE);
 	chg_req = sync_str2chgreq(chgtype);
 	switch (chg_req){ 
 		case LDAP_REQ_ADD:

ldap/servers/plugins/sync/sync_util.c

@@ -266,10 +266,10 @@ sync_cookie2str(Sync_Cookie *cookie)
 	char *cookiestr = NULL;
 
 	if (cookie) {
-		cookiestr = slapi_ch_smprintf("%s#%s#%d",
-				cookie->cookie_server_signature,
-				cookie->cookie_client_signature,
-				cookie->cookie_change_info);
+		cookiestr = slapi_ch_smprintf("%s#%s#%lu",
+		                               cookie->cookie_server_signature,
+		                               cookie->cookie_client_signature,
+		                               cookie->cookie_change_info);
 	}
 	return(cookiestr);
 }
@@ -370,10 +370,11 @@ sync_handle_cnum_entry(Slapi_Entry *e, void *cb_data)
 			slapi_attr_first_value( chattr,&sval );
 			if ( NULL != sval ) {
 				value = slapi_value_get_berval ( sval );
-				if( NULL != value && NULL != value->bv_val &&
-					'\0' != value->bv_val[0]) {
-					cb->changenr = sync_number2int(value->bv_val);
-					cb->cb_err = 0; /* changenr successfully set */
+				if (value && value->bv_val && ('\0' != value->bv_val[0])) {
+					cb->changenr = sync_number2ulong(value->bv_val);
+					if (SYNC_INVALID_CHANGENUM != cb->changenr) {
+						cb->cb_err = 0; /* changenr successfully set */
+					}
 				}
 			}
 		}
@@ -452,31 +453,30 @@ sync_cookie_get_client_info(Slapi_PBlock *pb)
 	clientinfo = slapi_ch_smprintf("%s:%s:%s",clientdn,targetdn,strfilter);
 	return (clientinfo);
 }
-static int
+static unsigned long
 sync_cookie_get_change_number(int lastnr, const char *uniqueid)
 {
 	Slapi_PBlock *srch_pb;
 	Slapi_Entry **entries;
 	Slapi_Entry *cl_entry;
 	int rv;
-	int newnr = -1;
+	unsigned long newnr = SYNC_INVALID_CHANGENUM;
 	char *filter = slapi_ch_smprintf("(&(changenumber>=%d)(targetuniqueid=%s))",lastnr,uniqueid);
 
 	srch_pb = slapi_pblock_new();
-    	slapi_search_internal_set_pb(srch_pb, CL_SRCH_BASE,
-           				LDAP_SCOPE_SUBTREE, filter,
-            				NULL, 0, NULL, NULL, plugin_get_default_component_id(), 0);
- 	slapi_search_internal_pb(srch_pb);
+	slapi_search_internal_set_pb(srch_pb, CL_SRCH_BASE, LDAP_SCOPE_SUBTREE, filter,
+	                             NULL, 0, NULL, NULL, plugin_get_default_component_id(), 0);
+	slapi_search_internal_pb(srch_pb);
 	slapi_pblock_get(srch_pb, SLAPI_PLUGIN_INTOP_RESULT, &rv);
-	if ( rv == LDAP_SUCCESS) {
+	if (rv == LDAP_SUCCESS) {
 		slapi_pblock_get(srch_pb, SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES, &entries);
-    		if (entries && *entries) {
+		if (entries && *entries) {
 			Slapi_Attr *attr;
 			Slapi_Value *val;
 			cl_entry = *entries; /* only use teh first one */
 			slapi_entry_attr_find(cl_entry, CL_ATTR_CHANGENUMBER, &attr);
 			slapi_attr_first_value(attr, &val);
-			newnr = sync_number2int((char *)slapi_value_get_string(val));
+			newnr = sync_number2ulong((char *)slapi_value_get_string(val));
 		}
 	}
 
@@ -579,8 +579,8 @@ sync_cookie_parse (char *cookie)
 		if (p) {
 			*p = '\0';
 			sc->cookie_client_signature = slapi_ch_strdup(q);
-			sc->cookie_change_info = sync_number2int(p+1);
-			if (sc->cookie_change_info < 0) {
+			sc->cookie_change_info = sync_number2ulong(p+1);
+			if (SYNC_INVALID_CHANGENUM == sc->cookie_change_info) {
 				goto error_return;
 			}
 		} else {
@@ -716,14 +716,28 @@ sync_pblock_copy(Slapi_PBlock *src)
 	return dest;
 }
 
-int sync_number2int(char *chgnrstr)
+int
+sync_number2int(char *chgnrstr)
 {
 	char *end;
 	int nr;
-	nr = strtoul(chgnrstr, &end, 10);
+	nr = (int)strtoul(chgnrstr, &end, 10);
 	if ( *end == '\0') {
 		return (nr);
 	} else {
 		return (-1);
 	}
 }
+
+unsigned long
+sync_number2ulong(char *chgnrstr)
+{
+	char *end;
+	unsigned long nr;
+	nr = strtoul(chgnrstr, &end, 10);
+	if ( *end == '\0') {
+		return (nr);
+	} else {
+		return SYNC_INVALID_CHANGENUM;
+	}
+}