Renames and fix a memory leak

Rename backend_staged_data to backend_staged_search.
Fix some formatting.
Change how we walk the list of entries retrieved using a staged search
so that if the map's been removed since the search was staged, we still
free the temporary entry structures.

3 files changed, 56 insertions, 55 deletions

diff --git a/src/back-sch-nss.c b/src/back-sch-nss.c
index a45d42d..c8aaa20 100644
--- a/src/back-sch-nss.c
+++ b/src/back-sch-nss.c
@@ -465,20 +465,20 @@ repeat:
 	return entries;
 }
 
-/* Check filter for a component like (uid=<value>) and if found,
- * stage NSSWITCH lookup. Lookup will be performed later, with call to backend_retrieve_from_nsswitch */
+/* Check if the filter is one (like uid=<value>) that should trigger an
+ * nsswitch lookup, and if it is, make a note that we should perform such a
+ * lookup. */
 void
 backend_search_nsswitch(struct backend_set_data *set_data,
 			struct backend_search_cbdata *cbdata)
 {
 	int result, rc;
 	struct backend_search_filter_config config = {FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, FALSE, NULL};
-	struct backend_staged_data *staged = NULL;
+	struct backend_staged_search *staged = NULL;
 	char *idptr = NULL;
 	unsigned long id;
 
-	/* There was no match but we asked to check NSSWITCH */
-	/* First, we search the filter to see if it includes cn|uid=<value> check */
+	/* First, we search the filter to see if it includes a cn|uid=<value> test. */
 	result = slapi_filter_apply(cbdata->filter,
 				    backend_search_filter_has_cn_uid, &config, &rc);
 	if ((result != SLAPI_FILTER_SCAN_STOP)) {
@@ -515,22 +515,21 @@ backend_search_nsswitch(struct backend_set_data *set_data,
 		}
 	}
 
-
-	staged = malloc(sizeof(struct backend_staged_data));
+	staged = malloc(sizeof(*staged));
 	if (staged == NULL) {
 		goto fail;
 	}
 
 	staged->map_group = slapi_ch_strdup(set_data->common.group);
 	staged->map_set = slapi_ch_strdup(set_data->common.set);
-	staged->set_data_fixup = NULL;
+	staged->set_data = NULL;
 	staged->count = 0;
 	staged->entries = NULL;
 
 	staged->container_sdn = slapi_ch_strdup(slapi_sdn_get_dn(set_data->container_sdn));
 
 	staged->type = cbdata->check_nsswitch;
-	staged->name = config.name;
+	staged->name = config.name; /* takes ownership */
 	staged->is_id = config.search_gid || config.search_uid;
 	staged->search_members = config.search_members;
 
@@ -543,9 +542,10 @@ fail:
 	return;
 }
 
- /* perform look up against NSSWITCH and create entry based on that */
+ /* Actually look up the information that we previously noted that we should,
+  * then convert whatever we find into one or more Slapi_Entry pointers. */
 bool_t
