coverity - mbo dead code - winsync leaks, deadcode, null check, test code

Reviewed by: mreynolds (Thanks!)
This fixes the following issues:
13060 Logically dead code
In memberof_test_membership_callback(): Code can never be reached because of a logical contradiction

13059 Use after free
In windows_plugin_cleanup_agmt(): A pointer to freed memory is dereferenced, used as a function argument, or otherwise used

13058 Dereference before null check
In windows_generate_update_mods(): All paths that lead to this null pointer comparison already dereference the pointer earlier

13057 Dereference before null check
In windows_generate_update_mods(): All paths that lead to this null pointer comparison already dereference the pointer earlier

13056 Resource leak
In windows_plugin_add(): Leak of memory or pointers to system resources

13055 Dereference after null check
In windows_generate_update_mods(): Pointer is checked against null but then dereferenced anyway

13054 Dereference after null check
In test_winsync_pre_ds_search_all_cb(): Pointer is checked against null but then dereferenced anyway

I've also commented out any code from the test winsync plugin except for
logging the entrance and exit of each function.

ldap/servers/plugins/memberof/memberof.c

@@ -1900,10 +1900,6 @@ int memberof_test_membership_callback(Slapi_Entry *e, void *callback_data)
 		goto bail;
 	}
 	slapi_value_set_flags(entry_dn, SLAPI_ATTR_FLAG_NORMALIZED_CIS);
-	if(0 == entry_dn)
-	{
-		goto bail;
-	}
 
 	/* divide groups into member and non-member lists */
 	slapi_entry_attr_find(e, config->memberof_attr, &attr );

ldap/servers/plugins/replication/windows_private.c

@@ -1108,13 +1108,15 @@ windows_plugin_add(void **theapi, int maxapi)
         while (elem && (elem != &winsync_plugin_list)) {
             if (precedence < elem->precedence) {
                 PR_INSERT_BEFORE(wpi, elem);
+                wpi = NULL; /* owned by list now */
                 break;
             }
             elem = PR_NEXT_LINK(elem);
         }
-        if (elem == &winsync_plugin_list) {
+        if (wpi) { /* was not added - precedence too high */
             /* just add to end of list */
             PR_INSERT_BEFORE(wpi, elem);
+            wpi = NULL; /* owned by list now */
         }
         return 0;
     }
@@ -1237,8 +1239,10 @@ windows_plugin_cleanup_agmt(Repl_Agmt *ra)
 
     while (list && !PR_CLIST_IS_EMPTY(list)) {
         elem = PR_LIST_HEAD(list);
-        PR_REMOVE_LINK(elem);
-        slapi_ch_free((void **)&elem);
+        if (elem != list) {
+            PR_REMOVE_LINK(elem);
+            slapi_ch_free((void **)&elem);
+        }
     }
     slapi_ch_free((void **)&list);
     windows_private_set_api_cookie(ra, NULL);
@@ -1686,12 +1690,19 @@ test_winsync_pre_ds_search_all_cb(void *cbdata, const char *agmt_dn,
                     "--> test_winsync_pre_ds_search_all_cb -- orig filter [%s] -- begin\n",
                     ((filter && *filter) ? *filter : "NULL"));
 
-    /* We only want to grab users from the ds side - no groups */
-    slapi_ch_free_string(filter);
-    /* maybe use ntUniqueId=* - only get users that have already been
-       synced with AD already - ntUniqueId and ntUserDomainId are
-       indexed for equality only - need to add presence? */
-    *filter = slapi_ch_strdup("(&(objectclass=ntuser)(ntUserDomainId=*))");
+#ifdef THIS_IS_JUST_AN_EXAMPLE
+    if (filter) {
+        /* We only want to grab users from the ds side - no groups */
+        slapi_ch_free_string(filter);
+        /* maybe use ntUniqueId=* - only get users that have already been
+           synced with AD already - ntUniqueId and ntUserDomainId are
+           indexed for equality only - need to add presence? */
+        *filter = slapi_ch_strdup("(&(objectclass=ntuser)(ntUserDomainId=*))");
+        slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
+                        "--> test_winsync_pre_ds_search_all_cb -- new filter [%s]\n",
+                        *filter ? *filter : "NULL"));
+    }
+#endif
 
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_pre_ds_search_all_cb -- end\n");
@@ -1786,13 +1797,12 @@ test_winsync_get_new_ds_user_dn_cb(void *cbdata, const Slapi_Entry *rawentry,
                                    Slapi_Entry *ad_entry, char **new_dn_string,
                                    const Slapi_DN *ds_suffix, const Slapi_DN *ad_suffix)
 {
-    char **rdns = NULL;
-
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_get_new_ds_user_dn_cb -- old dn [%s] -- begin\n",
                     *new_dn_string);
 
