From d8822be083b6ffd4c21004ede8cd14a8246c079c Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Tue, 21 Aug 2012 10:00:21 -0600 Subject: [PATCH] coverity - mbo dead code - winsync leaks, deadcode, null check, test code 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 | 4 - ldap/servers/plugins/replication/windows_private.c | 79 ++++++++++++++----- .../plugins/replication/windows_protocol_util.c | 27 +++++-- 3 files changed, 77 insertions(+), 33 deletions(-) diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index 2ca4d2f..6943d91 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/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 ); diff --git a/ldap/servers/plugins/replication/windows_private.c b/ldap/servers/plugins/replication/windows_private.c index 20accfd..0935c02 100644 --- a/ldap/servers/plugins/replication/windows_private.c +++ b/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"); @@ -1792,6 +1803,7 @@ test_winsync_get_new_ds_user_dn_cb(void *cbdata, const Slapi_Entry *rawentry, "--> test_winsync_get_new_ds_user_dn_cb -- old dn [%s] -- begin\n", *new_dn_string); +#ifdef THIS_IS_JUST_AN_EXAMPLE rdns = slapi_ldap_explode_dn(*new_dn_string, 0); if (!rdns || !rdns[0]) { slapi_ldap_value_free(rdns); @@ -1801,6 +1813,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 +1927,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 +1945,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 +1963,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 +1981,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 +1999,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 +2017,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 +2035,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 +2054,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 +2073,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 +2091,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 +2109,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 +2127,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"); diff --git a/ldap/servers/plugins/replication/windows_protocol_util.c b/ldap/servers/plugins/replication/windows_protocol_util.c index 0b16bde..af3cfa1 100644 --- a/ldap/servers/plugins/replication/windows_protocol_util.c +++ b/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; } -- 1.7.1