patch 8.0.0685: when conversion fails written file may be truncated

Problem:    When making backups is disabled and conversion with iconv fails
            the written file is truncated. (Luo Chen)
Solution:   First try converting the file and write the file only when it did
            not fail. (partly by Christian Brabandt)

src/fileio.c

@@ -3166,6 +3166,7 @@ buf_write(
     int		    device = FALSE;	    /* writing to a device */
     stat_T	    st_old;
     int		    prev_got_int = got_int;
+    int		    checking_conversion;
     int		    file_readonly = FALSE;  /* overwritten file is read-only */
     static char	    *err_readonly = "is read-only (cannot override: \"W\" in 'cpoptions')";
 #if defined(UNIX)			    /*XXX fix me sometime? */
@@ -4343,13 +4344,41 @@ buf_write(
     }
 #endif
 
+    /*
+     * If conversion is taking place, we may first pretend to write and check
+     * for conversion errors.  Then loop again to write for real.
+     * When not doing conversion this writes for real right away.
+     */
+    for (checking_conversion = TRUE; ; checking_conversion = FALSE)
+    {
+	/*
+	 * There is no need to check conversion when:
+	 * - there is no conversion
+	 * - we make a backup file, that can be restored in case of conversion
+	 *   failure.
+	 */
+#ifdef FEAT_MBYTE
+	if (!converted || dobackup)
+#endif
+	    checking_conversion = FALSE;
+
+	if (checking_conversion)
+	{
+	    /* Make sure we don't write anything. */
+	    fd = -1;
+	    write_info.bw_fd = fd;
+	}
+	else
+	{
 	    /*
 	     * Open the file "wfname" for writing.
-     * We may try to open the file twice: If we can't write to the
+	     * We may try to open the file twice: If we can't write to the file
-     * file and forceit is TRUE we delete the existing file and try to create
+	     * and forceit is TRUE we delete the existing file and try to
-     * a new one. If this still fails we may have lost the original file!
+	     * create a new one. If this still fails we may have lost the
-     * (this may happen when the user reached his quotum for number of files).
+	     * original file!  (this may happen when the user reached his
-     * Appending will fail if the file does not exist and forceit is FALSE.
+	     * quotum for number of files).
+	     * Appending will fail if the file does not exist and forceit is
+	     * FALSE.
 	     */
 	    while ((fd = mch_open((char *)wfname, O_WRONLY | O_EXTRA | (append
 				? (forceit ? (O_APPEND | O_CREAT) : O_APPEND)
@@ -4357,16 +4386,17 @@ buf_write(
 				, perm < 0 ? 0666 : (perm & 0777))) < 0)
 	    {
 		/*
-	 * A forced write will try to create a new file if the old one is
+		 * A forced write will try to create a new file if the old one
-	 * still readonly. This may also happen when the directory is
+		 * is still readonly. This may also happen when the directory
-	 * read-only. In that case the mch_remove() will fail.
+		 * is read-only. In that case the mch_remove() will fail.
 		 */
 		if (errmsg == NULL)
 		{
 #ifdef UNIX
 		    stat_T	st;
 
-	    /* Don't delete the file when it's a hard or symbolic link. */
+		    /* Don't delete the file when it's a hard or symbolic link.
+		     */
 		    if ((!newfile && st_old.st_nlink > 1)
 			    || (mch_lstat((char *)fname, &st) == 0
 				&& (st.st_dev != st_old.st_dev
@@ -4385,7 +4415,8 @@ buf_write(
 			    if (!(perm & 0200))
 				made_writable = TRUE;
 			    perm |= 0200;
-		    if (st_old.st_uid != getuid() || st_old.st_gid != getgid())
+			    if (st_old.st_uid != getuid()
+						  || st_old.st_gid != getgid())
 				perm &= 0777;
 #endif
 			    if (!append)  /* don't remove when appending */
@@ -4400,24 +4431,26 @@ restore_backup:
 		    stat_T	st;
 
 		    /*
-	     * If we failed to open the file, we don't need a backup. Throw it
+		     * If we failed to open the file, we don't need a backup.
-	     * away.  If we moved or removed the original file try to put the
+		     * Throw it away.  If we moved or removed the original file
-	     * backup in its place.
+		     * try to put the backup in its place.
 		     */
 		    if (backup != NULL && wfname == fname)
 		    {
 			if (backup_copy)
 			{
 			    /*
-		     * There is a small chance that we removed the original,
+			     * There is a small chance that we removed the
-		     * try to move the copy in its place.
+			     * original, try to move the copy in its place.
 			     * This may not work if the vim_rename() fails.
 			     * In that case we leave the copy around.
 			     */
-		    /* If file does not exist, put the copy in its place */
+			    /* If file does not exist, put the copy in its
+			     * place */
 			    if (mch_stat((char *)fname, &st) < 0)
 				vim_rename(backup, fname);
-		    /* if original file does exist throw away the copy */
+			    /* if original file does exist throw away the copy
+			     */
 			    if (mch_stat((char *)fname, &st) >= 0)
 				mch_remove(backup);
 			}
@@ -4428,7 +4461,8 @@ restore_backup:
 			}
 		    }
 
-	    /* if original file no longer exists give an extra warning */
+		    /* if original file no longer exists give an extra warning
+		     */
 		    if (!newfile && mch_stat((char *)fname, &st) < 0)
 			end = 0;
 		}
@@ -4439,7 +4473,7 @@ restore_backup:
 #endif
 		goto fail;
 	    }
-    errmsg = NULL;
+	    write_info.bw_fd = fd;
 
 #if defined(MACOS_CLASSIC) || defined(WIN3264)
 	    /* TODO: Is it need for MACOS_X? (Dany) */
@@ -4464,15 +4498,14 @@ restore_backup:
 	    }
 #endif
 
-    write_info.bw_fd = fd;
-
 #ifdef FEAT_CRYPT
 	    if (*buf->b_p_key != NUL && !filtering)
 	    {
 		char_u		*header;
 		int		header_len;
 
-	buf->b_cryptstate = crypt_create_for_writing(crypt_get_method_nr(buf),
+		buf->b_cryptstate = crypt_create_for_writing(
+						      crypt_get_method_nr(buf),
 					   buf->b_p_key, &header, &header_len);
 		if (buf->b_cryptstate == NULL || header == NULL)
 		    end = 0;
@@ -4490,6 +4523,8 @@ restore_backup:
 		}
 	    }
 #endif
+	}
+	errmsg = NULL;
 
 	write_info.bw_buf = buffer;
 	nchars = 0;
@@ -4503,8 +4538,8 @@ restore_backup:
 #ifdef FEAT_MBYTE
 	/*
 	 * The BOM is written just after the encryption magic number.
-     * Skip it when appending and the file already existed, the BOM only makes
+	 * Skip it when appending and the file already existed, the BOM only
-     * sense at the start of the file.
+	 * makes sense at the start of the file.
 	 */
 	if (buf->b_p_bomb && !write_bin && (!append || perm < 0))
 	{
@@ -4523,8 +4558,12 @@ restore_backup:
 #endif
 
 #ifdef FEAT_PERSISTENT_UNDO
-    write_undo_file = (buf->b_p_udf && overwriting && !append
+	write_undo_file = (buf->b_p_udf
-					      && !filtering && reset_changed);
+			    && overwriting
+			    && !append
+			    && !filtering
+			    && reset_changed
+			    && !checking_conversion);
 	if (write_undo_file)
 	    /* Prepare for computing the hash value of the text. */
 	    sha256_start(&sha_ctx);
@@ -4546,7 +4585,8 @@ restore_backup:
 	    ptr = ml_get_buf(buf, lnum, FALSE) - 1;
 #ifdef FEAT_PERSISTENT_UNDO
 	    if (write_undo_file)
-	    sha256_update(&sha_ctx, ptr + 1, (UINT32_T)(STRLEN(ptr + 1) + 1));
+		sha256_update(&sha_ctx, ptr + 1,
+					      (UINT32_T)(STRLEN(ptr + 1) + 1));
 #endif
 	    while ((c = *++ptr) != NUL)
 	    {
@@ -4576,7 +4616,8 @@ restore_backup:
 		    || (lnum == end
 			&& (write_bin || !buf->b_p_fixeol)
 			&& (lnum == buf->b_no_eol_lnum
-			|| (lnum == buf->b_ml.ml_line_count && !buf->b_p_eol))))
+			    || (lnum == buf->b_ml.ml_line_count
+							   && !buf->b_p_eol))))
 	    {
 		++lnum;			/* written the line, count it */
 		no_eol = TRUE;
@@ -4623,12 +4664,12 @@ restore_backup:
 	    }
 #ifdef VMS
 	    /*
-	 * On VMS there is a problem: newlines get added when writing blocks
+	     * On VMS there is a problem: newlines get added when writing
-	 * at a time. Fix it by writing a line at a time.
+	     * blocks at a time. Fix it by writing a line at a time.
 	     * This is much slower!
 	     * Explanation: VAX/DECC RTL insists that records in some RMS
-	 * structures end with a newline (carriage return) character, and if
+	     * structures end with a newline (carriage return) character, and
-	 * they don't it adds one.
+	     * if they don't it adds one.
 	     * With other RMS structures it works perfect without this fix.
 	     */
 	    if (buf->b_fab_rfm == FAB$C_VFC
@@ -4666,15 +4707,29 @@ restore_backup:
 	    nchars += len;
 	}
 
+	/* Stop when writing done or an error was encountered. */
+	if (!checking_conversion || end == 0)
+	    break;
+
+	/* If no error happened until now, writing should be ok, so loop to
+	 * really write the buffer. */
+    }
+
+    /* If we started writing, finish writing. Also when an error was
+     * encountered. */
+    if (!checking_conversion)
+    {
 #if defined(UNIX) && defined(HAVE_FSYNC)
-    /* On many journalling file systems there is a bug that causes both the
+	/*
-     * original and the backup file to be lost when halting the system right
+	 * On many journalling file systems there is a bug that causes both the
-     * after writing the file.  That's because only the meta-data is
+	 * original and the backup file to be lost when halting the system
-     * journalled.  Syncing the file slows down the system, but assures it has
+	 * right after writing the file.  That's because only the meta-data is
-     * been written to disk and we don't lose it.
+	 * journalled.  Syncing the file slows down the system, but assures it
-     * For a device do try the fsync() but don't complain if it does not work
+	 * has been written to disk and we don't lose it.
-     * (could be a pipe).
+	 * For a device do try the fsync() but don't complain if it does not
-     * If the 'fsync' option is FALSE, don't fsync().  Useful for laptops. */
+	 * work (could be a pipe).
+	 * If the 'fsync' option is FALSE, don't fsync().  Useful for laptops.
+	 */
 	if (p_fs && fsync(fd) != 0 && !device)
 	{
 	    errmsg = (char_u *)_("E667: Fsync failed");
@@ -4689,8 +4744,8 @@ restore_backup:
 #endif
 
 #ifdef UNIX
-    /* When creating a new file, set its owner/group to that of the original
+	/* When creating a new file, set its owner/group to that of the
-     * file.  Get the new device and inode number. */
+	 * original file.  Get the new device and inode number. */
 	if (backup != NULL && !backup_copy)
 	{
 # ifdef HAVE_FCHOWN
@@ -4731,9 +4786,9 @@ restore_backup:
 	 * Probably need to set the ACL before changing the user (can't set the
 	 * ACL on a file the user doesn't own).
 	 * On Solaris, with ZFS and the aclmode property set to "discard" (the
-     * default), chmod() discards all part of a file's ACL that don't represent
+	 * default), chmod() discards all part of a file's ACL that don't
-     * the mode of the file.  It's non-trivial for us to discover whether we're
+	 * represent the mode of the file.  It's non-trivial for us to discover
-     * in that situation, so we simply always re-set the ACL.
+	 * whether we're in that situation, so we simply always re-set the ACL.
 	 */
 # ifndef HAVE_SOLARIS_ZFS_ACL
 	if (!backup_copy)
@@ -4752,13 +4807,13 @@ restore_backup:
 	if (wfname != fname)
 	{
 	    /*
-	 * The file was written to a temp file, now it needs to be converted
+	     * The file was written to a temp file, now it needs to be
-	 * with 'charconvert' to (overwrite) the output file.
+	     * converted with 'charconvert' to (overwrite) the output file.
 	     */
 	    if (end != 0)
 	    {
-	    if (eval_charconvert(enc_utf8 ? (char_u *)"utf-8" : p_enc, fenc,
+		if (eval_charconvert(enc_utf8 ? (char_u *)"utf-8" : p_enc,
-						       wfname, fname) == FAIL)
+						  fenc, wfname, fname) == FAIL)
 		{
 		    write_info.bw_conv_error = TRUE;
 		    end = 0;
@@ -4768,9 +4823,13 @@ restore_backup:
 	    vim_free(wfname);
 	}
 #endif
+    }
 
     if (end == 0)
     {
+	/*
+	 * Error encountered.
+	 */
 	if (errmsg == NULL)
 	{
 #ifdef FEAT_MBYTE
@@ -5690,6 +5749,10 @@ buf_write_bytes(struct bw_info *ip)
     }
 #endif /* FEAT_MBYTE */
 
+    if (ip->bw_fd < 0)
+	/* Only checking conversion, which is OK if we get here. */
+	return OK;
+
 #ifdef FEAT_CRYPT
     if (flags & FIO_ENCRYPTED)
     {

src/testdir/test_writefile.vim

@@ -31,3 +31,21 @@ func Test_writefile_fails_gently()
 
   call assert_fails('call writefile([], [])', 'E730:')
 endfunc
+
+func Test_writefile_fails_conversion()
+  if !has('multi_byte') || !has('iconv')
+    return
+  endif
+  set nobackup nowritebackup
+  new
+  let contents = ["line one", "line two"]
+  call writefile(contents, 'Xfile')
+  edit Xfile
+  call setline(1, ["first line", "cannot convert \u010b", "third line"])
+  call assert_fails('write ++enc=cp932')
+  call assert_equal(contents, readfile('Xfile'))
+
+  call delete('Xfile')
+  bwipe!
+  set backup& writebackup&
+endfunc

src/version.c

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