From 1005f5dbe8b98bc665f57a7475f7dc876bdb9de6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 05:04:10 +0000 Subject: [PATCH] Save metadata and journal staleness: second-pass fixes - Ownership is applied before permissions: chown clears setuid/setgid, so saving a 06755 file left it 0755. - The umask is read once during static initialisation. Reading it at save time needed umask(0)/umask(old), briefly changing it for the swap writer thread creating files. - Before rewriting a hard-linked file in place, the new content is written and synced to a copy beside it; if the in-place write fails (ENOSPC, EIO) the copy is kept and named in the error. - Journal headers also record a CRC-32 of the base file (flags bit 1, bytes 40..43). A changed mtime with the same size and content (touch, checkout back and forth) no longer marks a journal stale. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Buffer.cc | 62 ++++++++++++++++++++++++++---- Swap.cc | 57 +++++++++++++++++++++++++-- Swap.h | 9 +++++ tests/test_audit_regressions.cc | 18 +++++++++ tests/test_swap_recovery_prompt.cc | 26 +++++++++++++ tests/test_swap_writer.cc | 4 +- 6 files changed, 162 insertions(+), 14 deletions(-) diff --git a/Buffer.cc b/Buffer.cc index a5f94d7..ba12d10 100644 --- a/Buffer.cc +++ b/Buffer.cc @@ -136,6 +136,16 @@ best_effort_fsync_dir(const std::string &path) } +// The process umask, read once during static initialisation (before any +// thread exists): reading it later needs umask(0) + umask(old), which briefly +// changes it for the swap writer thread creating files concurrently. +static const mode_t g_process_umask = [] { + const mode_t m = ::umask(0); + (void) ::umask(m); + return m; +}(); + + // 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 @@ -160,6 +170,31 @@ write_in_place(const std::string &path, const char *data, std::size_t len, std:: } +// Create `path` (which must not exist) with the content, fsynced. +static bool +write_new_file(const std::string &path, const char *data, std::size_t len, std::string &err) +{ + int flags = O_WRONLY | O_CREAT | O_EXCL; +#ifdef O_CLOEXEC + flags |= O_CLOEXEC; +#endif + const int fd = kte::syscall::Open(path.c_str(), flags, 0600); + if (fd < 0) { + err = "Failed to create " + path + ": " + 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); + if (!ok) + (void) ::unlink(path.c_str()); + return ok; +} + + static bool atomic_write_file(const std::string &path_in, const char *data, std::size_t len, std::string &err) { @@ -183,8 +218,20 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len, // 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); + if (dst_exists && S_ISREG(dst_st.st_mode) && dst_st.st_nlink > 1) { + // Truncating in place risks the only copy if the write then fails + // (ENOSPC, EIO, crash), so first write and sync a copy beside it. + const std::string copy = path + ".kte-save"; + if (!write_new_file(copy, data, len, err)) + return false; + if (!write_in_place(path, data, len, err)) { + err += " (the new content is in " + copy + ")"; + return false; + } + (void) ::unlink(copy.c_str()); + best_effort_fsync_dir(path); + return true; + } // Create a temp file in the same directory so rename() is atomic. std::filesystem::path p(path); @@ -216,17 +263,16 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len, std::string tmp_path(buf.data()); 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); + // Carry over ownership (best effort; needs privilege to give a file + // away) and then permissions: chown clears setuid/setgid, so the + // mode has to be applied after it. if (::fchown(fd, dst_st.st_uid, dst_st.st_gid) != 0) { // Expected without privilege when the owner differs; keep ours. } + (void) kte::syscall::Fchmod(fd, dst_st.st_mode & 07777); } 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); + (void) kte::syscall::Fchmod(fd, 0666 & ~g_process_umask); } bool ok = write_all_fd(fd, data, len, err); diff --git a/Swap.cc b/Swap.cc index 70d3598..9d5a88a 100644 --- a/Swap.cc +++ b/Swap.cc @@ -354,7 +354,7 @@ SwapManager::Attach(Buffer *buf) if (ctx.path.empty()) ctx.path = ComputeSidecarPath(*buf); if (fresh) - ctx.has_base = stat_base(buf->Filename(), ctx.base_size, ctx.base_mtime_ns); + capture_base(buf->Filename(), ctx); // Ensure a recorder exists as well. if (recorders_.find(buf) == recorders_.end()) { recorders_[buf] = std::make_unique(*this, *buf); @@ -433,7 +433,7 @@ SwapManager::ResetJournal(Buffer &buf) ctx.gap_chkpt_request_ns = 0; ctx.gap_unfixable_reported = false; ctx.locked_out = false; - ctx.has_base = stat_base(buf.Filename(), ctx.base_size, ctx.base_mtime_ns); + capture_base(buf.Filename(), ctx); ctx.last_flush_ns = 0; ctx.last_fsync_ns = 0; ctx.last_chkpt_ns = 0; @@ -566,7 +566,7 @@ SwapManager::NotifyFilenameChanged(Buffer &buf) ctx.gap_chkpt_request_ns = 0; ctx.gap_unfixable_reported = false; ctx.locked_out = false; - ctx.has_base = stat_base(buf.Filename(), ctx.base_size, ctx.base_mtime_ns); + capture_base(buf.Filename(), ctx); ctx.path = new_path; ctx.suspended = false; ctx.header_ok = false; @@ -693,10 +693,18 @@ SwapManager::write_header(int fd, const JournalCtx &ctx) hdr[10] = static_cast((VERSION >> 16) & 0xFFu); hdr[11] = static_cast((VERSION >> 24) & 0xFFu); // flags: bit 0 = base file identity present + // flags: bit 1 = base content CRC-32 present (bytes 40..43) if (ctx.has_base) { hdr[12] = 1; put_le64(hdr + 24, ctx.base_size); put_le64(hdr + 32, static_cast(ctx.base_mtime_ns)); + if (ctx.has_base_crc) { + hdr[12] |= 2; + hdr[40] = static_cast(ctx.base_crc & 0xFFu); + hdr[41] = static_cast((ctx.base_crc >> 8) & 0xFFu); + hdr[42] = static_cast((ctx.base_crc >> 16) & 0xFFu); + hdr[43] = static_cast((ctx.base_crc >> 24) & 0xFFu); + } } // created_time (unix seconds; little-endian) std::uint64_t ts = static_cast(std::time(nullptr)); @@ -1515,7 +1523,48 @@ SwapManager::JournalMatchesFile(const std::string &swap_path, const std::string std::int64_t mtime = 0; if (!stat_base(file_path, size, mtime)) return false; // the file the journal was based on is gone - return size == want_size && static_cast(mtime) == want_mtime; + if (size != want_size) + return false; + if (static_cast(mtime) == want_mtime) + return true; + // Timestamp changed (touch, checkout back and forth, sync tools): the + // content may still be the same, which is what matters. + if ((hdr[12] & 2u) == 0) + return false; + const std::uint32_t want_crc = static_cast(hdr[40]) | (static_cast(hdr[41]) << 8) | + (static_cast(hdr[42]) << 16) | + (static_cast(hdr[43]) << 24); + std::uint32_t crc = 0; + return file_crc32(file_path, crc) && crc == want_crc; +} + + +bool +SwapManager::file_crc32(const std::string &path, std::uint32_t &out) +{ + std::ifstream in(path, std::ios::binary); + if (!in) + return false; + std::uint32_t c = 0; + std::uint8_t chunk[1 << 16]; + while (in) { + in.read(reinterpret_cast(chunk), sizeof(chunk)); + const auto n = in.gcount(); + if (n > 0) + c = crc32(chunk, static_cast(n), c); + } + if (in.bad()) + return false; + out = c; + return true; +} + + +void +SwapManager::capture_base(const std::string &file, JournalCtx &ctx) +{ + ctx.has_base = stat_base(file, ctx.base_size, ctx.base_mtime_ns); + ctx.has_base_crc = ctx.has_base && file_crc32(file, ctx.base_crc); } diff --git a/Swap.h b/Swap.h index 966d65d..bb06434 100644 --- a/Swap.h +++ b/Swap.h @@ -222,6 +222,8 @@ private: bool has_base{false}; std::uint64_t base_size{0}; std::int64_t base_mtime_ns{0}; + bool has_base_crc{false}; // CRC-32 of the base file's content + std::uint32_t base_crc{0}; }; struct Pending { @@ -254,6 +256,13 @@ private: static std::uint32_t crc32(const std::uint8_t *data, std::size_t len, std::uint32_t seed = 0); + // CRC-32 of a file's content; false if it cannot be read. + static bool file_crc32(const std::string &path, std::uint32_t &out); + + // Record the identity (size, mtime, content CRC) of the file a journal's + // records will apply to. + static void capture_base(const std::string &file, JournalCtx &ctx); + static void put_le32(std::vector &out, std::uint32_t v); static void put_le64(std::uint8_t dst[8], std::uint64_t v); diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index 0c425dc..bf736ba 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -974,3 +974,21 @@ TEST(Audit_SaveAs_FileOpenInOtherBuffer_Refused) ASSERT_EQ(ed.BufferCount(), n); ASSERT_EQ(ed.FindOpenBuffer((d.path / "y.txt").string()) != ed.CurrentBufferIndex(), true); } + + +// Saving keeps setuid/setgid bits (chown after chmod used to clear them). +TEST(Audit_Save_KeepsSpecialModeBits) +{ + TempDir d("save_setuid"); + const auto f = d.path / "s.sh"; + std::ofstream(f) << "echo\n"; + ASSERT_EQ(::chmod(f.c_str(), 06755), 0); + Buffer b; + std::string err; + ASSERT_TRUE(b.OpenFromFile(f.string(), err)); + b.insert_text(0, 0, "#"); + ASSERT_TRUE(b.Save(err)); + struct stat st{}; + ASSERT_EQ(::stat(f.c_str(), &st), 0); + ASSERT_EQ(st.st_mode & 07777, (mode_t) 06755); +} diff --git a/tests/test_swap_recovery_prompt.cc b/tests/test_swap_recovery_prompt.cc index 0f17d14..67208aa 100644 --- a/tests/test_swap_recovery_prompt.cc +++ b/tests/test_swap_recovery_prompt.cc @@ -7,6 +7,7 @@ #include "tests/TestHarness.h" // for ktet::InstallDefaultCommandsOnce +#include #include #include #include @@ -653,3 +654,28 @@ TEST(SwapRecoveryPrompt_EmptyAnswer_KeepsSwap) ASSERT_TRUE(!ed.PromptActive()); ASSERT_TRUE(std::filesystem::exists(swp)); } + + +// A timestamp-only change (touch, git checkout back and forth) does not make +// a journal stale: its base content is unchanged. +TEST(SwapRecoveryPrompt_TouchedFile_StillRecoverable) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("touched"); + const std::string file = (sb.root / "work" / "t2.txt").string(); + write_file_bytes(file, "base\n"); + std::string expected; + (void) make_journal(file, [](Buffer &b) { + b.insert_text(0, 0, std::string("A")); + }, expected); + std::filesystem::last_write_time(file, std::filesystem::last_write_time(file) + std::chrono::hours(1)); + + Editor ed; + ed.SetDimensions(24, 80); + ed.AddBuffer(Buffer()); + ed.RequestOpenFile(file); + (void) ed.ProcessPendingOpens(); + ASSERT_EQ(ed.PendingRecoveryPrompt(), Editor::RecoveryPromptKind::RecoverOrDiscard); + answer(ed, "y"); + ASSERT_EQ(buffer_bytes_via_views(*ed.CurrentBuffer()), expected); +} diff --git a/tests/test_swap_writer.cc b/tests/test_swap_writer.cc index 4082b23..4697214 100644 --- a/tests/test_swap_writer.cc +++ b/tests/test_swap_writer.cc @@ -125,8 +125,8 @@ TEST(SwapWriter_Header_Records_And_CRC) ASSERT_EQ(bytes[(std::size_t) i], magic[i]); ASSERT_EQ(read_le32(bytes.data() + 8), (std::uint32_t) 1); // flags bit 0: the base file's identity (size, mtime) is recorded, since - // the buffer is file-backed. - ASSERT_EQ(read_le32(bytes.data() + 12), (std::uint32_t) 1); + // the buffer is file-backed; bit 1: its content CRC-32 too. + ASSERT_EQ(read_le32(bytes.data() + 12), (std::uint32_t) 3); ASSERT_EQ(read_le64(bytes.data() + 24), (std::uint64_t) std::filesystem::file_size(b.Filename())); ASSERT_TRUE(read_le64(bytes.data() + 16) != 0);