idmap tdb2: fix inconsistent mappings by checking for race and retrying to fetch mapping.

Michael
(This used to be commit cb4c74c9c206e5a445ca636fa6562ce721ea5839)

2 files changed, 22 insertions, 57 deletions

diff --git a/source3/winbindd/idmap.c b/source3/winbindd/idmap.c
index c23919fb18..2962fe6c1c 100644
--- a/source3/winbindd/idmap.c
+++ b/source3/winbindd/idmap.c
@@ -705,6 +705,15 @@ NTSTATUS idmap_new_mapping(const struct dom_sid *psid, enum id_type type,
 
 	status = dom->methods->set_mapping(dom, &map);
 
+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_COLLISION)) {
+		struct id_map *ids[2];
+		DEBUG(5, ("Mapping for %s exists - retrying to map sid\n",
+			  sid_string_dbg(map.sid)));
+		ids[0] = ↦
+		ids[1] = NULL;
+		status = dom->methods->sids_to_unixids(dom, ids);
+	}
+
 	if (!NT_STATUS_IS_OK(status)) {
 		DEBUG(3, ("Could not store the new mapping: %s\n",
 			  nt_errstr(status)));
diff --git a/source3/winbindd/idmap_tdb2.c b/source3/winbindd/idmap_tdb2.c
index 81553dc9c6..a3c867f5bb 100644
--- a/source3/winbindd/idmap_tdb2.c
+++ b/source3/winbindd/idmap_tdb2.c
@@ -790,14 +790,12 @@ static NTSTATUS idmap_tdb2_set_mapping(struct idmap_domain *dom, const struct id
 	TDB_DATA data;
 	char *ksidstr, *kidstr;
 	struct db_record *update_lock = NULL;
-	struct db_record *rec = NULL;
 
 	if (!map || !map->sid) {
 		return NT_STATUS_INVALID_PARAMETER;
 	}
 
 	ksidstr = kidstr = NULL;
-	data.dptr = NULL;
 
 	/* TODO: should we filter a set_mapping using low/high filters ? */
 	
@@ -845,66 +843,25 @@ static NTSTATUS idmap_tdb2_set_mapping(struct idmap_domain *dom, const struct id
 		goto done;
 	}
 
-	/*
-	 * *DELETE* previous mappings if any. *
-	 */
-
-	/* First delete indexed on SID */
-
-	if (((rec = idmap_tdb2_perm->fetch_locked(
-		     idmap_tdb2_perm, update_lock,
-		     string_term_tdb_data(ksidstr))) != NULL)
-	    && (rec->value.dsize != 0)) {
-		struct db_record *rec2;
-
-		if ((rec2 = idmap_tdb2_perm->fetch_locked(
-			     idmap_tdb2_perm, update_lock, rec->value))
-		    != NULL) {
-			rec2->delete_rec(rec2);
-			TALLOC_FREE(rec2);
-		}
-
-		rec->delete_rec(rec);
-
-		tdb_delete(idmap_tdb2_tmp, rec->key);
-		tdb_delete(idmap_tdb2_tmp, rec->value);
-	}
-	TALLOC_FREE(rec);
-
-	/* Now delete indexed on unix ID */
-
-	if (((rec = idmap_tdb2_perm->fetch_locked(
-		     idmap_tdb2_perm, update_lock,
-		     string_term_tdb_data(kidstr))) != NULL)
-	    && (rec->value.dsize != 0)) {
-		struct db_record *rec2;
-
-		if ((rec2 = idmap_tdb2_perm->fetch_locked(
-			     idmap_tdb2_perm, update_lock, rec->value))
-		    != NULL) {
-			rec2->delete_rec(rec2);
-			TALLOC_FREE(rec2);
-		}
-
-		rec->delete_rec(rec);
-
-		tdb_delete(idmap_tdb2_tmp, rec->key);
-		tdb_delete(idmap_tdb2_tmp, rec->value);
+	/* check wheter sid mapping is already present in db */
+	data = tdb2_fetch_bystring(ksidstr, ksidstr);
+	if (data.dptr) {
+		ret = NT_STATUS_OBJECT_NAME_COLLISION;
+		goto done;
 	}
-	TALLOC_FREE(rec);
 
-	if (!NT_STATUS_IS_OK(tdb2_store_bystring(ksidstr, string_term_tdb_data(kidstr),
-				TDB_INSERT))) {
-		DEBUG(0, ("Error storing SID -> ID\n"));
-		ret = NT_STATUS_UNSUCCESSFUL;
+	ret = tdb2_store_bystring(ksidstr, string_term_tdb_data(kidstr),
+				  TDB_INSERT);
+	if (!NT_STATUS_IS_OK(ret)) {
+		DEBUG(0, ("Error storing SID -> ID: %s\n", nt_errstr(ret)));
 		goto done;
 	}
-	if (!NT_STATUS_IS_OK(tdb2_store_bystring(kidstr, string_term_tdb_data(ksidstr),
-				TDB_INSERT))) {
-		DEBUG(0, ("Error storing ID -> SID\n"));
+	ret = tdb2_store_bystring(kidstr, string_term_tdb_data(ksidstr),
+				  TDB_INSERT);
+	if (!NT_STATUS_IS_OK(ret)) {
+		DEBUG(0, ("Error storing ID -> SID: %s\n", nt_errstr(ret)));
 		/* try to remove the previous stored SID -> ID map */
 		tdb2_delete_bystring(ksidstr);
-		ret = NT_STATUS_UNSUCCESSFUL;
 		goto done;
 	}
 
@@ -914,7 +871,6 @@ static NTSTATUS idmap_tdb2_set_mapping(struct idmap_domain *dom, const struct id
 done:
 	talloc_free(ksidstr);
 	talloc_free(kidstr);
-	SAFE_FREE(data.dptr);
 	TALLOC_FREE(update_lock);
 	return ret;
 }