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)

3 files changed, 415 insertions, 332 deletions

diff --git a/src/fileio.c b/src/fileio.c
index b41df3e7b..0f59d809d 100644
--- a/src/fileio.c
+++ b/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? */
@@ -4344,433 +4345,491 @@ buf_write(
 #endif
 
     /*
-     * Open the file "wfname" for writing.
-     * We may try to open the file twice: If we can't write to the
-     * file and forceit is TRUE we delete the existing file and try to create
-     * a new one. If this still fails we may have lost the original file!
-     * (this may happen when the user reached his quotum for number of files).
-     * Appending will fail if the file does not exist and forceit is FALSE.
+     * 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.
      */
-    while ((fd = mch_open((char *)wfname, O_WRONLY | O_EXTRA | (append
-			? (forceit ? (O_APPEND | O_CREAT) : O_APPEND)
-			: (O_CREAT | O_TRUNC))
-			, perm < 0 ? 0666 : (perm & 0777))) < 0)
+    for (checking_conversion = TRUE; ; checking_conversion = FALSE)
     {
 	/*
-	 * A forced write will try to create a new file if the old one is
-	 * still readonly. This may also happen when the directory is
-	 * read-only. In that case the mch_remove() will fail.
+	 * 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.
 	 */
-	if (errmsg == NULL)
+#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 file
+	     * and forceit is TRUE we delete the existing file and try to
+	     * create a new one. If this still fails we may have lost the
+	     * original file!  (this may happen when the user reached his
+	     * 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)
+				: (O_CREAT | O_TRUNC))
+				, perm < 0 ? 0666 : (perm & 0777))) < 0)
+	    {
+		/*
+		 * A forced write will try to create a new file if the old one
+		 * is still readonly. This may also happen when the directory
+		 * is read-only. In that case the mch_remove() will fail.
+		 */
+		if (errmsg == NULL)
+		{
 #ifdef UNIX
-	    stat_T	st;
+		    stat_T	st;
 
-	    /* 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
-			    || st.st_ino != st_old.st_ino)))
-		errmsg = (char_u *)_("E166: Can't open linked file for writing");
-	    else
+		    /* 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
+				    || st.st_ino != st_old.st_ino)))
+			errmsg = (char_u *)_("E166: Can't open linked file for writing");
+		    else
 #endif
-	    {
-		errmsg = (char_u *)_("E212: Can't open file for writing");
-		if (forceit && vim_strchr(p_cpo, CPO_FWRITE) == NULL
-								 && perm >= 0)
-		{
+		    {
+			errmsg = (char_u *)_("E212: Can't open file for writing");
+			if (forceit && vim_strchr(p_cpo, CPO_FWRITE) == NULL
+								  && perm >= 0)
+			{
 #ifdef UNIX
-		    /* we write to the file, thus it should be marked
-		       writable after all */
-		    if (!(perm & 0200))
-			made_writable = TRUE;
-		    perm |= 0200;
-		    if (st_old.st_uid != getuid() || st_old.st_gid != getgid())
-			perm &= 0777;
-#endif
-		    if (!append)	    /* don't remove when appending */
-			mch_remove(wfname);
-		    continue;
+			    /* we write to the file, thus it should be marked
+			       writable after all */
+			    if (!(perm & 0200))
+				made_writable = TRUE;
+			    perm |= 0200;
+			    if (st_old.st_uid != getuid()
+						  || st_old.st_gid != getgid())
+				perm &= 0777;
+#endif
+			    if (!append)  /* don't remove when appending */
+				mch_remove(wfname);
+			    continue;
+			}
+		    }
 		}
-	    }
-	}
 
 restore_backup:
-	{
-	    stat_T	st;
-
-	    /*
-	     * If we failed to open the file, we don't need a backup. Throw it
-	     * away.  If we moved or removed the original file try to put the
-	     * backup in its place.
-	     */
-	    if (backup != NULL && wfname == fname)
-	    {
-		if (backup_copy)
 		{
+		    stat_T	st;
+
 		    /*
-		     * There is a small chance that we removed the 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 we failed to open the file, we don't need a backup.
+		     * Throw it away.  If we moved or removed the original file
+		     * try to put the backup 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 (mch_stat((char *)fname, &st) >= 0)
-			mch_remove(backup);
-		}
-		else
-		{
-		    /* try to put the original file back */
-		    vim_rename(backup, fname);
-		}
-	    }
+		    if (backup != NULL && wfname == fname)
+		    {
+			if (backup_copy)
+			{
+			    /*
+			     * There is a small chance that we removed the
+			     * 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 (mch_stat((char *)fname, &st) < 0)
+				vim_rename(backup, fname);
+			    /* if original file does exist throw away the copy
+			     */
+			    if (mch_stat((char *)fname, &st) >= 0)
+				mch_remove(backup);
+			}
+			else
+			{
+			    /* try to put the original file back */
+			    vim_rename(backup, fname);
+			}
+		    }
 
-	    /* if original file no longer exists give an extra warning */
-	    if (!newfile && mch_stat((char *)fname, &st) < 0)
-		end = 0;
-	}
+		    /* if original file no longer exists give an extra warning
+		     */
+		    if (!newfile && mch_stat((char *)fname, &st) < 0)
+			end = 0;
+		}
 
 #ifdef FEAT_MBYTE
-	if (wfname != fname)
-	    vim_free(wfname);
+		if (wfname != fname)
+		    vim_free(wfname);
 #endif
-	goto fail;
-    }
-    errmsg = NULL;
+		goto fail;
+	    }
+	    write_info.bw_fd = fd;
 
 #if defined(MACOS_CLASSIC) || defined(WIN3264)
-    /* TODO: Is it need for MACOS_X? (Dany) */
-    /*
-     * On macintosh copy the original files attributes (i.e. the backup)
-     * This is done in order to preserve the resource fork and the
-     * Finder attribute (label, comments, custom icons, file creator)
-     */
-    if (backup != NULL && overwriting && !append)
-    {
-	if (backup_copy)
-	    (void)mch_copy_file_attribute(wfname, backup);
-	else
-	    (void)mch_copy_file_attribute(backup, wfname);
-    }
+	    /* TODO: Is it need for MACOS_X? (Dany) */
+	    /*
+	     * On macintosh copy the original files attributes (i.e. the backup)
+	     * This is done in order to preserve the resource fork and the
+	     * Finder attribute (label, comments, custom icons, file creator)
+	     */
+	    if (backup != NULL && overwriting && !append)
+	    {
+		if (backup_copy)
+		    (void)mch_copy_file_attribute(wfname, backup);
+		else
+		    (void)mch_copy_file_attribute(backup, wfname);
+	    }
 
-    if (!overwriting && !append)
-    {
-	if (buf->b_ffname != NULL)
-	    (void)mch_copy_file_attribute(buf->b_ffname, wfname);
-	/* Should copy resource fork */
-    }
+	    if (!overwriting && !append)
+	    {
+		if (buf->b_ffname != NULL)
+		    (void)mch_copy_file_attribute(buf->b_ffname, wfname);
+		/* Should copy resource fork */
+	    }
 #endif
 
-    write_info.bw_fd = fd;
-
 #ifdef FEAT_CRYPT
-    if (*buf->b_p_key != NUL && !filtering)
-    {
-	char_u		*header;
-	int		header_len;
+	    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_p_key, &header, &header_len);
-	if (buf->b_cryptstate == NULL || header == NULL)
-	    end = 0;
-	else
-	{
-	    /* Write magic number, so that Vim knows how this file is
-	     * encrypted when reading it back. */
-	    write_info.bw_buf = header;
-	    write_info.bw_len = header_len;
-	    write_info.bw_flags = FIO_NOCONVERT;
-	    if (buf_write_bytes(&write_info) == FAIL)
-		end = 0;
-	    wb_flags |= FIO_ENCRYPTED;
-	    vim_free(header);
-	}
-    }
+		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;
+		else
+		{
+		    /* Write magic number, so that Vim knows how this file is
+		     * encrypted when reading it back. */
+		    write_info.bw_buf = header;
+		    write_info.bw_len = header_len;
+		    write_info.bw_flags = FIO_NOCONVERT;
+		    if (buf_write_bytes(&write_info) == FAIL)
+			end = 0;
+		    wb_flags |= FIO_ENCRYPTED;
+		    vim_free(header);
+		}
+	    }
 #endif
