[SCM] Samba Shared Repository - branch master updated

Jeremy Allison jra at samba.org
Fri Oct 5 19:05:02 MDT 2012


The branch, master has been updated
       via  8287938 We should never just assign an st_mode to an ace->perms field, theoretically they are different so should go through a mapping function. Ensure this is so.
       via  47ebc8f Modify ensure_canon_entry_valid() into ensure_canon_entry_valid_on_set() - makes the logic clearer.
       via  9466cd1 Simplify ensure_canon_entry_valid by splitting out the _get codepath.
      from  36ea39e talloc: Convert error cecking macros into fns

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit 828793852f3785c620f2716c60f8b1640880ee50
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Oct 5 15:51:19 2012 -0700

    We should never just assign an st_mode to an ace->perms field, theoretically
    they are different so should go through a mapping function. Ensure this is so.
    
    Practically this does not matter, as for user permissions the mapping
    function is an identity, and the extra bits we may add are ignored
    anyway, but this makes the intent clear.
    
    Autobuild-User(master): Jeremy Allison <jra at samba.org>
    Autobuild-Date(master): Sat Oct  6 03:04:14 CEST 2012 on sn-devel-104

commit 47ebc8fbc93ee1eb9640d9ca30275fcfc3b50026
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Oct 5 15:48:07 2012 -0700

    Modify ensure_canon_entry_valid() into ensure_canon_entry_valid_on_set() - makes the logic clearer.

commit 9466cd189d6a07411f451f7596feee36f0be7f32
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Oct 5 15:09:06 2012 -0700

    Simplify ensure_canon_entry_valid by splitting out the _get codepath.

-----------------------------------------------------------------------

Summary of changes:
 source3/smbd/posix_acls.c |  386 ++++++++++++++++++++++++++------------------
 1 files changed, 228 insertions(+), 158 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 503727f..45a921f 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -1342,34 +1342,117 @@ static bool uid_entry_in_group(connection_struct *conn, canon_ace *uid_ace, cano
 }
 
 /****************************************************************************
- A well formed POSIX file or default ACL has at least 3 entries, a 
+ A well formed POSIX file or default ACL has at least 3 entries, a
  SMB_ACL_USER_OBJ, SMB_ACL_GROUP_OBJ, SMB_ACL_OTHER_OBJ.
  In addition, the owner must always have at least read access.
  When using this call on get_acl, the pst struct is valid and contains
- the mode of the file. When using this call on set_acl, the pst struct has
+ the mode of the file.
+****************************************************************************/
+
+static bool ensure_canon_entry_valid_on_get(connection_struct *conn,
+					canon_ace **pp_ace,
+					const struct dom_sid *pfile_owner_sid,
+					const struct dom_sid *pfile_grp_sid,
+					const SMB_STRUCT_STAT *pst)
+{
+	canon_ace *pace;
+	bool got_user = false;
+	bool got_group = false;
+	bool got_other = false;
+
+	for (pace = *pp_ace; pace; pace = pace->next) {
+		if (pace->type == SMB_ACL_USER_OBJ) {
+			got_user = true;
+		} else if (pace->type == SMB_ACL_GROUP_OBJ) {
+			got_group = true;
+		} else if (pace->type == SMB_ACL_OTHER) {
+			got_other = true;
+		}
+	}
+
+	if (!got_user) {
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("malloc fail.\n"));
+			return false;
+		}
+
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_USER_OBJ;
+		pace->owner_type = UID_ACE;
+		pace->unix_ug.type = ID_TYPE_UID;
+		pace->unix_ug.id = pst->st_ex_uid;
+		pace->trustee = *pfile_owner_sid;
+		pace->attr = ALLOW_ACE;
+		pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRUSR, S_IWUSR, S_IXUSR);
+		DLIST_ADD(*pp_ace, pace);
+	}
+
+	if (!got_group) {
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("malloc fail.\n"));
+			return false;
+		}
+
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_GROUP_OBJ;
+		pace->owner_type = GID_ACE;
+		pace->unix_ug.type = ID_TYPE_GID;
+		pace->unix_ug.id = pst->st_ex_gid;
+		pace->trustee = *pfile_grp_sid;
+		pace->attr = ALLOW_ACE;
+		pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRGRP, S_IWGRP, S_IXGRP);
+		DLIST_ADD(*pp_ace, pace);
+	}
+
+	if (!got_other) {
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("malloc fail.\n"));
+			return false;
+		}
+
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_OTHER;
+		pace->owner_type = WORLD_ACE;
+		pace->unix_ug.type = ID_TYPE_NOT_SPECIFIED;
+		pace->unix_ug.id = -1;
+		pace->trustee = global_sid_World;
+		pace->attr = ALLOW_ACE;
+		pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IROTH, S_IWOTH, S_IXOTH);
+		DLIST_ADD(*pp_ace, pace);
+	}
+
+	return true;
+}
+
+/****************************************************************************
+ A well formed POSIX file or default ACL has at least 3 entries, a
+ SMB_ACL_USER_OBJ, SMB_ACL_GROUP_OBJ, SMB_ACL_OTHER_OBJ.
+ In addition, the owner must always have at least read access.
+ When using this call on set_acl, the pst struct has
  been modified to have a mode containing the default for this file or directory
  type.
 ****************************************************************************/
 