-    rdns = slapi_ldap_explode_dn(*new_dn_string, 0);
+#ifdef THIS_IS_JUST_AN_EXAMPLE
+    char **rdns = slapi_ldap_explode_dn(*new_dn_string, 0);
     if (!rdns || !rdns[0]) {
         slapi_ldap_value_free(rdns);
         return;
@@ -1801,6 +1811,7 @@ test_winsync_get_new_ds_user_dn_cb(void *cbdata, const Slapi_Entry *rawentry,
     slapi_ch_free_string(new_dn_string);
     *new_dn_string = PR_smprintf("%s,%s", rdns[0], slapi_sdn_get_dn(ds_suffix));
     slapi_ldap_value_free(rdns);
+#endif
 
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_get_new_ds_user_dn_cb -- new dn [%s] -- end\n",
@@ -1914,10 +1925,12 @@ test_winsync_post_ad_mod_user_cb(void *cookie, const Slapi_Entry *rawentry, Slap
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_mod_user_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying AD entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ad_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_mod_user_cb -- end\n");
 
@@ -1930,10 +1943,12 @@ test_winsync_post_ad_mod_group_cb(void *cookie, const Slapi_Entry *rawentry, Sla
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_mod_group_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying AD entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ad_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_mod_group_cb -- end\n");
 
@@ -1946,10 +1961,12 @@ test_winsync_post_ds_mod_user_cb(void *cookie, const Slapi_Entry *rawentry, Slap
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ds_mod_user_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying DS entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ds_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ds_mod_user_cb -- end\n");
 
@@ -1962,10 +1979,12 @@ test_winsync_post_ds_mod_group_cb(void *cookie, const Slapi_Entry *rawentry, Sla
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ds_mod_group_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying DS entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ds_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ds_mod_group_cb -- end\n");
 
@@ -1978,10 +1997,12 @@ test_winsync_post_ds_add_user_cb(void *cookie, const Slapi_Entry *rawentry, Slap
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ds_add_user_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of adding DS entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ds_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ds_add_user_cb -- end\n");
 
@@ -1994,10 +2015,12 @@ test_winsync_post_ds_add_group_cb(void *cookie, const Slapi_Entry *rawentry, Sla
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ds_add_group_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of adding DS entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ds_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ds_add_group_cb -- end\n");
 
@@ -2010,11 +2033,13 @@ test_winsync_pre_ad_add_user_cb(void *cookie, Slapi_Entry *ds_entry, Slapi_Entry
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_pre_ad_add_user_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Adding AD entry [%s] from add of DS entry [%s]\n",
                     slapi_entry_get_dn(ad_entry), slapi_entry_get_dn(ds_entry));
     /* make modifications to ad_entry here */
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_pre_ad_add_user_cb -- end\n");
 
@@ -2027,11 +2052,13 @@ test_winsync_pre_ad_add_group_cb(void *cookie, Slapi_Entry *ds_entry, Slapi_Entr
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_pre_ad_add_group_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Adding AD entry [%s] from add of DS entry [%s]\n",
                     slapi_entry_get_dn(ad_entry), slapi_entry_get_dn(ds_entry));
     /* make modifications to ad_entry here */
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_pre_ad_add_group_cb -- end\n");
 
@@ -2044,10 +2071,12 @@ test_winsync_post_ad_add_user_cb(void *cookie, Slapi_Entry *ds_entry, Slapi_Entr
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_add_user_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of adding AD entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ad_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_add_user_cb -- end\n");
 
@@ -2060,10 +2089,12 @@ test_winsync_post_ad_add_group_cb(void *cookie, Slapi_Entry *ds_entry, Slapi_Ent
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_add_group_cb -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of adding AD entry [%s] was [%d:%s]\n",
                     slapi_entry_get_dn(ad_entry), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_add_group_cb -- end\n");
 
@@ -2076,10 +2107,12 @@ test_winsync_post_ad_mod_user_mods_cb(void *cookie, const Slapi_Entry *rawentry,
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_mod_user_mods_cb  -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying AD entry [%s] was [%d:%s]\n",
                     slapi_sdn_get_dn(remote_dn), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_mod_user_mods_cb -- end\n");
 
@@ -2092,10 +2125,12 @@ test_winsync_post_ad_mod_group_mods_cb(void *cookie, const Slapi_Entry *rawentry
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "--> test_winsync_post_ad_mod_group_mods_cb  -- begin\n");
 
+#ifdef THIS_IS_JUST_AN_EXAMPLE
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "Result of modifying AD entry [%s] was [%d:%s]\n",
                     slapi_sdn_get_dn(remote_dn), *result, ldap_err2string(*result));
-    
+#endif
+
     slapi_log_error(SLAPI_LOG_PLUGIN, test_winsync_plugin_name,
                     "<-- test_winsync_post_ad_mod_group_mods_cb -- end\n");
 

ldap/servers/plugins/replication/windows_protocol_util.c

@@ -4204,6 +4204,16 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 	LDAPDebug( LDAP_DEBUG_TRACE, "=> windows_generate_update_mods\n", 0, 0, 0 );
 
 	*do_modify = 0;
+
+        if (!remote_entry || !local_entry) {
+            slapi_log_error(SLAPI_LOG_REPL, windows_repl_plugin_name,
+                            "%s: windows_generate_update_mods: remote_entry is [%s] local_entry is [%s] "
+                            "cannot generate update mods\n", agmt_get_long_name(prp->agmt),
+                            remote_entry ? slapi_entry_get_dn_const(remote_entry) : "NULL",
+                            local_entry ? slapi_entry_get_dn_const(local_entry) : "NULL");
+            goto bail;
+        }
+
 	if (to_windows)
 	{
 		windows_is_local_entry_user_or_group(remote_entry,&is_user,&is_group);
@@ -4212,8 +4222,8 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 		windows_is_remote_entry_user_or_group(remote_entry,&is_user,&is_group);
 	}
 
-    for (rc = slapi_entry_first_attr(remote_entry, &attr); rc == 0;
-			rc = slapi_entry_next_attr(remote_entry, attr, &attr)) 
+        for (rc = slapi_entry_first_attr(remote_entry, &attr); rc == 0;
+             rc = slapi_entry_next_attr(remote_entry, attr, &attr)) 
 	{
 		int is_present_local = 0;
 		char *type = NULL;
@@ -4383,9 +4393,9 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 											"local attribute %s in local entry %s for remote attribute "
 											"%s in remote entry %s\n",
 											local_type ? local_type : "NULL",
-											local_entry ? slapi_entry_get_dn(local_entry) : "NULL",
+											slapi_entry_get_dn(local_entry),
 											type ? type : "NULL",
-											remote_entry ? slapi_entry_get_dn(remote_entry) : "NULL");
+											slapi_entry_get_dn(remote_entry));
 						}
 						slapi_valueset_free(local_values);
 						local_values = NULL;
@@ -4395,9 +4405,9 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 										"local attribute %s in local entry %s for remote attribute "
 										"%s in remote entry %s\n",
 										local_type ? local_type : "NULL",
-										local_entry ? slapi_entry_get_dn(local_entry) : "NULL",
+										slapi_entry_get_dn(local_entry),
 										type ? type : "NULL",
-										remote_entry ? slapi_entry_get_dn(remote_entry) : "NULL");
+										slapi_entry_get_dn(remote_entry));
 					}
 					slapi_valueset_free(mapped_remote_values);
 					mapped_remote_values = NULL;
@@ -4407,9 +4417,9 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 									"local attribute %s in local entry %s for remote attribute "
 									"%s in remote entry %s\n",
 									local_type ? local_type : "NULL",
-									local_entry ? slapi_entry_get_dn(local_entry) : "NULL",
+									slapi_entry_get_dn(local_entry),
 									type ? type : "NULL",
-									remote_entry ? slapi_entry_get_dn(remote_entry) : "NULL");
+									slapi_entry_get_dn(remote_entry));
 				}
 			}
 		} else
@@ -4580,6 +4590,7 @@ windows_generate_update_mods(Private_Repl_Protocol *prp,Slapi_Entry *remote_entr
 	{
 		slapi_mods_dump(smods,"windows sync");
 	}
+bail:
 	LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_generate_update_mods: %d\n", retval, 0, 0 );
 	return retval;
 }