+	}
+	errmsg = NULL;
 
-    write_info.bw_buf = buffer;
-    nchars = 0;
+	write_info.bw_buf = buffer;
+	nchars = 0;
 
-    /* use "++bin", "++nobin" or 'binary' */
-    if (eap != NULL && eap->force_bin != 0)
-	write_bin = (eap->force_bin == FORCE_BIN);
-    else
-	write_bin = buf->b_p_bin;
+	/* use "++bin", "++nobin" or 'binary' */
+	if (eap != NULL && eap->force_bin != 0)
+	    write_bin = (eap->force_bin == FORCE_BIN);
+	else
+	    write_bin = buf->b_p_bin;
 
 #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
-     * sense at the start of the file.
-     */
-    if (buf->b_p_bomb && !write_bin && (!append || perm < 0))
-    {
-	write_info.bw_len = make_bom(buffer, fenc);
-	if (write_info.bw_len > 0)
+	/*
+	 * The BOM is written just after the encryption magic number.
+	 * Skip it when appending and the file already existed, the BOM only
+	 * makes sense at the start of the file.
+	 */
+	if (buf->b_p_bomb && !write_bin && (!append || perm < 0))
 	{
-	    /* don't convert, do encryption */
-	    write_info.bw_flags = FIO_NOCONVERT | wb_flags;
-	    if (buf_write_bytes(&write_info) == FAIL)
-		end = 0;
-	    else
-		nchars += write_info.bw_len;
+	    write_info.bw_len = make_bom(buffer, fenc);
+	    if (write_info.bw_len > 0)
+	    {
+		/* don't convert, do encryption */
+		write_info.bw_flags = FIO_NOCONVERT | wb_flags;
+		if (buf_write_bytes(&write_info) == FAIL)
+		    end = 0;
+		else
+		    nchars += write_info.bw_len;
+	    }
 	}
-    }
-    write_info.bw_start_lnum = start;
+	write_info.bw_start_lnum = start;
 #endif
 
 #ifdef FEAT_PERSISTENT_UNDO
-    write_undo_file = (buf->b_p_udf && overwriting && !append
-					      && !filtering && reset_changed);
-    if (write_undo_file)
-	/* Prepare for computing the hash value of the text. */
-	sha256_start(&sha_ctx);
+	write_undo_file = (buf->b_p_udf
+			    && overwriting
+			    && !append
+			    && !filtering
+			    && reset_changed
+			    && !checking_conversion);
+	if (write_undo_file)
+	    /* Prepare for computing the hash value of the text. */
+	    sha256_start(&sha_ctx);
 #endif
 
-    write_info.bw_len = bufsize;
+	write_info.bw_len = bufsize;
 #ifdef HAS_BW_FLAGS
-    write_info.bw_flags = wb_flags;
+	write_info.bw_flags = wb_flags;
 #endif
-    fileformat = get_fileformat_force(buf, eap);
-    s = buffer;
-    len = 0;
-    for (lnum = start; lnum <= end; ++lnum)
-    {
-	/*
-	 * The next while loop is done once for each character written.
-	 * Keep it fast!
-	 */
-	ptr = ml_get_buf(buf, lnum, FALSE) - 1;
+	fileformat = get_fileformat_force(buf, eap);
+	s = buffer;
+	len = 0;
+	for (lnum = start; lnum <= end; ++lnum)
+	{
+	    /*
+	     * The next while loop is done once for each character written.
+	     * Keep it fast!
+	     */
+	    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));
+	    if (write_undo_file)
+		sha256_update(&sha_ctx, ptr + 1,
+					      (UINT32_T)(STRLEN(ptr + 1) + 1));
 #endif