-backend_retrieve_from_nsswitch(struct backend_staged_data *staged,
+backend_retrieve_from_nsswitch(struct backend_staged_search *staged,
 			       struct backend_search_cbdata *cbdata)
 {
 	Slapi_Entry *entry, **entries;
@@ -554,7 +554,7 @@ backend_retrieve_from_nsswitch(struct backend_staged_data *staged,
 	if (((staged->type == SCH_NSSWITCH_GROUP) && staged->search_members) &&
 	    (NULL != staged->name)) {
 		entries = backend_retrieve_group_list_from_nsswitch(staged->name, staged->container_sdn,
-								cbdata, &staged->count);
+								    cbdata, &staged->count);
 		if (entries != NULL) {
 			staged->entries = entries;
 			for (i = 0; i < staged->count; i++) {
@@ -567,11 +567,11 @@ backend_retrieve_from_nsswitch(struct backend_staged_data *staged,
 
 	if ((staged->type == SCH_NSSWITCH_GROUP) && (NULL != staged->name)) {
 		entry = backend_retrieve_group_entry_from_nsswitch(staged->name, staged->is_id,
-							       staged->container_sdn,
-							       cbdata);
-		if (entry) {
+							           staged->container_sdn,
+							           cbdata);
+		if (entry != NULL) {
 			slapi_entry_add_string(entry, "schema-compat-origin", "nsswitch");
-			staged->entries = malloc(sizeof(Slapi_Entry *));
+			staged->entries = malloc(sizeof(staged->entries[0]));
 			if (staged->entries != NULL) {
 				staged->entries[0] = entry;
 				staged->count = 1;
@@ -585,11 +585,11 @@ backend_retrieve_from_nsswitch(struct backend_staged_data *staged,
 
 	if ((staged->type == SCH_NSSWITCH_USER) && (NULL != staged->name)) {
 		entry = backend_retrieve_user_entry_from_nsswitch(staged->name, staged->is_id,
-							      staged->container_sdn,
-							      cbdata);
-		if (entry) {
+							          staged->container_sdn,
+							          cbdata);
+		if (entry != NULL) {
 			slapi_entry_add_string(entry, "schema-compat-origin", "nsswitch");
-			staged->entries = malloc(sizeof(Slapi_Entry *));
+			staged->entries = malloc(sizeof(staged->entries[0]));
 			if (staged->entries != NULL) {
 				staged->entries[0] = entry;
 				staged->count = 1;
diff --git a/src/back-sch.c b/src/back-sch.c
index 63ee110..8359056 100644
--- a/src/back-sch.c
+++ b/src/back-sch.c
@@ -155,7 +155,8 @@ backend_set_config_read_config(struct plugin_state *state, Slapi_Entry *e,
 			       const char *group, const char *container,
 			       bool_t *flag, struct backend_shr_set_data **pret)
 {
-	char **bases, *entry_filter, **attributes, *rdn_format, *dn, *nsswitch_min_id, *check_nsswitch, *strp;
+	char **bases, *entry_filter, **attributes, *rdn_format, *dn;
+	char *nsswitch_min_id, *check_nsswitch, *strp;
 	bool_t check_access;
 	struct backend_set_data ret;
 	Slapi_DN *tmp_sdn;
@@ -238,7 +239,8 @@ backend_set_config_read_config(struct plugin_state *state, Slapi_Entry *e,
 		/* If we're adding nsswitch-based entries to this map, make
 		 * sure that we copy the schema-compat-origin and SID
 		 * attributes, so that we can read the former during the BIND
-		 * callback. */
+		 * callback.  FIXME: store that in the entry's backend_data to
+		 * avoid surprising clients. */
 		backend_shr_add_strlist(&ret.attribute_format, "objectClass=extensibleObject");
 		backend_shr_add_strlist(&ret.attribute_format, "schema-compat-origin=%{schema-compat-origin}");
 		backend_shr_add_strlist(&ret.attribute_format, "ipaNTSecurityIdentifier=%{ipaNTSecurityIdentifier}");
@@ -1005,7 +1007,7 @@ backend_search_set_cb(const char *group, const char *set, bool_t flag,
 	struct backend_set_data *set_data;
 	Slapi_Entry *set_entry;
 	int result, n_entries;
-	int n_entries_nsswitch;
+	int n_entries_without_nsswitch;
 	const char *ndn;
 
 	cbdata = cb_data;
@@ -1013,9 +1015,10 @@ backend_search_set_cb(const char *group, const char *set, bool_t flag,
 	cbdata->check_access = set_data->check_access;
 	cbdata->check_nsswitch = set_data->check_nsswitch;
 	cbdata->nsswitch_min_id = set_data->nsswitch_min_id;
-	/* If any entries were actually returned by the descending callback,
-	 * avoid to look up in nsswitch even if this set is marked to look up */
-	n_entries_nsswitch = cbdata->n_entries;
+
+	/* Count the number of results that we've found before looking at this
+	 * set of entries. */
+	n_entries_without_nsswitch = cbdata->n_entries;
 
 	/* Check the set itself, unless it's also the group, in which case we
 	 * already evaluated it for this search. */
@@ -1071,14 +1074,13 @@ backend_search_set_cb(const char *group, const char *set, bool_t flag,
 		map_data_foreach_entry_id(cbdata->state, group, set, NULL,
 					  backend_search_entry_cb, cbdata);
 #ifdef USE_NSSWITCH
-		/* If we didn't find an exact match for the entry but asked to look up NSSWITCH,
-		 * then try to search NSSWITCH. If search filters would match, the search will be
-		 * staged for retrieval. The retrieval process is run after lookup is completed
-		 * for all maps as we need to ensure there is no contention for the global
-		 * map cache lock. The contention might occur if NSSWITCH would need to re-kinit
-		 * its kerberos credentials -- this would cause changes in the original LDAP tree
-		 * which, in turn, will trigger modification of the map cache entries. */
-		if ((n_entries_nsswitch == cbdata->n_entries) &&
+		/* If we didn't find a matching entry in this set, but we're
+		 * configured to also consult nsswitch, check if the search
+		 * filter is one that should trigger an nsswitch lookup, and
+		 * make a note if it would.  We'll come back and actually
+		 * perform the lookup later when we're not holding a lock that
+		 * can stall other threads. */
+		if ((cbdata->n_entries == n_entries_without_nsswitch) &&
 		    (cbdata->check_nsswitch != SCH_NSSWITCH_NONE)) {
 			backend_search_nsswitch(set_data, cbdata);
 		}
@@ -1109,7 +1111,7 @@ backend_search_find_set_data_in_group_cb(const char *group, const char *set, boo
 
 	if ((0 == strcmp(group, cbdata->cur_staged->map_group)) &&
 	    (0 == strcmp(set, cbdata->cur_staged->map_set))) {
-		cbdata->cur_staged->set_data_fixup = set_data;
+		cbdata->cur_staged->set_data = set_data;
 	}
 
 	return TRUE;
@@ -1123,7 +1125,7 @@ backend_search_find_set_data_cb(const char *group, void *cb_data)
 
 	cbdata = cb_data;
 	map_data_foreach_map(cbdata->state, group,
-			     backend_search_find_set_data_in_group_cb, cbdata);
+			     backend_search_find_set_data_in_group_cb, cb_data);
 	return TRUE;
 }
 
@@ -1230,7 +1232,7 @@ static int
 backend_search_cb(Slapi_PBlock *pb)
 {
 	struct backend_search_cbdata cbdata;
-	struct backend_staged_data *staged, *next;
+	struct backend_staged_search *staged, *next;
 	int i;
 
 	if (wrap_get_call_level() > 0) {
@@ -1298,7 +1300,7 @@ backend_search_cb(Slapi_PBlock *pb)
 		/* Go over the list of staged requests and retrieve entries.
 		 * It is important to perform the retrieval *without* holding any locks to the map cache */
 		staged = cbdata.staged;
-		while (staged) {
+		while (staged != NULL) {
 			if (staged->entries == NULL) {
 				backend_retrieve_from_nsswitch(staged, &cbdata);
 			}
@@ -1309,22 +1311,21 @@ backend_search_cb(Slapi_PBlock *pb)
 		/* Add the entries to the map cache */
 		wrap_inc_call_level();
 		map_wrlock();
-		while (staged) {
+		while (staged != NULL) {
 			if (staged->entries) {
 				cbdata.cur_staged = staged;
 				/* We actually need to find the original set first */
 				map_data_foreach_domain(cbdata.state, backend_search_find_set_data_cb, &cbdata);
-				if (cbdata.cur_staged->set_data_fixup != NULL) {
-					for (i = 0; i < staged->count ; i++) {
-						if (staged->entries[i] != NULL) {
-							if (!map_data_check_entry(cbdata.state,
-										  staged->map_group, staged->map_set,
-										  slapi_sdn_get_ndn(staged->entries[i]))) {
-								backend_set_entry(cbdata.pb, staged->entries[i], staged->set_data_fixup);
-							}
-							slapi_entry_free(staged->entries[i]);
-							staged->entries[i] = NULL;
+				for (i = 0; i < staged->count ; i++) {
+					if (staged->entries[i] != NULL) {
+						if ((cbdata.cur_staged->set_data != NULL) &&
+						    !map_data_check_entry(cbdata.state,
+									  staged->map_group, staged->map_set,
+									  slapi_sdn_get_ndn(staged->entries[i]))) {
+							backend_set_entry(cbdata.pb, staged->entries[i], staged->set_data);
 						}
+						slapi_entry_free(staged->entries[i]);
+						staged->entries[i] = NULL;
 					}
 				}
 				free(staged->entries);
diff --git a/src/back-sch.h b/src/back-sch.h
index 3b91c66..7887127 100644
--- a/src/back-sch.h
+++ b/src/back-sch.h
@@ -45,10 +45,10 @@ struct backend_entry_data {
 	Slapi_Entry *e;
 };
 
-struct backend_staged_data {
-	struct backend_staged_data *next;
-	struct backend_set_data *set_data_fixup;
+struct backend_staged_search {
+	struct backend_staged_search *next;
 	char *map_group, *map_set;
+	struct backend_set_data *set_data;
 	enum sch_search_nsswitch_t type;
 	bool_t is_id;
 	bool_t search_members;
@@ -79,14 +79,14 @@ struct backend_search_cbdata {
 	bool_t matched;
 	char *closest_match, *text;
 	int n_entries;
-	struct backend_staged_data *staged;
-	struct backend_staged_data *cur_staged;
+	struct backend_staged_search *staged;
+	struct backend_staged_search *cur_staged;
 };
 
 void backend_search_nsswitch(struct backend_set_data *set_data,
 			     struct backend_search_cbdata *cbdata);
 
-bool_t backend_retrieve_from_nsswitch(struct backend_staged_data *staged,
+bool_t backend_retrieve_from_nsswitch(struct backend_staged_search *staged,
 				      struct backend_search_cbdata *cbdata);
 
 int backend_sch_do_pam_auth(Slapi_PBlock *pb, const char *username);