[Acl-devel] Re: acl: Preserving the setuid/setgid/sticky bits

[Brandon Philips]

On 14:46 Wed 17 Jun 2009, Andreas Gruenbacher wrote:
> In the current version, getfacl only includes the new flags: comment for 
> files 
> which have any of the special set. Setfacl --restore clears all special bits 
> if there is no flags: comment, and sets them accordingly otherwise. (Without 
> --restore, setfacl disregards such comments, just like it disregards the 
> other 
> comments.)
> 
> Does this extension look reasonable?

Seems reasonable to me.

> Any objections to changing the behavior of --restore to clear the special 
> flags in case there is no flags: comment? (And if so, how would you like this 
> solved instead?)

Will need to put a block around the getfacl mode output in the
POSIXLY_CORRECT case. Other than that SUID/SGID/sticky restore if the
comment exists makes sense to me.

> @@ -467,19 +468,21 @@ do_set(
>               goto cleanup;
>       }
>       if (acl) {
> +             mode_t mode = 0;
> +             int equiv_mode;
> +
> +             equiv_mode = acl_equiv_mode(acl, &mode);
> +
>               if (acl_set_file(path_p, ACL_TYPE_ACCESS, acl) != 0) {
>                       if (errno == ENOSYS || errno == ENOTSUP) {
> -                             int saved_errno = errno;
> -                             mode_t mode;
> -
> -                             if (acl_equiv_mode(acl, &mode) != 0) {
> -                                     errno = saved_errno;
> +                             if (equiv_mode != 0)
>                                       goto fail;
> -                             } else if (chmod(path_p, mode) != 0)
> +                             else if (chmod(path_p, mode) != 0)
>                                       goto fail;
>                       } else
>                               goto fail;
>               }
> +             args->mode = mode;
>       }

These is a bit of a bug here. If the acl in do_set is the same as the
current acl then we will never reach here and mode will remain 0.

restore() from setfacl.c will OR this 0 mode with the flags from the
comment and chmod, obviously creating an incorrect result.

Here is the fix in restore()

New patch attached with this fix and a test case that will break with
the original patch applied.

+                       if (!args.mode)
+                               args.mode = st.st_mode;
+                       args.mode &= (S_IRWXU | S_IRWXG | S_IRWXO);
+                       if (chmod(path_p, flags | args.mode) != 0) {
+                               fprintf(stderr, _("%s: %s: Cannot change
"
+                                                 "mode: %s\n"),
+                                       progname, xquote(path_p,
"\n\r"),
+                                       strerror(errno));
+                               status = 1;
+                       }
+               }

Cheers,

        Brandon

0001-Include-the-S_ISUID-S_ISGID-S_ISVTX-flags-in-the-g.patch
Description: Text Data