-	while ((c = *++ptr) != NUL)
-	{
-	    if (c == NL)
-		*s = NUL;		/* replace newlines with NULs */
-	    else if (c == CAR && fileformat == EOL_MAC)
-		*s = NL;		/* Mac: replace CRs with NLs */
-	    else
-		*s = c;
-	    ++s;
-	    if (++len != bufsize)
-		continue;
-	    if (buf_write_bytes(&write_info) == FAIL)
+	    while ((c = *++ptr) != NUL)
 	    {
-		end = 0;		/* write error: break loop */
-		break;
-	    }
-	    nchars += bufsize;
-	    s = buffer;
-	    len = 0;
+		if (c == NL)
+		    *s = NUL;		/* replace newlines with NULs */
+		else if (c == CAR && fileformat == EOL_MAC)
+		    *s = NL;		/* Mac: replace CRs with NLs */
+		else
+		    *s = c;
+		++s;
+		if (++len != bufsize)
+		    continue;
+		if (buf_write_bytes(&write_info) == FAIL)
+		{
+		    end = 0;		/* write error: break loop */
+		    break;
+		}
+		nchars += bufsize;
+		s = buffer;
+		len = 0;
 #ifdef FEAT_MBYTE
-	    write_info.bw_start_lnum = lnum;
+		write_info.bw_start_lnum = lnum;
 #endif
-	}
-	/* write failed or last line has no EOL: stop here */
-	if (end == 0
-		|| (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;			/* written the line, count it */
-	    no_eol = TRUE;
-	    break;
-	}
-	if (fileformat == EOL_UNIX)
-	    *s++ = NL;
-	else
-	{
-	    *s++ = CAR;			/* EOL_MAC or EOL_DOS: write CR */
-	    if (fileformat == EOL_DOS)	/* write CR-NL */
+	    }
+	    /* write failed or last line has no EOL: stop here */
+	    if (end == 0
+		    || (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))))
 	    {
-		if (++len == bufsize)
+		++lnum;			/* written the line, count it */
+		no_eol = TRUE;
+		break;
+	    }
+	    if (fileformat == EOL_UNIX)
+		*s++ = NL;
+	    else
+	    {
+		*s++ = CAR;		    /* EOL_MAC or EOL_DOS: write CR */
+		if (fileformat == EOL_DOS)  /* write CR-NL */
 		{
-		    if (buf_write_bytes(&write_info) == FAIL)
+		    if (++len == bufsize)
 		    {
-			end = 0;	/* write error: break loop */
-			break;
+			if (buf_write_bytes(&write_info) == FAIL)
+			{
+			    end = 0;	/* write error: break loop */
+			    break;
+			}
+			nchars += bufsize;
+			s = buffer;
+			len = 0;
 		    }
-		    nchars += bufsize;
-		    s = buffer;
-		    len = 0;
+		    *s++ = NL;
 		}
-		*s++ = NL;
 	    }
-	}
-	if (++len == bufsize && end)
-	{
-	    if (buf_write_bytes(&write_info) == FAIL)
+	    if (++len == bufsize && end)
 	    {
-		end = 0;		/* write error: break loop */
-		break;
-	    }
-	    nchars += bufsize;
-	    s = buffer;
-	    len = 0;
+		if (buf_write_bytes(&write_info) == FAIL)
+		{
+		    end = 0;		/* write error: break loop */
+		    break;
+		}
+		nchars += bufsize;
+		s = buffer;
+		len = 0;
 
-	    ui_breakcheck();
-	    if (got_int)
-	    {
-		end = 0;		/* Interrupted, break loop */
-		break;
+		ui_breakcheck();
+		if (got_int)
+		{
+		    end = 0;		/* Interrupted, break loop */
+		    break;
+		}
 	    }
-	}
 #ifdef VMS
