diff --git a/Buffer.cc b/Buffer.cc index efdef54..830aa75 100644 --- a/Buffer.cc +++ b/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,24 +210,25 @@ 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; - ok = false; - } + // 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); diff --git a/Command.cc b/Command.cc index 897b83c..fc671ff 100644 --- a/Command.cc +++ b/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( diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index 2b88e3c..fc0e3d5 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -14,6 +14,10 @@ #include "syntax/LanguageHighlighter.h" #include +#include +#include +#include +#include #include #include #include @@ -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(in)), std::istreambuf_iterator()); +} +} // 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")); +}