This patch fixes one of my longest-standing pet hates with Samba :-).

When we look see if a user is in a list, and we try to 'expand' an @group, we
should lookup the user's own list of groups, rather than looking for all the
members of a group.

I'm sure this will fix some nasty performance issues, particularly on large
domains etc.  In particular, this avoids contacting winbind at all, if the
group is not a winbind group.

(This caused a deadlock on my winbind-on-PDC setup).

The groups list always includes the user's primary group, as per the
getgrouplist manpage, and my recent changes to our implementation.

Andrew Bartlett

2 files changed, 47 insertions, 18 deletions

diff --git a/source/lib/username.c b/source/lib/username.c
index b1c9ca0f08a..b8f33494ee4 100644
--- a/source/lib/username.c
+++ b/source/lib/username.c
@@ -169,7 +169,7 @@ BOOL map_username(char *user)
 			return False;
 		}
 
-		if (strchr_m(dosname,'*') || user_in_list(user, (const char **)dosuserlist)) {
+		if (strchr_m(dosname,'*') || user_in_list(user, (const char **)dosuserlist, NULL, 0)) {
 			DEBUG(3,("Mapped user %s to %s\n",user,unixname));
 			mapped_user = True;
 			fstrcpy(last_from,user);
@@ -328,11 +328,27 @@ static BOOL user_in_winbind_group_list(const char *user, const char *gname, BOOL
 	int num_groups;
 	int i;
  	gid_t *groups = NULL;
- 	gid_t gid;
+ 	gid_t gid, gid_low, gid_high;
  	BOOL ret = False;
  
  	*winbind_answered = False;
  
+	if ((gid = nametogid(gname)) == (gid_t)-1) {
+ 		DEBUG(0,("user_in_winbind_group_list: nametogid for group %s failed.\n",
+ 			gname ));
+ 		goto err;
+ 	}
+
+	if (!lp_winbind_gid(&gid_low, &gid_high)) {
+		DEBUG(4, ("winbind gid range not configured, therefore %s cannot be a winbind group\n", gname));
+ 		goto err;
+	}
+
+	if (gid < gid_low || gid > gid_high) {
+		DEBUG(4, ("group %s is not a winbind group\n", gname));
+ 		goto err;
+	}
+ 
  	/*
  	 * Get the gid's that this user belongs to.
  	 */
@@ -361,12 +377,6 @@ failed with error %s\n", strerror(errno) ));
 	 * to a gid_t via either winbind or the local UNIX lookup and do the comparison.
 	 */
  
-	if ((gid = nametogid(gname)) == (gid_t)-1) {
- 		DEBUG(0,("user_in_winbind_group_list: winbind_lookup_name for group %s failed.\n",
- 			gname ));
- 		goto err;
- 	}
- 
  	for (i = 0; i < num_groups; i++) {
  		if (gid == groups[i]) {
  			ret = True;
@@ -389,7 +399,7 @@ failed with error %s\n", strerror(errno) ));
  Check if a user is in a UNIX group.
 ****************************************************************************/
 
-static BOOL user_in_unix_group_list(const char *user,const char *gname)
+BOOL user_in_unix_group_list(const char *user,const char *gname)
 {
 	struct passwd *pass = Get_Pwnam(user);
 	struct sys_userlist *user_list;
@@ -432,10 +442,27 @@ static BOOL user_in_unix_group_list(const char *user,const char *gname)
  Check if a user is in a group list. Ask winbind first, then use UNIX.
 ****************************************************************************/
 
-BOOL user_in_group_list(const char *user, const char *gname)
+BOOL user_in_group_list(const char *user, const char *gname, gid_t *groups, size_t n_groups)
 {
 	BOOL winbind_answered = False;
 	BOOL ret;
+	gid_t gid;
+	unsigned i;
+
+	gid = nametogid(gname);
+	if (gid == (gid_t)-1) 
+		return False;
+
+	if (groups && n_groups > 0) {
+		for (i=0; i < n_groups; i++) {
+			if (groups[i] == gid) {
+				return True;
+			}
+		}
+		return False;
+	}
+
+	/* fallback if we don't yet have the group list */
 
 	ret = user_in_winbind_group_list(user, gname, &winbind_answered);
 	if (!winbind_answered)
@@ -451,7 +478,7 @@ BOOL user_in_group_list(const char *user, const char *gname)
  and netgroup lists.
 ****************************************************************************/
 
-BOOL user_in_list(const char *user,const char **list)
+BOOL user_in_list(const char *user,const char **list, gid_t *groups, size_t n_groups)
 {
 	if (!list || !*list)
 		return False;
@@ -480,7 +507,7 @@ BOOL user_in_list(const char *user,const char **list)
 			 */
 			if(user_in_netgroup_list(user, *list +1))
 				return True;
-			if(user_in_group_list(user, *list +1))
+			if(user_in_group_list(user, *list +1, groups, n_groups))
 				return True;
 		} else if (**list == '+') {
 
@@ -488,7 +515,7 @@ BOOL user_in_list(const char *user,const char **list)
 				/*
 				 * Search UNIX list followed by netgroup.
 				 */
-				if(user_in_group_list(user, *list +2))
+				if(user_in_group_list(user, *list +2, groups, n_groups))
 					return True;
 				if(user_in_netgroup_list(user, *list +2))
 					return True;
@@ -499,7 +526,7 @@ BOOL user_in_list(const char *user,const char **list)
 				 * Just search UNIX list.
 				 */
 
-				if(user_in_group_list(user, *list +1))
+				if(user_in_group_list(user, *list +1, groups, n_groups))
 					return True;
 			}
 
@@ -511,7 +538,7 @@ BOOL user_in_list(const char *user,const char **list)
 				 */
 				if(user_in_netgroup_list(user, *list +2))
 					return True;
-				if(user_in_group_list(user, *list +2))
+				if(user_in_group_list(user, *list +2, groups, n_groups))
 					return True;
 			} else {
 				/*
diff --git a/source/lib/util_str.c b/source/lib/util_str.c
index 17c7cc29fe4..cd906d37c00 100644
--- a/source/lib/util_str.c
+++ b/source/lib/util_str.c
@@ -442,6 +442,8 @@ char *safe_strcpy(char *dest,const char *src, size_t maxlength)
 		return NULL;
 	}
 
+	dest[maxlength]='\0';
+
 	if (!src) {
 		*dest = 0;
 		return dest;
@@ -450,8 +452,8 @@ char *safe_strcpy(char *dest,const char *src, size_t maxlength)
 	len = strlen(src);
 
 	if (len > maxlength) {
-		DEBUG(0,("ERROR: string overflow by %d in safe_strcpy [%.50s]\n",
-			 (int)(len-maxlength), src));
+		DEBUG(0,("ERROR: string overflow by %u (%u - %u) in safe_strcpy [%.50s]\n",
+			 (unsigned int)(len-maxlength), len, maxlength, src));
 		len = maxlength;
 	}
       
@@ -1597,7 +1599,7 @@ char * base64_encode_data_blob(DATA_BLOB data)
 {
 	int bits = 0;
 	int char_count = 0;
-	int out_cnt = 0;
+	size_t out_cnt = 0;
 	size_t len = data.length;
 	size_t output_len = data.length * 2;
 	char *result = malloc(output_len); /* get us plenty of space */