-	/*
-	 * On VMS there is a problem: newlines get added when writing 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
-	 * they don't it adds one.
-	 * With other RMS structures it works perfect without this fix.
-	 */
-	if (buf->b_fab_rfm == FAB$C_VFC
-		|| ((buf->b_fab_rat & (FAB$M_FTN | FAB$M_CR)) != 0))
-	{
-	    int b2write;
+	    /*
+	     * On VMS there is a problem: newlines get added when writing
+	     * 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 they don't it adds one.
+	     * With other RMS structures it works perfect without this fix.
+	     */
+	    if (buf->b_fab_rfm == FAB$C_VFC
+		    || ((buf->b_fab_rat & (FAB$M_FTN | FAB$M_CR)) != 0))
+	    {
+		int b2write;
 
-	    buf->b_fab_mrs = (buf->b_fab_mrs == 0
-		    ? MIN(4096, bufsize)
-		    : MIN(buf->b_fab_mrs, bufsize));
+		buf->b_fab_mrs = (buf->b_fab_mrs == 0
+			? MIN(4096, bufsize)
+			: MIN(buf->b_fab_mrs, bufsize));
 
-	    b2write = len;
-	    while (b2write > 0)
-	    {
-		write_info.bw_len = MIN(b2write, buf->b_fab_mrs);
-		if (buf_write_bytes(&write_info) == FAIL)
+		b2write = len;
+		while (b2write > 0)
 		{
-		    end = 0;
-		    break;
+		    write_info.bw_len = MIN(b2write, buf->b_fab_mrs);
+		    if (buf_write_bytes(&write_info) == FAIL)
+		    {
+			end = 0;
+			break;
+		    }
+		    b2write -= MIN(b2write, buf->b_fab_mrs);
 		}
-		b2write -= MIN(b2write, buf->b_fab_mrs);
+		write_info.bw_len = bufsize;
+		nchars += len;
+		s = buffer;
+		len = 0;
 	    }
-	    write_info.bw_len = bufsize;
+#endif
+	}
+	if (len > 0 && end > 0)
+	{
+	    write_info.bw_len = len;
+	    if (buf_write_bytes(&write_info) == FAIL)
+		end = 0;		    /* write error */
 	    nchars += len;
-	    s = buffer;
-	    len = 0;
 	}
-#endif
-    }
-    if (len > 0 && end > 0)
-    {
-	write_info.bw_len = len;
-	if (buf_write_bytes(&write_info) == FAIL)
-	    end = 0;		    /* write error */
-	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 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
-     * after writing the file.  That's because only the meta-data is
-     * journalled.  Syncing the file slows down the system, but assures it has
-     * been written to disk and we don't lose it.
-     * For a device do try the fsync() but don't complain if it does not work
-     * (could be a pipe).
-     * If the 'fsync' option is FALSE, don't fsync().  Useful for laptops. */
-    if (p_fs && fsync(fd) != 0 && !device)
+    /* If we started writing, finish writing. Also when an error was
+     * encountered. */
+    if (!checking_conversion)
     {
-	errmsg = (char_u *)_("E667: Fsync failed");
-	end = 0;
-    }
+#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 after writing the file.  That's because only the meta-data is
+	 * journalled.  Syncing the file slows down the system, but assures it
+	 * has been written to disk and we don't lose it.
+	 * For a device do try the fsync() but don't complain if it does not
+	 * 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");
+	    end = 0;
+	}
 #endif
 
 #if defined(HAVE_SELINUX) || defined(HAVE_SMACK)
-    /* Probably need to set the security context. */
-    if (!backup_copy)
-	mch_copy_sec(backup, wfname);
+	/* Probably need to set the security context. */
+	if (!backup_copy)
+	    mch_copy_sec(backup, wfname);
 #endif
 
 #ifdef UNIX