-static bool ensure_canon_entry_valid(connection_struct *conn,
+static bool ensure_canon_entry_valid_on_set(connection_struct *conn,
 					canon_ace **pp_ace,
 					bool is_default_acl,
 					const struct share_params *params,
 					const bool is_directory,
 					const struct dom_sid *pfile_owner_sid,
 					const struct dom_sid *pfile_grp_sid,
-					const SMB_STRUCT_STAT *pst,
-					bool setting_acl)
+					const SMB_STRUCT_STAT *pst)
 {
 	canon_ace *pace;
 	canon_ace *pace_user = NULL;
 	canon_ace *pace_group = NULL;
 	canon_ace *pace_other = NULL;
+	bool got_duplicate_user = false;
+	bool got_duplicate_group = false;
 
 	for (pace = *pp_ace; pace; pace = pace->next) {
 		if (pace->type == SMB_ACL_USER_OBJ) {
 
-			if (setting_acl && !is_default_acl) {
+			if (!is_default_acl) {
 				apply_default_perms(params, is_directory, pace, S_IRUSR);
 			}
 			pace_user = pace;
@@ -1380,7 +1463,7 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 			 * Ensure create mask/force create mode is respected on set.
 			 */
 
-			if (setting_acl && !is_default_acl) {
+			if (!is_default_acl) {
 				apply_default_perms(params, is_directory, pace, S_IRGRP);
 			}
 			pace_group = pace;
@@ -1391,7 +1474,7 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 			 * Ensure create mask/force create mode is respected on set.
 			 */
 
-			if (setting_acl && !is_default_acl) {
+			if (!is_default_acl) {
 				apply_default_perms(params, is_directory, pace, S_IROTH);
 			}
 			pace_other = pace;
@@ -1402,16 +1485,18 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 			 * Ensure create mask/force create mode is respected on set.
 			 */
 
-			if (setting_acl && !is_default_acl) {
+			if (!is_default_acl) {
 				apply_default_perms(params, is_directory, pace, S_IRGRP);
 			}
 		}
 	}
 
 	if (!pace_user) {
+		canon_ace *pace_iter;
+
 		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-			DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n"));
-			return False;
+			DEBUG(0,("talloc fail.\n"));
+			return false;
 		}
 
 		ZERO_STRUCTP(pace);
@@ -1421,42 +1506,37 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 		pace->unix_ug.id = pst->st_ex_uid;
 		pace->trustee = *pfile_owner_sid;
 		pace->attr = ALLOW_ACE;
-		/* Start with existing permissions, principle of least
+		/* Start with existing user permissions, principle of least
 		   surprises for the user. */
-		pace->perms = pst->st_ex_mode;
+		pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRUSR, S_IWUSR, S_IXUSR);
 
-		if (setting_acl) {
-			/* See if the owning user is in any of the other groups in
-			   the ACE, or if there's a matching user entry (by uid
-			   or in the case of ID_TYPE_BOTH by SID).
-			   If so, OR in the permissions from that entry. */
+		/* See if the owning user is in any of the other groups in
+		   the ACE, or if there's a matching user entry (by uid
+		   or in the case of ID_TYPE_BOTH by SID).
+		   If so, OR in the permissions from that entry. */
 
-			canon_ace *pace_iter;
 
-			for (pace_iter = *pp_ace; pace_iter; pace_iter = pace_iter->next) {
-				if (pace_iter->type == SMB_ACL_USER &&
-						pace_iter->unix_ug.id == pace->unix_ug.id) {
+		for (pace_iter = *pp_ace; pace_iter; pace_iter = pace_iter->next) {
+			if (pace_iter->type == SMB_ACL_USER &&
+					pace_iter->unix_ug.id == pace->unix_ug.id) {
+				pace->perms |= pace_iter->perms;
+			} else if (pace_iter->type == SMB_ACL_GROUP_OBJ || pace_iter->type == SMB_ACL_GROUP) {
+				if (dom_sid_equal(&pace->trustee, &pace_iter->trustee)) {
+					pace->perms |= pace_iter->perms;
+				} else if (uid_entry_in_group(conn, pace, pace_iter)) {
 					pace->perms |= pace_iter->perms;
-				} else if (pace_iter->type == SMB_ACL_GROUP_OBJ || pace_iter->type == SMB_ACL_GROUP) {
-					if (dom_sid_equal(&pace->trustee, &pace_iter->trustee)) {
-						pace->perms |= pace_iter->perms;
-					} else if (uid_entry_in_group(conn, pace, pace_iter)) {
-						pace->perms |= pace_iter->perms;
-					}
 				}
 			}
+		}
 
-			if (pace->perms == 0) {
-				/* If we only got an "everyone" perm, just use that. */
-				if (pace_other)
-					pace->perms = pace_other->perms;
-			}
+		if (pace->perms == 0) {
+			/* If we only got an "everyone" perm, just use that. */
+			if (pace_other)
+				pace->perms = pace_other->perms;
+		}
 
-			if (!is_default_acl) {
-				apply_default_perms(params, is_directory, pace, S_IRUSR);
-			}
-		} else {
-			pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRUSR, S_IWUSR, S_IXUSR);
+		if (!is_default_acl) {
+			apply_default_perms(params, is_directory, pace, S_IRUSR);
 		}
 
 		DLIST_ADD(*pp_ace, pace);
@@ -1465,8 +1545,8 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 
 	if (!pace_group) {
 		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-			DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n"));
-			return False;
+			DEBUG(0,("talloc fail.\n"));
+			return false;
 		}
 
 		ZERO_STRUCTP(pace);
@@ -1476,17 +1556,15 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 		pace->unix_ug.id = pst->st_ex_gid;
 		pace->trustee = *pfile_grp_sid;
 		pace->attr = ALLOW_ACE;
-		if (setting_acl) {
-			/* If we only got an "everyone" perm, just use that. */
-			if (pace_other)
-				pace->perms = pace_other->perms;
-			else
-				pace->perms = 0;
-			if (!is_default_acl) {
-				apply_default_perms(params, is_directory, pace, S_IRGRP);
-			}
+
+		/* If we only got an "everyone" perm, just use that. */
+		if (pace_other) {
+			pace->perms = pace_other->perms;
 		} else {
-			pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRGRP, S_IWGRP, S_IXGRP);
+			pace->perms = 0;
+		}
+		if (!is_default_acl) {
+			apply_default_perms(params, is_directory, pace, S_IRGRP);
 		}
 
 		DLIST_ADD(*pp_ace, pace);
@@ -1495,8 +1573,8 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 
 	if (!pace_other) {
 		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-			DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n"));
-			return False;
+			DEBUG(0,("talloc fail.\n"));
+			return false;
 		}
 
 		ZERO_STRUCTP(pace);
@@ -1506,126 +1584,118 @@ static bool ensure_canon_entry_valid(connection_struct *conn,
 		pace->unix_ug.id = -1;
 		pace->trustee = global_sid_World;
 		pace->attr = ALLOW_ACE;
-		if (setting_acl) {
-			pace->perms = 0;
-			if (!is_default_acl) {
-				apply_default_perms(params, is_directory, pace, S_IROTH);
-			}
-		} else
-			pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IROTH, S_IWOTH, S_IXOTH);
+		pace->perms = 0;
+		if (!is_default_acl) {
+			apply_default_perms(params, is_directory, pace, S_IROTH);
+		}
 
 		DLIST_ADD(*pp_ace, pace);
 		pace_other = pace;
 	}
 
-	if (setting_acl) {
-		/* Ensure when setting a POSIX ACL, that the uid for a
-		   SMB_ACL_USER_OBJ ACE (the owner ACE entry) has a duplicate
-		   permission entry as an SMB_ACL_USER, and a gid for a
-		   SMB_ACL_GROUP_OBJ ACE (the primary group ACE entry) also has
-		   a duplicate permission entry as an SMB_ACL_GROUP. If not,
-		   then if the ownership or group ownership of this file or
-		   directory gets changed, the user or group can lose their
-		   access. */
-		bool got_duplicate_user = false;
-		bool got_duplicate_group = false;
-
-		for (pace = *pp_ace; pace; pace = pace->next) {
-			if (pace->type == SMB_ACL_USER &&
-					pace->unix_ug.id == pace_user->unix_ug.id) {
-				/* Already got one. */
-				got_duplicate_user = true;
-			} else if (pace->type == SMB_ACL_GROUP &&
-					pace->unix_ug.id == pace_user->unix_ug.id) {
-				/* Already got one. */
-				got_duplicate_group = true;
-			} else if ((pace->type == SMB_ACL_GROUP)
-				   && (dom_sid_equal(&pace->trustee, &pace_user->trustee))) {
-				/* If the SID owning the file appears
-				 * in a group entry, then we have
-				 * enough duplication, they will still
-				 * have access */
-				got_duplicate_user = true;
-			}
-		}
-
-		/* If the SID is equal for the user and group that we need
-		   to add the duplicate for, add only the group */
-		if (!got_duplicate_user && !got_duplicate_group
-				&& dom_sid_equal(&pace_group->trustee,
-						&pace_user->trustee)) {
-			/* Add a duplicate SMB_ACL_GROUP entry, this
-			 * will cover the owning SID as well, as it
-			 * will always be mapped to both a uid and
-			 * gid. */
-
-			if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-				DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n"));
-				return false;
-			}
-
-			ZERO_STRUCTP(pace);
-			pace->type = SMB_ACL_GROUP;;
-			pace->owner_type = GID_ACE;
-			pace->unix_ug.type = ID_TYPE_GID;
-			pace->unix_ug.id = pace_group->unix_ug.id;
-			pace->trustee = pace_group->trustee;
-			pace->attr = pace_group->attr;
-			pace->perms = pace_group->perms;
-
-			DLIST_ADD(*pp_ace, pace);
+	/* Ensure when setting a POSIX ACL, that the uid for a
+	   SMB_ACL_USER_OBJ ACE (the owner ACE entry) has a duplicate
+	   permission entry as an SMB_ACL_USER, and a gid for a
+	   SMB_ACL_GROUP_OBJ ACE (the primary group ACE entry) also has
+	   a duplicate permission entry as an SMB_ACL_GROUP. If not,
+	   then if the ownership or group ownership of this file or
+	   directory gets changed, the user or group can lose their
+	   access. */
 
-			/* We're done here, make sure the
-			   statements below are not executed. */
+	for (pace = *pp_ace; pace; pace = pace->next) {
+		if (pace->type == SMB_ACL_USER &&
+				pace->unix_ug.id == pace_user->unix_ug.id) {
+			/* Already got one. */
 			got_duplicate_user = true;
+		} else if (pace->type == SMB_ACL_GROUP &&
+				pace->unix_ug.id == pace_user->unix_ug.id) {
+			/* Already got one. */
 			got_duplicate_group = true;
+		} else if ((pace->type == SMB_ACL_GROUP)
+			   && (dom_sid_equal(&pace->trustee, &pace_user->trustee))) {
+			/* If the SID owning the file appears
+			 * in a group entry, then we have
+			 * enough duplication, they will still
+			 * have access */
+			got_duplicate_user = true;
 		}
+	}
 
-		if (!got_duplicate_user) {
-			/* Add a duplicate SMB_ACL_USER entry. */
-			if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-				DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n"));
-				return false;
-			}
+	/* If the SID is equal for the user and group that we need
+	   to add the duplicate for, add only the group */
+	if (!got_duplicate_user && !got_duplicate_group
+			&& dom_sid_equal(&pace_group->trustee,
+					&pace_user->trustee)) {
+		/* Add a duplicate SMB_ACL_GROUP entry, this
+		 * will cover the owning SID as well, as it
+		 * will always be mapped to both a uid and
+		 * gid. */
+
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("talloc fail.\n"));
+			return false;
+		}
 
-			ZERO_STRUCTP(pace);
-			pace->type = SMB_ACL_USER;;
-			pace->owner_type = UID_ACE;
-			pace->unix_ug.type = ID_TYPE_UID;
-			pace->unix_ug.id = pace_user->unix_ug.id;
-			pace->trustee = pace_user->trustee;
-			pace->attr = pace_user->attr;
-			pace->perms = pace_user->perms;
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_GROUP;;
+		pace->owner_type = GID_ACE;
+		pace->unix_ug.type = ID_TYPE_GID;
+		pace->unix_ug.id = pace_group->unix_ug.id;
+		pace->trustee = pace_group->trustee;
+		pace->attr = pace_group->attr;
+		pace->perms = pace_group->perms;
 
-			DLIST_ADD(*pp_ace, pace);
+		DLIST_ADD(*pp_ace, pace);
 
-			got_duplicate_user = true;
+		/* We're done here, make sure the
+		   statements below are not executed. */
+		got_duplicate_user = true;
+		got_duplicate_group = true;
+	}
+
+	if (!got_duplicate_user) {
+		/* Add a duplicate SMB_ACL_USER entry. */
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("talloc fail.\n"));
+			return false;
 		}
 
-		if (!got_duplicate_group) {
-			/* Add a duplicate SMB_ACL_GROUP entry. */
-			if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
-				DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n"));
-				return false;
-			}
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_USER;;
+		pace->owner_type = UID_ACE;
+		pace->unix_ug.type = ID_TYPE_UID;
+		pace->unix_ug.id = pace_user->unix_ug.id;
+		pace->trustee = pace_user->trustee;
+		pace->attr = pace_user->attr;
+		pace->perms = pace_user->perms;
 
-			ZERO_STRUCTP(pace);
-			pace->type = SMB_ACL_GROUP;;
-			pace->owner_type = GID_ACE;
-			pace->unix_ug.type = ID_TYPE_GID;
-			pace->unix_ug.id = pace_group->unix_ug.id;
-			pace->trustee = pace_group->trustee;
-			pace->attr = pace_group->attr;
-			pace->perms = pace_group->perms;
+		DLIST_ADD(*pp_ace, pace);
 
-			DLIST_ADD(*pp_ace, pace);
+		got_duplicate_user = true;
+	}
 
-			got_duplicate_group = true;
+	if (!got_duplicate_group) {
+		/* Add a duplicate SMB_ACL_GROUP entry. */
+		if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) {
+			DEBUG(0,("talloc fail.\n"));
+			return false;
 		}
 
+		ZERO_STRUCTP(pace);
+		pace->type = SMB_ACL_GROUP;;
+		pace->owner_type = GID_ACE;
+		pace->unix_ug.type = ID_TYPE_GID;
+		pace->unix_ug.id = pace_group->unix_ug.id;
+		pace->trustee = pace_group->trustee;
+		pace->attr = pace_group->attr;
+		pace->perms = pace_group->perms;
+
+		DLIST_ADD(*pp_ace, pace);
+
+		got_duplicate_group = true;
 	}
 
-	return True;
+	return true;
 }
 
 /****************************************************************************
@@ -2149,7 +2219,7 @@ static bool create_canon_ace_lists(files_struct *fsp,
 		 * the file ACL. If we don't have them, check if any SMB_ACL_USER/SMB_ACL_GROUP


-- 
Samba Shared Repository


More information about the samba-cvs mailing list