Re: [PATCH] install: add -C option to install file only when necessary

[Jim Meyering]

Kamil Dudka <address@hidden> wrote:

> On Thursday 12 February 2009 14:27:09 Jim Meyering wrote:
>> While rewriting that,
>>
>>   install accepts a new option, --compare (-C): compare each pair of source
>>   and destination files, and if the destination has identical content and
>>   any specified owner, group, permissions, and possibly SELinux context,
>> then do not modify the destination at all.
> Thanks for review!
>
>> I realized that install must also handle the case in which
>> no explicit owner or group option is specified, yet the destination
>> owner and/or group do not match the effective ones.
>>
>> i.e., some file is installed with owner:group of WRONG_USER:WRONG_GROUP,
>> yet with proper permissions and matching content, and root runs
>> install F /ABS/NAME/OF/F
>>
>> In that case we *do* want it to unlink the original and perform the
>> copy.  Currently it would not.  This is especially important with
>> set-gid and set-uid programs.
>>
>> > +  if (!S_ISREG(src_sb.st_mode) || !S_ISREG(dest_sb.st_mode))
>> > +    return true;
>> > +
>> > +  if (src_sb.st_size != dest_sb.st_size
>> > +      || (dest_sb.st_mode & CHMOD_MODE_BITS) != mode
>> > +      || (owner_id != (uid_t) -1 && dest_sb.st_uid != owner_id)
>> > +      || (group_id != (gid_t) -1 && dest_sb.st_gid != group_id))
>> > +    return true;
>>
>> so replacing the owner/group tests with these should fix it:
>>     || dest_sb.st_uid != (owner_id == (uid_t) -1 ? geteuid () : owner_id)
>>     || dest_sb.st_gid != (group_id == (gid_t) -1 ? getegid () : group_id)
> Fixed and added new test case.
>
>> But that doesn't take account of the perhaps-unusual case
>> in which the destination directory is set-gid (on a file system
>> where that matters).
> Any idea how to solve this? Should we stat destination directory? Do we really
> need this?

I suspect that it's not worth solving at all.
stat'ing the destination directory would be a start,
but if you do that, you also have to determine file-system-
and mount-specific semantics like whether directory-set-gid bits
make a difference(nosuid), or whether all files on that partition
will always have a particular UID or GID (uid=, gid=).

>> Now that I think of security, I'd prefer that if any non-permission mode
>> bits (S_ISUID, S_ISGID, S_ISVTX) should be set, we simply short-circuit
>> the optimization and always unlink and then copy.
> Also fixed and added new test case.

Thanks!
Just a few more changes:

...
> diff --git a/NEWS b/NEWS
> index 9de4f25..4f80813 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -16,6 +16,11 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    dd accepts iflag=cio and oflag=cio to open the file in CIO (concurrent I/O)
>    mode where this feature is available.
>
> +  install accepts a new option, --compare (-C): compare each pair of source
> +  and destination files, and if the destination has identical content and
> +  any specified owner, group, permissions, and possibly SELinux context, then
> +  do not modify the destination at all.
...
> diff --git a/src/install.c b/src/install.c
> index 9bf9eee..0a102bd 100644
> --- a/src/install.c
> +++ b/src/install.c
> @@ -31,6 +31,7 @@
>  #include "cp-hash.h"
>  #include "copy.h"
>  #include "filenamecat.h"
> +#include "full-read.h"
>  #include "mkancesdirs.h"
>  #include "mkdir-p.h"
>  #include "modechange.h"
> @@ -111,6 +112,8 @@ static char *group_name;
>  static gid_t group_id;
>
>  #define DEFAULT_MODE (S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)
> +#define EXTRA_MODE(mode) ((S_ISUID & (mode)) || (S_ISGID & (mode)) \
> +    || (S_ISVTX & (mode)))

Please write this as:

static bool
extra_mode (mode_t mode)
{
  return mode & ~S_IRWXUGO;
}

...
> +  if (copy_only_if_needed && EXTRA_MODE (mode))
> +    error (0, 0, _("options -C is ignored if any non-permission mode should "
> +                "be set"));

Please use this wording:

    error (0, 0, _("the --compare (-C) option is ignored when you"
                   " specify a mode with non-permission bits"));

>    get_ids ();
>
>    if (dir_arg)
> @@ -645,6 +766,9 @@ copy_file (const char *from, const char *to, const struct 
> cp_options *x)
>  {
>    bool copy_into_self;
>
> +  if (copy_only_if_needed && !need_copy (from, to, x))
> +    return true;
> +
>    /* Allow installing from non-regular files like /dev/null.
>       Charles Karney reported that some Sun version of install allows that
>       and that sendmail's installation process relies on the behavior.
> @@ -835,6 +959,9 @@ Mandatory arguments to long options are mandatory for 
> short options too.\n\
>        --backup[=CONTROL]  make a backup of each existing destination file\n\
>    -b                  like --backup but does not accept an argument\n\
>    -c                  (ignored)\n\
> +  -C, --compare       install file, unless target already exists and is\n\
> +                        the same as the new file, in which case the\n\
> +                        modification time won't be changed\n\

At first I wanted to say something like in NEWS, but how about this
instead, which gives the flavor, but defers to texinfo documentation
for the full details:

   -C, --compare       compare each pair of source and destination files, and\n\
                         and in some cases, do not modify the destination at 
all\n\

Also, I agree about keeping stack usage low,
so have a slight preference for making those two buffers static.

+#define CMP_BLOCK_SIZE 65536

enum { CMP_BLOCK_SIZE = 65536 };

In general, prefer an enum; it is then usable in the debugger.

+  char a_buff[CMP_BLOCK_SIZE];
+  char b_buff[CMP_BLOCK_SIZE];
+
+  size_t size;
+  while (0 < (size = full_read (a_fd, a_buff, CMP_BLOCK_SIZE))) {
+    if (size != full_read (b_fd, b_buff, CMP_BLOCK_SIZE))

Please use "sizeof a_buff" and "sizeof b_buff" in the while-loop,
rather than CMP_BLOCK_SIZE.

+      return false;
+
+    if (memcmp (a_buff, b_buff, size) != 0)
+      return false;
+  }
+
+  return size == 0;
+#undef CMP_BLOCK_SIZE

Finally, please remove this #undef.
Not only will it no longer be needed,
but #undefs are rarely worth the trouble in .c files.