PATCH (was: Re: An extra 'executable' bit is seen when POSIX ACL is used by Samba)

[Dmitry Butskoy]

Dmitry Butskoy wrote:

>When POSIX ACL is used, some files created by Samba appears as "have 
>extra group-executable bit". For example, "ls -l" command shows 
>"-rw-rwxr--" instead of "-rw-rw-r--" .
>
>Such a behaviour is caused by the 
>source/smbd/posix_alcs.c:chmos_acl_internals() code, the corresponding 
>code fragment is:
>
>case SMB_ACL_MASK:
>        /*
>         * FIXME: The ACL_MASK entry permissions should really be set to
>         * the union of the permissions of all ACL_USER,
>         * ACL_GROUP_OBJ, and ACL_GROUP entries. That's what
>         * acl_calc_mask() does, but Samba ACLs doesn't provide it.
>         */
>        perms = S_IRUSR|S_IWUSR|S_IXUSR;
>        break;
>
>where instead of some union computing or acl_calc_mask() call just "rwx" 
>is used.
>
>As many traditional UNIX utilities are focused on simple 
>"owner/group/other" possibilities, such utilities are confused a little 
>by this the 'extra' bit. I.e.:
>* colored "ls -l" output shows the file as an executable, Midnight 
>Commander and friends do the same;
>* the system shell tries to execute this file after typing its filename 
>and occasionally typing "enter" immediately after this;
>* potentially the file with such an extra bit can be interpreted by 
>Samba as "system" file (if "map system = yes")
>  
>
Well,

As acl_calc_mask() probably cannot be used for some reason, the actual 
mask can be calculated by Samba itself. The patch attached.

It is not tested yet, just a concept for now.
The one thing I doubt is the saving of 'mask_entry' for adding to it the 
actual mask later. It seems to be correct under Linux, but I know 
nothing about other POSIX ACL implementations.

If it looks OK, tell me and I'll test it , including some production 
environments.

Regards,
Dmitry Butskoy


-------------- next part --------------
diff -Nrbup samba-3.0.23a/source/smbd/posix_acls.c samba-3.0.23a-OK/source/smbd/posix_acls.c
--- samba-3.0.23a/source/smbd/posix_acls.c	2006-07-21 20:22:56.000000000 +0400
+++ samba-3.0.23a-OK/source/smbd/posix_acls.c	2006-09-22 17:18:14.000000000 +0400
@@ -3372,12 +3372,13 @@ int get_acl_group_bits( connection_struc
 static int chmod_acl_internals( connection_struct *conn, SMB_ACL_T posix_acl, mode_t mode)
 {
 	int entry_id = SMB_ACL_FIRST_ENTRY;
-	SMB_ACL_ENTRY_T entry;
+	SMB_ACL_ENTRY_T entry, mask_entry;
+	mode_t mask = 0;
+	SMB_ACL_PERMSET_T permset;
 	int num_entries = 0;
 
 	while ( SMB_VFS_SYS_ACL_GET_ENTRY(conn, posix_acl, entry_id, &entry) == 1) {
 		SMB_ACL_TAG_T tagtype;
-		SMB_ACL_PERMSET_T permset;
 		mode_t perms;
 
 		/* get_next... */
@@ -3398,15 +3399,16 @@ static int chmod_acl_internals( connecti
 				break;
 			case SMB_ACL_GROUP_OBJ:
 				perms = unix_perms_to_acl_perms(mode, S_IRGRP, S_IWGRP, S_IXGRP);
+				mask |= perms;
+				break;
+			case SMB_ACL_USER:
+			case SMB_ACL_GROUP:
+				mask |= convert_permset_to_mode_t(conn, permset);
 				break;
 			case SMB_ACL_MASK:
-				/*
-				 * FIXME: The ACL_MASK entry permissions should really be set to
-				 * the union of the permissions of all ACL_USER,
-				 * ACL_GROUP_OBJ, and ACL_GROUP entries. That's what
-				 * acl_calc_mask() does, but Samba ACLs doesn't provide it.
-				 */
-				perms = S_IRUSR|S_IWUSR|S_IXUSR;
+				/*  save it to set the actual mask later  */
+				mask_entry = entry;
+				continue;
 				break;
 			case SMB_ACL_OTHER:
 				perms = unix_perms_to_acl_perms(mode, S_IROTH, S_IWOTH, S_IXOTH);
@@ -3430,6 +3432,12 @@ static int chmod_acl_internals( connecti
 	if ((num_entries == 3) || (num_entries == 0))
 		return -1;
 
+	/*  set the mask calculated as ACL_MASK entry  */
+	if (map_acl_perms_to_permset(conn, mask, &permset) == -1)
+		return -1;
+	if (SMB_VFS_SYS_ACL_SET_PERMSET(conn, mask_entry, permset) == -1)
+		return -1;
+
 	return 0;
 }