From 42b1fcf2495703c05a7ae1515d7b7b7e6b26f864 Mon Sep 17 00:00:00 2001
From: Michalis Kokologiannakis <mixaskok@gmail.com>
Date: Mon, 22 Jun 2020 18:36:17 +0300
Subject: [PATCH] files: ensure no data loss under ext4 when saving

The current save procedure followed by nano is not crash-safe
under ext4 when editing existing files. Modifying an existing
file can lead to data loss, since a modified file is not
replaced atomically on disk.

This commit address this issue as it:

1) Takes a temporary backup of the file being written so that
there is no time window such that (a) an existing file is truncated
and (b) the buffer corresponding to said file has not been
flushed to disk. Such windows are dangerous because, under certain
circumstances, they can lead to data loss under ext4.

2) Uses fsync() so that, when save finishes, it is certain that
the file has been successfully written to disk.

Signed-off-by: Michalis Kokologiannakis <michalis@mpi-sws.org>
---
 src/files.c | 132 +++++++++++++++++-----------------------------------
 1 file changed, 42 insertions(+), 90 deletions(-)

diff --git a/src/files.c b/src/files.c
index 9d0a1ca4..d4c5bd1c 100644
--- a/src/files.c
+++ b/src/files.c
@@ -1589,6 +1589,8 @@ bool write_file(const char *name, FILE *thefile, bool tmp,
 		/* The status fields filled in by stat(). */
 	char *backupname = NULL;
 		/* The name of the backup file, in case we make one. */
+	bool is_file_backed = FALSE;
+	        /* Whether the original is moved to a backup. */
 #endif
 	char *realname = real_dir_from_tilde(name);
 		/* The filename after tilde expansion. */
@@ -1619,29 +1621,13 @@ bool write_file(const char *name, FILE *thefile, bool tmp,
 	if (openfile->statinfo == NULL && is_existing_file)
 		stat_with_alloc(realname, &openfile->statinfo);
 
-	/* When the user requested a backup, we do this only if the file exists and
-	 * isn't temporary AND the file has not been modified by someone else since
-	 * we opened it (or we are appending/prepending or writing a selection). */
-	if (ISSET(MAKE_BACKUP) && is_existing_file && openfile->statinfo &&
-						(openfile->statinfo->st_mtime == st.st_mtime ||
-						method != OVERWRITE || openfile->mark)) {
-		static struct timespec filetime[2];
-		int backup_cflags, backup_fd, verdict;
-		FILE *original = NULL, *backup_file = NULL;
-
-		/* Remember the original file's access and modification times. */
-		filetime[0].tv_sec = openfile->statinfo->st_atime;
-		filetime[1].tv_sec = openfile->statinfo->st_mtime;
-
-		/* Open the file of which a backup must be made. */
-		original = fopen(realname, "rb");
-
-		/* If we can't read from the original file, go on, since saving
-		 * only the current buffer is better than saving nothing. */
-		if (original == NULL) {
-			statusline(ALERT, _("Error reading %s: %s"), realname, strerror(errno));
-			goto skip_backup;
-		}
+	/* We make a temporary backup if the file exists and isn't temporary AND
+	 * the file has not been modified by someone else since we opened it
+	 * (or we are appending/prepending or writing a selection).
+	 * The backup will become permanent later, and only if the user has specified so. */
+	if (is_existing_file && openfile->statinfo &&
+	    (openfile->statinfo->st_mtime == st.st_mtime ||
+	     method != OVERWRITE || openfile->mark)) {
 
 		/* If no backup directory was specified, we make a simple backup
 		 * by appending a tilde to the original file name.  Otherwise,
@@ -1678,81 +1664,23 @@ bool write_file(const char *name, FILE *thefile, bool tmp,
 			}
 		}
 
-		/* Now first try to delete an existing backup file. */
-		if (unlink(backupname) < 0 && errno != ENOENT && !ISSET(INSECURE_BACKUP)) {
-			warn_and_briefly_pause(_("Cannot delete existing backup"));
-			if (user_wants_to_proceed())
-				goto skip_backup;
-			statusline(HUSH, _("Cannot delete backup %s: %s"),
-								backupname, strerror(errno));
-			goto cleanup_and_exit;
-		}
-
-		if (ISSET(INSECURE_BACKUP))
-			backup_cflags = O_WRONLY | O_CREAT | O_TRUNC;
-		else
-			backup_cflags = O_WRONLY | O_CREAT | O_EXCL;
-
-		/* Create the backup file (or truncate the existing one). */
-		backup_fd = open(backupname, backup_cflags, S_IRUSR|S_IWUSR);
-
-		if (backup_fd >= 0)
-			backup_file = fdopen(backup_fd, "wb");
-
-		if (backup_file == NULL) {
+		/* Move the existing file to the backup. */
+		if (rename(realname, backupname) == -1) {
 			warn_and_briefly_pause(_("Cannot create backup file"));
 			if (user_wants_to_proceed())
 				goto skip_backup;
 			statusline(HUSH, _("Cannot create backup %s: %s"),
-								backupname, strerror(errno));
-			goto cleanup_and_exit;
-		}
-
-		/* Try to change owner and group to those of the original file;
-		 * ignore errors, as a normal user cannot change the owner. */
-		IGNORE_CALL_RESULT(fchown(backup_fd, openfile->statinfo->st_uid,
-											openfile->statinfo->st_gid));
-
-		/* Set the backup's permissions to those of the original file.
-		 * It is not a security issue if this fails, as we have created
-		 * the file with just read and write permission for the owner. */
-		IGNORE_CALL_RESULT(fchmod(backup_fd, openfile->statinfo->st_mode));
-
-		/* Copy the existing file to the backup. */
-		verdict = copy_file(original, backup_file, FALSE);
-
-		if (verdict < 0) {
-			fclose(backup_file);
-			statusline(ALERT, _("Error reading %s: %s"), realname, strerror(errno));
-			goto cleanup_and_exit;
-		} else if (verdict > 0) {
-			fclose(backup_file);
-			warn_and_briefly_pause(_("Cannot write backup file"));
-			if (user_wants_to_proceed())
-				goto skip_backup;
-			statusline(HUSH, _("Cannot write backup %s: %s"),
-								backupname, strerror(errno));
+				   backupname, strerror(errno));
 			goto cleanup_and_exit;
-		}
-
-		/* Set the backup's timestamps to those of the original file.
-		 * Failure is unimportant: saving the file apparently worked. */
-		IGNORE_CALL_RESULT(futimens(backup_fd, filetime));
-
-		if (fclose(backup_file) != 0) {
-			warn_and_briefly_pause(_("Cannot write backup"));
-			if (user_wants_to_proceed())
-				goto skip_backup;
-			statusline(HUSH, _("Cannot write backup %s: %s"),
-								backupname, strerror(errno));
-			goto cleanup_and_exit;
-		}
+		} else
+			is_file_backed = TRUE;
 	}
 
   skip_backup:
 	/* When prepending, first copy the existing file to a temporary file. */
 	if (method == PREPEND) {
-		FILE *source = fopen(realname, "rb");
+		char *original = (is_file_backed) ? backupname : realname;
+		FILE *source = fopen(original, "rb");
 		FILE *target = NULL;
 		int verdict;
 
@@ -1890,24 +1818,48 @@ bool write_file(const char *name, FILE *thefile, bool tmp,
 			goto cleanup_and_exit;
 		}
 
-		verdict = copy_file(source, thefile, TRUE);
+		verdict = copy_file(source, thefile, FALSE);
 
 		if (verdict < 0) {
+			fclose(thefile);
 			statusline(ALERT, _("Error reading temp file: %s"), strerror(errno));
 			goto cleanup_and_exit;
 		} else if (verdict > 0) {
+			fclose(thefile);
 			statusline(ALERT, _("Error writing %s: %s"), realname, strerror(errno));
 			goto cleanup_and_exit;
 		}
 
 		unlink(tempname);
-	} else
+	}
 #endif
+	/* Send all buffered data from fwrite()s to the kernel. */
+	if (fflush(thefile) != 0) {
+		fclose(thefile);
+		statusline(ALERT, _("Error writing %s: %s"), realname, strerror(errno));
+		goto cleanup_and_exit;
+	}
+
+	/* Make sure data reaches disk. */
+	if (fsync(fileno(thefile)) != 0) {
+		fclose(thefile);
+		statusline(ALERT, _("Error writing %s: %s"), realname, strerror(errno));
+		goto cleanup_and_exit;
+	}
+
 	if (fclose(thefile) != 0) {
 		statusline(ALERT, _("Error writing %s: %s"), realname, strerror(errno));
 		goto cleanup_and_exit;
 	}
 
+	/* If the user hasn't specified that a backup should be created, delete the
+	 * temporary backup created to ensure the atomic replacement of the file. */
+	if (!ISSET(MAKE_BACKUP) && backupname != NULL) {
+		/* Doesn't really matter if this fails, since the file has been written. */
+		if (unlink(backupname) != 0)
+			warn_and_briefly_pause(_("Cannot delete temporary backup"));
+	}
+
 	/* When having written an entire buffer, update some administrivia. */
 	if (fullbuffer && method == OVERWRITE && !tmp) {
 		/* If the filename was changed, write a new lockfile when needed,
-- 
2.27.0

