patch 8.0.1300: file permissions may end up wrong when writing

Problem:    File permissions may end up wrong when writing.
Solution:   Use fchmod() instead of chmod() when possible.  Don't truncate
            until we know we can change the file.

Author: brammool

src/auto/configure

@@ -12090,13 +12090,13 @@ if test "x$vim_cv_getcwd_broken" = "xyes" ; then
 
 fi
 
-for ac_func in fchdir fchown fsync getcwd getpseudotty \
+for ac_func in fchdir fchown fchmod fsync getcwd getpseudotty \
 	getpwent getpwnam getpwuid getrlimit gettimeofday getwd lstat \
 	memset mkdtemp nanosleep opendir putenv qsort readlink select setenv \
 	getpgid setpgid setsid sigaltstack sigstack sigset sigsetjmp sigaction \
 	sigprocmask sigvec strcasecmp strerror strftime stricmp strncasecmp \
 	strnicmp strpbrk strtol tgetent towlower towupper iswupper \
-	usleep utime utimes mblen
+	usleep utime utimes mblen ftruncate
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"

src/config.h.in

@@ -156,9 +156,11 @@
 /* Define if you the function: */
 #undef HAVE_FCHDIR
 #undef HAVE_FCHOWN
+#undef HAVE_FCHMOD
+#undef HAVE_FLOAT_FUNCS
 #undef HAVE_FSEEKO
 #undef HAVE_FSYNC
-#undef HAVE_FLOAT_FUNCS
+#undef HAVE_FTRUNCATE
 #undef HAVE_GETCWD
 #undef HAVE_GETPGID
 #undef HAVE_GETPSEUDOTTY

src/configure.ac

@@ -3655,13 +3655,13 @@ fi
 
 dnl Check for functions in one big call, to reduce the size of configure.
 dnl Can only be used for functions that do not require any include.
-AC_CHECK_FUNCS(fchdir fchown fsync getcwd getpseudotty \
+AC_CHECK_FUNCS(fchdir fchown fchmod fsync getcwd getpseudotty \
 	getpwent getpwnam getpwuid getrlimit gettimeofday getwd lstat \
 	memset mkdtemp nanosleep opendir putenv qsort readlink select setenv \
 	getpgid setpgid setsid sigaltstack sigstack sigset sigsetjmp sigaction \
 	sigprocmask sigvec strcasecmp strerror strftime stricmp strncasecmp \
 	strnicmp strpbrk strtol tgetent towlower towupper iswupper \
-	usleep utime utimes mblen)
+	usleep utime utimes mblen ftruncate)
 AC_FUNC_FSEEKO
 
 dnl define _LARGE_FILES, _FILE_OFFSET_BITS and _LARGEFILE_SOURCE when

src/fileio.c

