bin/112336: install -S (safe copy) with -C or -p is not so safe

[John E Hein]

The following reply was made to PR bin/112336; it has been noted by GNATS.

From: John E Hein <jhein at timing.com>
To: FreeBSD-gnats-submit at FreeBSD.org, freebsd-bugs at FreeBSD.org
Cc:  
Subject: Re: bin/112336: install -S (safe copy) with -C or -p is not so safe
Date: Mon, 7 May 2007 15:13:24 -0600

 Here is a patch that implements 2 and was simpler than I thought it
 would be.
 
 It creates the temp file and does a rename, not only if the contents
 of the "from" & "to" files don't match, but also if perms/ownership
 don't match.
 
 One optimization could be to try to do the chown/chmod first and if
 that succceeds and the files match contents, don't bother with the
 temp file.  But that seems to be needlessly complex for the relatively
 miniscule number of cases where we could gain from the added pedantry.
 And there may be edge case failures associated with an early
 chmod/chown.
 
 Any committers willing to take this one?
 
 
 Index: src/usr.bin/xinstall/xinstall.c
 ===================================================================
 RCS file: /base/FreeBSD-CVS/src/usr.bin/xinstall/xinstall.c,v
 retrieving revision 1.67
 diff -u -p -r1.67 xinstall.c
 --- src/usr.bin/xinstall/xinstall.c	6 Mar 2006 21:52:59 -0000	1.67
 +++ src/usr.bin/xinstall/xinstall.c	7 May 2007 20:52:18 -0000
 @@ -317,7 +317,19 @@ install(const char *from_name, const cha
  	if (docompare && !dostrip && target) {
  		if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
  			err(EX_OSERR, "%s", to_name);
 -		if (devnull)
 +		/*
 +		 * Even if the contents are the same, we want to rename
 +		 * temp files when doing a "safe" copy if the
 +		 * permissions and ownership need to change.  We may
 +		 * not have permission to chown/chmod the "to" file
 +		 * directly.
 +		 */
 +		if (tempcopy &&
 +		    ((gid != (gid_t)-1 && gid != to_sb.st_gid) ||
 +		    (uid != (uid_t)-1 && uid != to_sb.st_uid) ||
 +		    (mode != (to_sb.st_mode & ALLPERMS))))
 +		    files_match = 0;
 +		else if (devnull)
  			files_match = to_sb.st_size == 0;
  		else
  			files_match = !(compare(from_fd, from_name,