[PATCH] Remove fstring from wb_acct_info

[Samuel Cabrero]

Hi,

the attached patch removes two fstrings from wb_acct_info struct. The
reason for this change is because the winbindd group enumeration
backend functions (ADS in particular) try to allocate an array of
wb_acct_info as long as the number of groups in the domain, which may
result in a huge chunk of memory for domains with a large number of
groups.

Branch:
https://gitlab.com/samuelcabrero/samba/commits/winbind_enum_grp_nomem

CI:
https://gitlab.com/samuelcabrero/samba/pipelines/34956873


Please review and push if you agree.
-------------- next part --------------
From 218f58f374d7d79a62b43b407c6785b91e6c7a16 Mon Sep 17 00:00:00 2001
From: Samuel Cabrero <scabrero at suse.de>
Date: Tue, 30 Oct 2018 18:47:16 +0100
Subject: [PATCH] s3: winbind: Remove fstring from wb_acct_info struct

The group enumeration backend functions try to allocate an array of
wb_acct_info structs with a number of elements equal to the number of
groups. In domains with a large number of groups this allocation may
fail due to the size of the chunk.

Found while trying to enumerate the groups in a domain with more than
700k groups.

Signed-off-by: Samuel Cabrero <scabrero at suse.de>
---
 source3/winbindd/winbindd.h       |  4 ++--
 source3/winbindd/winbindd_ads.c   |  8 ++++----
 source3/winbindd/winbindd_cache.c |  8 ++++----
 source3/winbindd/winbindd_rpc.c   | 16 ++++++++++++----
 4 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h
index 57371765484..6d4b92f27cf 100644
--- a/source3/winbindd/winbindd.h
+++ b/source3/winbindd/winbindd.h
@@ -189,8 +189,8 @@ struct winbindd_domain {
 };
 
 struct wb_acct_info {
-	fstring acct_name; /* account name */
-	fstring acct_desc; /* account name */
+	const char *acct_name; /* account name */
+	const char *acct_desc; /* account name */
 	uint32_t rid; /* domain-relative RID */
 };
 
diff --git a/source3/winbindd/winbindd_ads.c b/source3/winbindd/winbindd_ads.c
index 76d6a304366..abc044d54ac 100644
--- a/source3/winbindd/winbindd_ads.c
+++ b/source3/winbindd/winbindd_ads.c
@@ -500,8 +500,8 @@ static NTSTATUS enum_dom_groups(struct winbindd_domain *domain,
 		struct dom_sid sid;
 		uint32_t rid;
 
-		name = ads_pull_username(ads, mem_ctx, msg);
-		gecos = ads_pull_string(ads, mem_ctx, msg, "name");
+		name = ads_pull_username(ads, (*info), msg);
+		gecos = ads_pull_string(ads, (*info), msg, "name");
 		if (!ads_pull_sid(ads, msg, "objectSid", &sid)) {
 			DEBUG(1,("No sid for %s !?\n", name));
 			continue;
@@ -512,8 +512,8 @@ static NTSTATUS enum_dom_groups(struct winbindd_domain *domain,
 			continue;
 		}
 
-		fstrcpy((*info)[i].acct_name, name);
-		fstrcpy((*info)[i].acct_desc, gecos);
+		(*info)[i].acct_name = name;
+		(*info)[i].acct_desc = gecos;
 		(*info)[i].rid = rid;
 		i++;
 	}
diff --git a/source3/winbindd/winbindd_cache.c b/source3/winbindd/winbindd_cache.c
index 7d98d613ac4..2e2a04d09d8 100644
--- a/source3/winbindd/winbindd_cache.c
+++ b/source3/winbindd/winbindd_cache.c
@@ -1565,8 +1565,8 @@ do_fetch_cache:
 		smb_panic_fn("enum_dom_groups out of memory");
 	}
 	for (i=0; i<(*num_entries); i++) {
-		fstrcpy((*info)[i].acct_name, centry_string(centry, mem_ctx));
-		fstrcpy((*info)[i].acct_desc, centry_string(centry, mem_ctx));
+		(*info)[i].acct_name = centry_string(centry, (*info));
+		(*info)[i].acct_desc = centry_string(centry, (*info));
 		(*info)[i].rid = centry_uint32(centry);
 	}
 
@@ -1660,8 +1660,8 @@ do_fetch_cache:
 		smb_panic_fn("enum_dom_groups out of memory");
 	}
 	for (i=0; i<(*num_entries); i++) {
-		fstrcpy((*info)[i].acct_name, centry_string(centry, mem_ctx));
-		fstrcpy((*info)[i].acct_desc, centry_string(centry, mem_ctx));
+		(*info)[i].acct_name = centry_string(centry, (*info));
+		(*info)[i].acct_desc = centry_string(centry, (*info));
 		(*info)[i].rid = centry_uint32(centry);
 	}
 
diff --git a/source3/winbindd/winbindd_rpc.c b/source3/winbindd/winbindd_rpc.c
index f50fb8fa5db..6f7cb07f4e3 100644
--- a/source3/winbindd/winbindd_rpc.c
+++ b/source3/winbindd/winbindd_rpc.c
@@ -155,9 +155,13 @@ NTSTATUS rpc_enum_dom_groups(TALLOC_CTX *mem_ctx,
 		for (g = 0; g < count; g++) {
 			struct wb_acct_info *i = &info[num_info + g];
 
-			fstrcpy(i->acct_name,
+			i->acct_name = talloc_strdup(info,
 				sam_array->entries[g].name.string);
-			fstrcpy(i->acct_desc, "");
+			if (i->acct_name == NULL) {
+				TALLOC_FREE(info);
+				return NT_STATUS_NO_MEMORY;
+			}
+			i->acct_desc = NULL;
 			i->rid = sam_array->entries[g].idx;
 		}
 
@@ -217,9 +221,13 @@ NTSTATUS rpc_enum_local_groups(TALLOC_CTX *mem_ctx,
 		for (g = 0; g < count; g++) {
 			struct wb_acct_info *i = &info[num_info + g];
 
-			fstrcpy(i->acct_name,
+			i->acct_name = talloc_strdup(info,
 				sam_array->entries[g].name.string);
-			fstrcpy(i->acct_desc, "");
+			if (i->acct_name == NULL) {
+				TALLOC_FREE(info);
+				return NT_STATUS_NO_MEMORY;
+			}
+			i->acct_desc = NULL;
 			i->rid = sam_array->entries[g].idx;
 		}
 
-- 
2.19.1