@@ -3863,6 +3863,7 @@ buf_write(
 	    char_u	*rootname;
 #if defined(UNIX)
 	    int		did_set_shortname;
+	    mode_t	umask_save;
 #endif
 
 	    copybuf = alloc(BUFSIZE + 1);
@@ -3994,22 +3995,30 @@ buf_write(
 		    /* remove old backup, if present */
 		    mch_remove(backup);
 		    /* Open with O_EXCL to avoid the file being created while
-		     * we were sleeping (symlink hacker attack?) */
+		     * we were sleeping (symlink hacker attack?). Reset umask
+		     * if possible to avoid mch_setperm() below. */
+#ifdef UNIX
+		    umask_save = umask(0);
+#endif
 		    bfd = mch_open((char *)backup,
 				O_WRONLY|O_CREAT|O_EXTRA|O_EXCL|O_NOFOLLOW,
 								 perm & 0777);
+#ifdef UNIX
+		    (void)umask(umask_save);
+#endif
 		    if (bfd < 0)
 		    {
 			vim_free(backup);
 			backup = NULL;
 		    }
 		    else
 		    {
-			/* set file protection same as original file, but
-			 * strip s-bit */
+			/* Set file protection same as original file, but
+			 * strip s-bit.  Only needed if umask() wasn't used
+			 * above. */
+#ifndef UNIX
 			(void)mch_setperm(backup, perm & 0777);
-
-#ifdef UNIX
+#else
 			/*
 			 * Try to set the group of the backup same as the
 			 * original file. If this fails, set the protection
@@ -4377,6 +4386,11 @@ buf_write(
 	}
 	else
 	{
+#ifdef HAVE_FTRUNCATE
+# define TRUNC_ON_OPEN 0
+#else
+# define TRUNC_ON_OPEN O_TRUNC
+#endif
 	    /*
 	     * Open the file "wfname" for writing.
 	     * We may try to open the file twice: If we can't write to the file
@@ -4389,7 +4403,7 @@ buf_write(
 	     */
 	    while ((fd = mch_open((char *)wfname, O_WRONLY | O_EXTRA | (append
 				? (forceit ? (O_APPEND | O_CREAT) : O_APPEND)
-				: (O_CREAT | O_TRUNC))
+				: (O_CREAT | TRUNC_ON_OPEN))
 				, perm < 0 ? 0666 : (perm & 0777))) < 0)
 	    {
 		/*
@@ -4482,6 +4496,30 @@ buf_write(
 	    }
 	    write_info.bw_fd = fd;
 
+#if defined(UNIX)
+	    {
+		stat_T	st;
+
+		/* Double check we are writing the intended file before making
+		 * any changes. */
+		if (overwriting
+			&& (!dobackup || backup_copy)
+			&& fname == wfname
+			&& perm >= 0
+			&& mch_fstat(fd, &st) == 0
+			&& st.st_ino != st_old.st_ino)
+		{
+		    close(fd);
+		    errmsg = (char_u *)_("E949: File changed while writing");
+		    goto fail;
+		}
+	    }
+#endif
+#ifdef HAVE_FTRUNCATE
+	    if (!append)
+		ftruncate(fd, (off_t)0);
+#endif
+
 #if defined(WIN3264)
 	    if (backup != NULL && overwriting && !append)
 	    {
@@ -4752,15 +4790,17 @@ buf_write(
 # ifdef HAVE_FCHOWN
 	    stat_T	st;
 
-	    /* don't change the owner when it's already OK, some systems remove
-	     * permission or ACL stuff */
+	    /* Don't change the owner when it's already OK, some systems remove
+	     * permission or ACL stuff. */
 	    if (mch_stat((char *)wfname, &st) < 0
 		    || st.st_uid != st_old.st_uid
 		    || st.st_gid != st_old.st_gid)
 	    {
-		ignored = fchown(fd, st_old.st_uid, st_old.st_gid);
-		if (perm >= 0)	/* set permission again, may have changed */
-		    (void)mch_setperm(wfname, perm);
+		/* changing owner might not be possible */
+		ignored = fchown(fd, st_old.st_uid, -1);
+		/* if changing group fails clear the group permissions */
+		if (fchown(fd, -1, st_old.st_gid) == -1 && perm > 0)
+		    perm &= ~070;
 	    }
 # endif
 	    buf_setino(buf);
@@ -4770,18 +4810,26 @@ buf_write(
 	    buf_setino(buf);
 #endif
 
+#ifdef UNIX
+	if (made_writable)
+	    perm &= ~0200;	/* reset 'w' bit for security reasons */
+#endif
+#ifdef HAVE_FCHMOD
+	/* set permission of new file same as old file */
+	if (perm >= 0)
+	    (void)mch_fsetperm(fd, perm);
+#endif
 	if (close(fd) != 0)
 	{
 	    errmsg = (char_u *)_("E512: Close failed");
 	    end = 0;
 	}
 
-#ifdef UNIX
-	if (made_writable)
-	    perm &= ~0200;	/* reset 'w' bit for security reasons */
-#endif
-	if (perm >= 0)		/* set perm. of new file same as old file */
+#ifndef HAVE_FCHMOD
+	/* set permission of new file same as old file */
+	if (perm >= 0)
 	    (void)mch_setperm(wfname, perm);
+#endif
 #ifdef HAVE_ACL
 	/*
 	 * Probably need to set the ACL before changing the user (can't set the

src/os_unix.c

@@ -2725,9 +2725,8 @@ mch_getperm(char_u *name)
 }
 
 /*
- * set file permission for 'name' to 'perm'
- *
- * return FAIL for failure, OK otherwise
+ * Set file permission for "name" to "perm".
+ * Return FAIL for failure, OK otherwise.
  */
     int
 mch_setperm(char_u *name, long perm)
@@ -2741,6 +2740,18 @@ mch_setperm(char_u *name, long perm)
 		    (mode_t)perm) == 0 ? OK : FAIL);
 }
 
+#if defined(HAVE_FCHMOD) || defined(PROTO)
+/*
+ * Set file permission for open file "fd" to "perm".
+ * Return FAIL for failure, OK otherwise.
+ */
+    int
+mch_fsetperm(int fd, long perm)
+{
+    return (fchmod(fd, (mode_t)perm) == 0 ? OK : FAIL);
+}
+#endif
+
 #if defined(HAVE_ACL) || defined(PROTO)
 # ifdef HAVE_SYS_ACL_H
 #  include <sys/acl.h>

src/proto/os_unix.pro

@@ -36,6 +36,7 @@ int mch_isFullName(char_u *fname);
 void fname_case(char_u *name, int len);
 long mch_getperm(char_u *name);
 int mch_setperm(char_u *name, long perm);
+int mch_fsetperm(int fd, long perm);
 void mch_copy_sec(char_u *from_file, char_u *to_file);
 vim_acl_T mch_get_acl(char_u *fname);
 void mch_set_acl(char_u *fname, vim_acl_T aclent);

src/version.c

@@ -766,6 +766,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1300,
 /**/
     1299,
 /**/