From 3b61c03ee575881538a408d1ceebe0770e80409a Mon Sep 17 00:00:00 2001 From: Rich Megginson Date: Aug 28 2012 16:18:30 +0000 Subject: coverity - posix winsync mem leaks, null check, deadcode, null ref, use after free Fixes the following issues: 13068 Resource leak In posix_winsync_pre_ds_mod_group_cb(): Leak of memory or pointers to system resources. 13067 Resource leak In posix_winsync_end_update_cb(): Leak of memory or pointers to system resources 13066 Resource leak In modGroupMembership(): Leak of memory or pointers to system resources 13065 Resource leak In dn_in_set(): Leak of memory or pointers to system resources 13064 Resource leak In dn_in_set(): Leak of memory or pointers to system resources 13063 Dereference after null check In windows_plugin_add(): Pointer is checked against null but then dereferenced anyway 13062 Explicit null dereferenced In searchUid(): Dereference of an explicit null value 13061 Logically dead code In check_account_lock(): 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 Reviewed by: nkinder (Thanks!) Branch: 389-ds-base-1.2.11 (cherry picked from commit 1a9b5ffe655c7ec8dd935106376ad8dabe69b561) (cherry picked from commit 0c24dfcff939c6c30d336cc498f074d743daf302) --- diff --git a/ldap/servers/plugins/posix-winsync/posix-group-func.c b/ldap/servers/plugins/posix-winsync/posix-group-func.c index 72963ac..d4bbcfe 100644 --- a/ldap/servers/plugins/posix-winsync/posix-group-func.c +++ b/ldap/servers/plugins/posix-winsync/posix-group-func.c @@ -86,7 +86,7 @@ searchUid(const char *udn) } slapi_free_search_results_internal(int_search_pb); slapi_pblock_destroy(int_search_pb); - if (posix_winsync_config_get_lowercase()) { + if (uid && posix_winsync_config_get_lowercase()) { return slapi_dn_ignore_case(uid); } return uid; @@ -103,11 +103,15 @@ int dn_in_set(const char* uid, char **uids) { int i; - Slapi_DN *sdn_uid = slapi_sdn_new_dn_byval(uid); - Slapi_DN *sdn_ul = slapi_sdn_new(); + Slapi_DN *sdn_uid = NULL; + Slapi_DN *sdn_ul = NULL; if (uids == NULL || uid == NULL) return false; + + sdn_uid = slapi_sdn_new_dn_byval(uid); + sdn_ul = slapi_sdn_new(); + for (i = 0; uids[i]; i++) { slapi_sdn_set_dn_byref(sdn_ul, uids[i]); if (slapi_sdn_compare(sdn_uid, sdn_ul) == 0) { @@ -346,7 +350,9 @@ modGroupMembership(Slapi_Entry *entry, Slapi_Mods *smods, int *do_modify) "modGroupMembership: add to modlist %s\n", uid); doModify = true; } - /* slapi_value_free(&v); */ + slapi_valueset_free(vs); + slapi_value_init_berval(v, NULL); /* otherwise we will try to free memory we do not own */ + slapi_value_free(&v); } } } diff --git a/ldap/servers/plugins/posix-winsync/posix-winsync.c b/ldap/servers/plugins/posix-winsync/posix-winsync.c index 6eddc23..dc56475 100644 --- a/ldap/servers/plugins/posix-winsync/posix-winsync.c +++ b/ldap/servers/plugins/posix-winsync/posix-winsync.c @@ -184,8 +184,8 @@ check_account_lock(Slapi_Entry *ds_entry, int *isvirt) rc = 1; /* no attr == entry is enabled */ slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name, "<-- check_account_lock - entry [%s] does not " - "have attribute nsAccountLock - entry %s locked\n", - slapi_entry_get_dn_const(ds_entry), rc ? "is not" : "is"); + "have attribute nsAccountLock - entry is not locked\n", + slapi_entry_get_dn_const(ds_entry)); } return rc; @@ -918,7 +918,7 @@ posix_winsync_pre_ds_mod_group_cb(void *cbdata, const Slapi_Entry *rawentry, Sla slapi_value_init_string(voc, "posixGroup"); slapi_entry_attr_find(ds_entry, "objectClass", &oc_attr); if (slapi_attr_value_find(oc_attr, slapi_value_get_berval(voc)) != 0) { - Slapi_ValueSet *oc_vs = slapi_valueset_new(); + Slapi_ValueSet *oc_vs = NULL; Slapi_Value *oc_nv = slapi_value_new(); slapi_attr_get_valueset(oc_attr, &oc_vs); @@ -933,7 +933,7 @@ posix_winsync_pre_ds_mod_group_cb(void *cbdata, const Slapi_Entry *rawentry, Sla slapi_value_free(&oc_nv); slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name, "_pre_ds_mod_group_cb step\n"); - /* slapi_valuset_free(oc_vs); */ + slapi_valueset_free(oc_vs); slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name, "_pre_ds_mod_group_cb step\n"); } @@ -1292,6 +1292,7 @@ posix_winsync_end_update_cb(void *cbdata, const Slapi_DN *ds_subtree, const Slap slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name, "--> posix_winsync_end_update_cb, create task %s\n", dn); if (NULL == dn) { + slapi_pblock_destroy(pb); slapi_log_error(SLAPI_LOG_FATAL, posix_winsync_plugin_name, "posix_winsync_end_update_cb: " "failed to create task dn: cn=%s,%s,cn=tasks,cn=config\n", diff --git a/ldap/servers/plugins/replication/windows_private.c b/ldap/servers/plugins/replication/windows_private.c index 5698d37..66e1e98 100644 --- a/ldap/servers/plugins/replication/windows_private.c +++ b/ldap/servers/plugins/replication/windows_private.c @@ -1241,7 +1241,7 @@ windows_plugin_add(void **theapi, int maxapi) } elem = PR_NEXT_LINK(elem); } - if (wpi) { /* was not added - precedence too high */ + if (elem && wpi) { /* was not added - precedence too high */ /* just add to end of list */ PR_INSERT_BEFORE(wpi, elem); wpi = NULL; /* owned by list now */ @@ -1367,10 +1367,8 @@ windows_plugin_cleanup_agmt(Repl_Agmt *ra) while (list && !PR_CLIST_IS_EMPTY(list)) { elem = PR_LIST_HEAD(list); - if (elem != list) { - PR_REMOVE_LINK(elem); - slapi_ch_free((void **)&elem); - } + PR_REMOVE_LINK(elem); + slapi_ch_free((void **)&elem); } slapi_ch_free((void **)&list); windows_private_set_api_cookie(ra, NULL);