Save: don't lie about fsync; keep file metadata; confirm overwrites

- fsync is never retried. After a writeback error Linux may mark the
  pages clean, so a retried fsync could succeed although the data was
  lost, and the rename then replaced a good file with a bad one.
- New files get 0666 & ~umask instead of mkstemp's 0600. Replaced files
  keep their mode (file-type bits masked) and, best effort, their owner
  and group.
- Saving through a symlink writes the link's target instead of
  replacing the link with a regular file (SaveAs did not resolve it).
- A file with several hard links is rewritten in place, so the other
  names see the new content (as vim does by default).
- Save-as (prompt and ':save-as') asks before replacing any existing
  file other than the buffer's own; it used to ask only for unnamed
  buffers. Save-and-quit refuses to overwrite a file changed on disk,
  like save. The first save of a new file records the saved undo state.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB

Buffer.cc

@@ -136,9 +136,51 @@ best_effort_fsync_dir(const std::string &path)
 }
 
 
+// Rewrite `path` in place (truncate, write, fsync). Used for files with
+// several hard links, which a temp-file-and-rename save would split.
 static bool
-atomic_write_file(const std::string &path, const char *data, std::size_t len, std::string &err)
+write_in_place(const std::string &path, const char *data, std::size_t len, std::string &err)
 {
+	int flags = O_WRONLY | O_TRUNC;
+#ifdef O_CLOEXEC
+	flags |= O_CLOEXEC;
+#endif
+	const int fd = kte::syscall::Open(path.c_str(), flags);
+	if (fd < 0) {
+		err = std::string("Failed to open file for writing: ") + std::strerror(errno);
+		return false;
+	}
+	bool ok = write_all_fd(fd, data, len, err);
+	if (ok && kte::syscall::Fsync(fd) != 0) {
+		err = std::string("fsync failed: ") + std::strerror(errno);
+		ok  = false;
+	}
+	(void) kte::syscall::Close(fd);
+	return ok;
+}
+
+
+static bool
+atomic_write_file(const std::string &path_in, const char *data, std::size_t len, std::string &err)
+{
+	// Write through a symlink to its target: renaming over the link itself
+	// would replace the link with a regular file.
+	std::string path = path_in;
+	try {
+		if (std::filesystem::is_symlink(path_in))
+			path = std::filesystem::weakly_canonical(path_in).string();
+	} catch (...) {
+		// Fall back to the given path.
+	}
+
+	struct stat dst_st{};
+	const bool dst_exists = ::stat(path.c_str(), &dst_st) == 0;
+
+	// A file with other hard links must be rewritten in place, or those
+	// names would keep the old content.
+	if (dst_exists && S_ISREG(dst_st.st_mode) && dst_st.st_nlink > 1)
+		return write_in_place(path, data, len, err);
+
 	// Create a temp file in the same directory so rename() is atomic.
 	std::filesystem::path p(path);
 	std::filesystem::path dir  = p.parent_path();
@@ -168,25 +210,26 @@ atomic_write_file(const std::string &path, const char *data, std::size_t len, st
 	}
 	std::string tmp_path(buf.data());
 
-	// If the destination exists, carry over its permissions.
-	struct stat dst_st{};
-	if (::stat(path.c_str(), &dst_st) == 0) {
-		(void) kte::syscall::Fchmod(fd, dst_st.st_mode);
+	if (dst_exists) {
+		// Carry over permissions and (best effort; needs privilege to give a
+		// file away) ownership of the file being replaced.
+		(void) kte::syscall::Fchmod(fd, dst_st.st_mode & 07777);
+		(void) ::fchown(fd, dst_st.st_uid, dst_st.st_gid);
+	} else {
+		// mkstemp creates 0600; a new file gets the usual 0666 & ~umask.
+		const mode_t mask = ::umask(0);
+		(void) ::umask(mask);
+		(void) kte::syscall::Fchmod(fd, 0666 & ~mask);
 	}
 
 	bool ok = write_all_fd(fd, data, len, err);
-	if (ok) {
-		// Retry fsync on transient errors
-		auto fsync_fn = [&]() -> bool {
-			return kte::syscall::Fsync(fd) == 0;
-		};
-
-		std::string fsync_err;
-		if (!kte::RetryOnTransientError(fsync_fn, kte::RetryPolicy::Aggressive(), fsync_err)) {
-			err = std::string("fsync failed: ") + std::strerror(errno) + fsync_err;
+	// Never retry fsync: after a writeback error Linux may mark the pages
+	// clean, so a second fsync can succeed although the data was lost, and
+	// the rename below would then replace a good file with a bad one.
+	if (ok && kte::syscall::Fsync(fd) != 0) {
+		err = std::string("fsync failed: ") + std::strerror(errno);
 		ok  = false;
 	}
-	}
 	(void) kte::syscall::Close(fd);
 
 	if (ok) {

Command.cc

@@ -342,6 +342,20 @@ is_mutating_command(CommandId id)
 }
 
 
+// True if `path` names the file `buf` is backed by (same inode).
+static bool
+is_buffers_own_file(const Buffer &buf, const std::string &path)
+{
+	if (!buf.IsFileBacked() || buf.Filename().empty())
+		return false;
+	try {
+		return std::filesystem::equivalent(path, buf.Filename());
+	} catch (...) {
+		return false;
+	}
+}
+
+
 // Commands that may run while a prompt is open: prompt editing and
 // navigation (their handlers check PromptActive), cancel, and view-only
 // commands that do not touch buffer contents or buffer selection.
@@ -907,6 +921,8 @@ cmd_save(CommandContext &ctx)
 				if (auto *sm = ctx.editor.Swap())
 					sm->ResetJournal(*buf);
 				ctx.editor.SetStatus("Saved " + buf->Filename());
+				if (auto *u = buf->Undo())
+					u->mark_saved();
 				return true;
 			}
 		}
@@ -980,6 +996,13 @@ cmd_save_as(CommandContext &ctx)
 		ctx.editor.SetStatus("save-as requires a filename");
 		return false;
 	}