-    /* When creating a new file, set its owner/group to that of the original
-     * file.  Get the new device and inode number. */
-    if (backup != NULL && !backup_copy)
-    {
+	/* When creating a new file, set its owner/group to that of the
+	 * original file.  Get the new device and inode number. */
+	if (backup != NULL && !backup_copy)
+	{
 # ifdef HAVE_FCHOWN
-	stat_T	st;
+	    stat_T	st;
 
-	/* 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);
-	}
+	    /* 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);
+	    }
 # endif
-	buf_setino(buf);
-    }
-    else if (!buf->b_dev_valid)
-	/* Set the inode when creating a new file. */
-	buf_setino(buf);
+	    buf_setino(buf);
+	}
+	else if (!buf->b_dev_valid)
+	    /* Set the inode when creating a new file. */
+	    buf_setino(buf);
 #endif
 
-    if (close(fd) != 0)
-    {
-	errmsg = (char_u *)_("E512: Close failed");
-	end = 0;
-    }
+	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 */
+	if (made_writable)
+	    perm &= ~0200;	/* reset 'w' bit for security reasons */
 #endif
-    if (perm >= 0)		/* set perm. of new file same as old file */
-	(void)mch_setperm(wfname, perm);
+	if (perm >= 0)		/* set perm. of new file same as old file */
+	    (void)mch_setperm(wfname, perm);
 #ifdef HAVE_ACL
-    /*
-     * 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
-     * the mode of the file.  It's non-trivial for us to discover whether we're
-     * in that situation, so we simply always re-set the ACL.
-     */
+	/*
+	 * 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 the mode of the file.  It's non-trivial for us to discover
+	 * whether we're in that situation, so we simply always re-set the ACL.
+	 */
 # ifndef HAVE_SOLARIS_ZFS_ACL
-    if (!backup_copy)
+	if (!backup_copy)
 # endif
-	mch_set_acl(wfname, acl);
+	    mch_set_acl(wfname, acl);
 #endif
 #ifdef FEAT_CRYPT
-    if (buf->b_cryptstate != NULL)
-    {
-	crypt_free_state(buf->b_cryptstate);
-	buf->b_cryptstate = NULL;
-    }
+	if (buf->b_cryptstate != NULL)
+	{
+	    crypt_free_state(buf->b_cryptstate);
+	    buf->b_cryptstate = NULL;
+	}
 #endif
 
 #if defined(FEAT_MBYTE) && defined(FEAT_EVAL)
-    if (wfname != fname)
-    {
-	/*
-	 * The file was written to a temp file, now it needs to be converted
-	 * with 'charconvert' to (overwrite) the output file.
-	 */
-	if (end != 0)
+	if (wfname != fname)
 	{
-	    if (eval_charconvert(enc_utf8 ? (char_u *)"utf-8" : p_enc, fenc,
-						       wfname, fname) == FAIL)
+	    /*
+	     * The file was written to a temp file, now it needs to be
+	     * converted with 'charconvert' to (overwrite) the output file.
+	     */
+	    if (end != 0)
 	    {
-		write_info.bw_conv_error = TRUE;
-		end = 0;
+		if (eval_charconvert(enc_utf8 ? (char_u *)"utf-8" : p_enc,
+						  fenc, wfname, fname) == FAIL)
+		{
+		    write_info.bw_conv_error = TRUE;
+		    end = 0;
+		}
 	    }
+	    mch_remove(wfname);
+	    vim_free(wfname);
 	}
-	mch_remove(wfname);
-	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)
     {
diff --git a/src/testdir/test_writefile.vim b/src/testdir/test_writefile.vim
index 13c1a888d..6768e3154 100644
--- a/src/testdir/test_writefile.vim
+++ b/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
diff --git a/src/version.c b/src/version.c
index 48c62a458..d595256fa 100644
--- a/src/version.c
+++ b/src/version.c
@@ -765,6 +765,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    685,
+/**/
     684,
 /**/
     683,