+	// Ask before replacing an existing file other than the buffer's own.
+	if (std::filesystem::exists(ctx.arg) && !is_buffers_own_file(*buf, ctx.arg)) {
+		ctx.editor.StartPrompt(Editor::PromptKind::Confirm, "Overwrite", "");
+		ctx.editor.SetPendingOverwritePath(ctx.arg);
+		ctx.editor.SetStatus(std::string("Overwrite existing file '") + ctx.arg + "'? (y/N)");
+		return true;
+	}
 	std::string err;
 	if (!buf->SaveAs(ctx.arg, err)) {
 		ctx.editor.SetStatus(err);
@@ -1027,6 +1050,12 @@ cmd_save_and_quit(CommandContext &ctx)
 	Buffer *buf = ctx.editor.CurrentBuffer();
 	if (buf && buf->Dirty()) {
 		std::string err;
+		// Same guard as save: do not silently overwrite a file that changed
+		// on disk since it was loaded.
+		if (buf->IsFileBacked() && buf->ExternallyModifiedOnDisk()) {
+			ctx.editor.SetStatus("File changed on disk; save with C-k s to confirm overwriting, then quit");
+			return false;
+		}
 		if (buf->IsFileBacked()) {
 			if (buf->Save(err)) {
 				buf->SetDirty(false);
@@ -2903,9 +2932,9 @@ cmd_newline(CommandContext &ctx)
 						return in;
 					};
 					value = expand_user_path(value);
-					// If this is a first-time save (unnamed/non-file-backed) and the
-					// target exists, ask for confirmation before overwriting.
-					if (!buf->IsFileBacked() && std::filesystem::exists(value)) {
+					// Ask before overwriting any existing file other than the
+					// buffer's own (it used to ask only for unnamed buffers).
+					if (std::filesystem::exists(value) && !is_buffers_own_file(*buf, value)) {
 						ctx.editor.StartPrompt(Editor::PromptKind::Confirm, "Overwrite", "");
 						ctx.editor.SetPendingOverwritePath(value);
 						ctx.editor.SetStatus(

tests/test_audit_regressions.cc

@@ -14,6 +14,10 @@
 #include "syntax/LanguageHighlighter.h"
 
 #include <chrono>
+#include <filesystem>
+#include <fstream>
+#include <sys/stat.h>
+#include <unistd.h>
 #include <random>
 #include <vector>
 #include <clocale>
@@ -716,3 +720,101 @@ TEST(Audit_Regex_LongLine_NoStackOverflow)
 	ASSERT_EQ(h.Line(0), std::string("b"));
 	ASSERT_EQ(h.Line(1), std::string("short"));
 }
+
+
+namespace {
+struct TempDir {
+	std::filesystem::path path;
+
+
+	explicit TempDir(const char *tag)
+	{
+		path = std::filesystem::temp_directory_path() /
+		       (std::string("kte_ut_") + tag + "_" + std::to_string((int) ::getpid()));
+		std::filesystem::remove_all(path);
+		std::filesystem::create_directories(path);
+	}
+
+
+	~TempDir()
+	{
+		std::filesystem::remove_all(path);
+	}
+};
+
+
+std::string
+slurp(const std::filesystem::path &p)
+{
+	std::ifstream in(p, std::ios::binary);
+	return std::string((std::istreambuf_iterator<char>(in)), std::istreambuf_iterator<char>());
+}
+} // namespace
+
+
+// New files honour the umask (mkstemp's 0600 used to stick).
+TEST(Audit_Save_NewFileHonoursUmask)
+{
+	TempDir d("save_umask");
+	const mode_t old = ::umask(022);
+	Buffer b;
+	b.insert_text(0, 0, "x\n");
+	std::string err;
+	const bool ok = b.SaveAs((d.path / "new.txt").string(), err);
+	::umask(old);
+	ASSERT_TRUE(ok);
+	struct stat st{};
+	ASSERT_EQ(::stat((d.path / "new.txt").c_str(), &st), 0);
+	ASSERT_EQ(st.st_mode & 0777, (mode_t) 0644);
+}
+
+
+// Save-as onto a symlink writes the target and keeps the link.
+TEST(Audit_Save_ThroughSymlinkKeepsLink)
+{
+	TempDir d("save_symlink");
+	std::ofstream(d.path / "target.txt") << "old\n";
+	std::filesystem::create_symlink(d.path / "target.txt", d.path / "link.txt");
+	Buffer b;
+	b.insert_text(0, 0, "new\n");
+	std::string err;
+	ASSERT_TRUE(b.SaveAs((d.path / "link.txt").string(), err));
+	ASSERT_TRUE(std::filesystem::is_symlink(d.path / "link.txt"));
+	ASSERT_EQ(slurp(d.path / "target.txt"), std::string("new\n"));
+}
+
+
+// A file with several hard links is rewritten in place, so every name sees
+// the new content.
+TEST(Audit_Save_KeepsHardLinks)
+{
+	TempDir d("save_hardlink");
+	std::ofstream(d.path / "a.txt") << "old\n";
+	std::filesystem::create_hard_link(d.path / "a.txt", d.path / "b.txt");
+	Buffer b;
+	std::string err;
+	ASSERT_TRUE(b.OpenFromFile((d.path / "a.txt").string(), err));
+	b.insert_text(0, 0, "new ");
+	ASSERT_TRUE(b.Save(err));
+	ASSERT_EQ(slurp(d.path / "b.txt"), std::string("new old\n"));
+	ASSERT_EQ(std::filesystem::hard_link_count(d.path / "a.txt"), (std::uintmax_t) 2);
+}
+
+
+// Save-as from a file-backed buffer onto another existing file asks first.
+TEST(Audit_SaveAs_ExistingFile_AsksFirst)
+{
+	TempDir d("saveas_confirm");
+	std::ofstream(d.path / "mine.txt") << "mine\n";
+	std::ofstream(d.path / "other.txt") << "precious\n";
+	TestHarness h;
+	Editor &ed = h.EditorRef();
+	std::string err;
+	ASSERT_TRUE(ed.OpenFile((d.path / "mine.txt").string(), err));
+	ASSERT_TRUE(Execute(ed, "save-as", (d.path / "other.txt").string()));
+	ASSERT_TRUE(ed.PromptActive());
+	ASSERT_EQ(slurp(d.path / "other.txt"), std::string("precious\n"));
+	ASSERT_TRUE(h.Exec(CommandId::InsertText, "n"));
+	ASSERT_TRUE(h.Exec(CommandId::Newline));
+	ASSERT_EQ(slurp(d.path / "other.txt"), std::string("precious\n"));
+}