From fb50a02f6ac69bfee0a0b623252f088724723f54 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 04:58:02 +0000 Subject: [PATCH] Swap ownership, fsync race, RowsView memory, renderer regex stack - A session never unlinks a journal it does not own: Detach, ResetJournal and NotifyFilenameChanged skip removal when this session was locked out or another session holds the lock, and PruneSwapDir skips held journals. A second session used to delete the first session's live journal on save or close. - The writer thread's periodic fsync now runs under mtx_. It fsynced fds that the main thread could close concurrently (or that had been reused); an in-flight fsync also kept a just-closed journal's flock alive, so the owner's own cleanup saw its journal as held. - Compaction locks the compacted file before renaming it into place and keeps using it, so the journal is never unlocked in between; a failed lock on the truncating reopen marks the session locked out. - Unnamed-buffer journals include the pid, so sessions no longer share (and delete) each other's unnamed-N.swp. - RowsView keeps only the previous generation of cached rows: keeping all of them made C-u 2000 M-d on a 1 MiB line use ~2 GB. - Renderers run regex match highlighting through ForEachRegexMatch, which uses the large-stack worker for lines over 512 bytes; some patterns overflowed the main stack well under the previous 10,000 byte limit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Command.cc | 13 ++-- ImGuiRenderer.cc | 14 ++-- QtFrontend.cc | 18 ++--- RegexGuard.cc | 18 +++++ RegexGuard.h | 15 ++++- Swap.cc | 104 ++++++++++++++++++----------- TerminalRenderer.cc | 10 +-- tests/test_audit_regressions.cc | 18 +++++ tests/test_swap_recovery_prompt.cc | 39 +++++++++++ 9 files changed, 172 insertions(+), 77 deletions(-) diff --git a/Command.cc b/Command.cc index 1da39fe..606decb 100644 --- a/Command.cc +++ b/Command.cc @@ -457,9 +457,11 @@ compute_mark_region(Buffer &buf, std::size_t &sx, std::size_t &sy, std::size_t & // Row access for commands that touch a few rows. Buffer::Rows() rebuilds a // string for every line of the file on the first call after any edit, which // made every motion and delete O(file size). RowsView fetches rows on demand -// and caches them for the current buffer version. Strings fetched for an -// older version are retired, not freed, so a reference obtained before an -// edit stays valid (if stale) for the lifetime of the view, as with Rows(). +// and caches them for the current buffer version. When the version changes +// the current cache becomes the previous one (the one before that is freed), +// so a reference obtained just before an edit stays valid (if stale) across +// that edit. Keeping every generation grew without bound in loops that edit +// on each pass (C-u 2000 M-d on a long line used ~2 GB). class RowsView { public: explicit RowsView(const Buffer &buf) : buf_(&buf) {} @@ -480,8 +482,7 @@ public: const std::string &operator[](std::size_t row) const { if (buf_->Version() != version_) { - if (!cache_.empty()) - retired_.push_back(std::move(cache_)); + previous_ = std::move(cache_); cache_.clear(); version_ = buf_->Version(); } @@ -495,7 +496,7 @@ private: const Buffer *buf_; mutable std::uint64_t version_ = ~std::uint64_t{0}; mutable std::map cache_; - mutable std::vector > retired_; + mutable std::map previous_; }; diff --git a/ImGuiRenderer.cc b/ImGuiRenderer.cc index 67f378f..3abee55 100644 --- a/ImGuiRenderer.cc +++ b/ImGuiRenderer.cc @@ -360,17 +360,13 @@ ImGuiRenderer::Draw(Editor &ed) if (search_mode) { // In regex mode, reuse the compiled regex hoisted above the loop. if (regex_mode) { - // Long lines are not highlighted: std::regex recursion - // could overflow the stack (RegexGuard.h). + // std::regex recursion is kept off the main stack for long + // lines (RegexGuard.h); very long lines are not highlighted. if (search_rx_valid && line.size() <= kte::kRegexRenderLineLimit) { try { - for (auto it = std::sregex_iterator(line.begin(), line.end(), search_rx); - it != std::sregex_iterator(); ++it) { - const auto &m = *it; - std::size_t sx = static_cast(m.position()); - std::size_t ex = sx + static_cast(m.length()); - hl_src_ranges.emplace_back(sx, ex); - } + kte::ForEachRegexMatch(line, search_rx, [&](std::size_t pos, std::size_t len) { + hl_src_ranges.emplace_back(pos, pos + len); + }); } catch (const std::regex_error &) { // ignore invalid patterns here; status line already shows the error } diff --git a/QtFrontend.cc b/QtFrontend.cc index 16b9e98..406cf66 100644 --- a/QtFrontend.cc +++ b/QtFrontend.cc @@ -166,21 +166,13 @@ protected: (ed_->CurrentPromptKind() == Editor::PromptKind::RegexSearch || ed_->CurrentPromptKind() == Editor::PromptKind::RegexReplaceFind)) { - // Long lines are not highlighted: std::regex recursion - // could overflow the stack (RegexGuard.h). + // std::regex recursion is kept off the main stack for long + // lines (RegexGuard.h); very long lines are not highlighted. if (line.size() <= kte::kRegexRenderLineLimit) try { std::regex rx(ed_->SearchQuery()); - for (auto it = std::sregex_iterator( - line.begin(), line.end(), rx); - it != std::sregex_iterator(); ++it) { - const auto &m = *it; - std::size_t sx = static_cast(m. - position()); - std::size_t ex = - sx + static_cast(m. - length()); - hl_src_ranges.emplace_back(sx, ex); - } + kte::ForEachRegexMatch(line, rx, [&](std::size_t pos, std::size_t len) { + hl_src_ranges.emplace_back(pos, pos + len); + }); } catch (const std::regex_error &) { // Invalid regex: ignore, status line already shows errors } diff --git a/RegexGuard.cc b/RegexGuard.cc index 31dd133..c9d61e7 100644 --- a/RegexGuard.cc +++ b/RegexGuard.cc @@ -48,4 +48,22 @@ RunWithLargeStack(const std::function &fn) if (job.error) std::rethrow_exception(job.error); } + + +void +ForEachRegexMatch(const std::string &line, const std::regex &rx, + const std::function &on_match) +{ + auto scan = [&] { + for (auto it = std::sregex_iterator(line.begin(), line.end(), rx); it != std::sregex_iterator(); ++it) + on_match(static_cast(it->position()), static_cast(it->length())); + }; + // Stack use grows with match length; a few hundred bytes stays far below + // any thread's stack for every pattern we measured. + constexpr std::size_t kInlineLimit = 512; + if (line.size() <= kInlineLimit) + scan(); + else + RunWithLargeStack(scan); +} } // namespace kte diff --git a/RegexGuard.h b/RegexGuard.h index 6d0ebac..b6cf352 100644 --- a/RegexGuard.h +++ b/RegexGuard.h @@ -8,15 +8,24 @@ #include #include +#include +#include namespace kte { -// Renderers highlight regex matches only on lines up to this many bytes; -// longer lines are drawn without match highlighting. -constexpr std::size_t kRegexRenderLineLimit = 10000; +// Renderers highlight regex matches only on lines up to this many bytes (to +// bound per-frame work); longer lines are drawn without match highlighting. +constexpr std::size_t kRegexRenderLineLimit = 1000000; // Run fn to completion on a thread with a large stack (address space is // reserved, pages are committed only as used), so regex work over long lines // cannot overflow. If such a thread cannot be created, fn runs on the calling // thread. Exceptions thrown by fn are rethrown to the caller. void RunWithLargeStack(const std::function &fn); + +// Call on_match(position, length) for each match of rx in line. Lines short +// enough to be safe on any stack are matched inline; longer ones on a large +// stack (a 10,000-byte line already overflowed 8 MiB for some patterns). +// Throws what std::regex throws. +void ForEachRegexMatch(const std::string &line, const std::regex &rx, + const std::function &on_match); } // namespace kte diff --git a/Swap.cc b/Swap.cc index dedf1cc..70d3598 100644 --- a/Swap.cc +++ b/Swap.cc @@ -29,6 +29,8 @@ constexpr std::size_t kMaxRecordPayload = 0xFFFFFFu; constexpr std::uint64_t kGapCheckpointIntervalNs = 1000000000ULL; } // namespace +static void remove_journal_unless_held(const std::string &path, bool locked_out); + namespace { constexpr std::uint8_t MAGIC[8] = {'K', 'T', 'E', '_', 'S', 'W', 'P', '\0'}; constexpr std::uint32_t VERSION = 1; @@ -381,26 +383,28 @@ SwapManager::Detach(Buffer *buf, const bool remove_file) Flush(buf); std::string path; + bool locked_out = false; { std::lock_guard lg(mtx_); auto it = journals_.find(buf); if (it != journals_.end()) { - path = it->second.path; + path = it->second.path; + locked_out = it->second.locked_out; close_ctx(it->second); journals_.erase(it); } recorders_.erase(buf); } - if (remove_file && !path.empty()) { - (void) std::remove(path.c_str()); - } + if (remove_file) + remove_journal_unless_held(path, locked_out); } void SwapManager::ResetJournal(Buffer &buf) { + bool was_locked_out = false; std::string path; { std::lock_guard lg(mtx_); @@ -423,6 +427,7 @@ SwapManager::ResetJournal(Buffer &buf) return; JournalCtx &ctx = it->second; close_ctx(ctx); + was_locked_out = ctx.locked_out; ctx.header_ok = false; ctx.gap = false; // the journal restarts from the saved file ctx.gap_chkpt_request_ns = 0; @@ -437,9 +442,7 @@ SwapManager::ResetJournal(Buffer &buf) ctx.suspended = false; } - if (!path.empty()) { - (void) std::remove(path.c_str()); - } + remove_journal_unless_held(path, was_locked_out); } @@ -520,7 +523,8 @@ SwapManager::PruneSwapDir() too_old = true; } bool over_limit = (cfg.prune_max_files > 0) && (kept >= cfg.prune_max_files); - if (too_old || over_limit) { + // Never prune a journal another running session holds. + if ((too_old || over_limit) && !JournalInUse(e.path.string())) { std::error_code ec3; fs::remove(e.path, ec3); } else { @@ -557,7 +561,7 @@ SwapManager::NotifyFilenameChanged(Buffer &buf) // file's swap), that file may be the one the caller is about to replay // for recovery: removing it here lost the recovery. if (!old_path.empty() && old_path != new_path) - (void) std::remove(old_path.c_str()); + remove_journal_unless_held(old_path, ctx.locked_out); ctx.gap = false; ctx.gap_chkpt_request_ns = 0; ctx.gap_unfixable_reported = false; @@ -621,10 +625,13 @@ SwapManager::ComputeSidecarPath(const Buffer &buf) return compute_swap_path_for_filename(buf.Filename()); } - // Unnamed buffers: unique within the process. + // Unnamed buffers: unique across processes (pid) and within one (counter). + // A per-process counter alone let a new session's scratch buffer claim, + // and later delete, another running session's unnamed journal. static std::atomic ctr{0}; const std::uint64_t n = ++ctr; - const std::string name = "unnamed-" + std::to_string(n) + ".swp"; + const std::string name = "unnamed-" + std::to_string(static_cast(::getpid())) + "-" + + std::to_string(n) + ".swp"; return (root / name).string(); } @@ -769,7 +776,12 @@ SwapManager::open_ctx(JournalCtx &ctx, const std::string &path, std::string &err } return false; } - (void) ::flock(fd, LOCK_EX | LOCK_NB); + if (::flock(fd, LOCK_EX | LOCK_NB) != 0 && errno == EWOULDBLOCK) { + kte::syscall::Close(fd); + ctx.locked_out = true; + err = "Swap file is in use by another kte process: " + path; + return false; + } (void) kte::syscall::Fchmod(fd, 0600); st.st_size = 0; } @@ -819,14 +831,10 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector= 0) { - (void) kte::syscall::Fsync(ctx.fd); - kte::syscall::Close(ctx.fd); - ctx.fd = -1; - } - ctx.header_ok = false; - + // Keep the current (locked) journal fd open until the compacted file has + // replaced it, so the journal is never unlocked in between: another + // session would otherwise see it as abandoned and offer to recover or + // discard it. const std::string tmp_path = ctx.path + ".tmp"; // Create the compacted file: header + checkpoint record. if (!ensure_parent_dir(tmp_path)) { @@ -864,8 +872,10 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector= 0) + kte::syscall::Close(ctx.fd); + const int fl = ::fcntl(tfd, F_GETFL); + if (fl >= 0) + (void) ::fcntl(tfd, F_SETFL, fl | O_APPEND); + ctx.fd = tfd; + ctx.header_ok = true; // Best-effort: fsync parent dir to persist the rename. try { @@ -902,10 +921,6 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector(chkpt_record.size()); return true; } @@ -1213,24 +1228,23 @@ SwapManager::writer_loop() // Throttled fsync: best-effort (grouped) try { - std::vector to_sync; std::uint64_t now = now_ns(); - { - std::lock_guard lg(mtx_); - for (auto &kv: journals_) { - JournalCtx &ctx = kv.second; - if (ctx.fd >= 0) { - if (ctx.last_fsync_ns == 0 || (now - ctx.last_fsync_ns) / 1000000ULL >= - cfg_.fsync_interval_ms) { - ctx.last_fsync_ns = now; - to_sync.push_back(ctx.fd); - } + // fsync under mtx_: the main thread closes journal fds under the + // same lock. Syncing outside it raced with those closes: the fd + // could be closed (or reused) mid-fsync, and the in-flight fsync + // kept the closed journal's flock alive, so the owner's own + // cleanup saw its journal as held by someone else. + std::lock_guard lg(mtx_); + for (auto &kv: journals_) { + JournalCtx &ctx = kv.second; + if (ctx.fd >= 0) { + if (ctx.last_fsync_ns == 0 || (now - ctx.last_fsync_ns) / 1000000ULL >= + cfg_.fsync_interval_ms) { + ctx.last_fsync_ns = now; + (void) kte::syscall::Fsync(ctx.fd); } } } - for (int fd: to_sync) { - (void) kte::syscall::Fsync(fd); - } } catch (const std::exception &e) { report_error(std::string("Exception in fsync operations: ") + e.what()); } catch (...) { @@ -1455,6 +1469,18 @@ parse_u32_le(const std::vector &p, std::size_t &off, std::uint32_t } +// Remove a journal this session is done with, unless another session holds +// it: a session that was locked out of the journal (or finds it locked) must +// not unlink the live journal of the session that owns it. +static void +remove_journal_unless_held(const std::string &path, bool locked_out) +{ + if (path.empty() || locked_out || SwapManager::JournalInUse(path)) + return; + (void) std::remove(path.c_str()); +} + + bool SwapManager::JournalInUse(const std::string &swap_path) { diff --git a/TerminalRenderer.cc b/TerminalRenderer.cc index 82b7461..f06657f 100644 --- a/TerminalRenderer.cc +++ b/TerminalRenderer.cc @@ -76,13 +76,9 @@ TerminalRenderer::Draw(Editor &ed) // could overflow the stack (RegexGuard.h). if (sline.size() <= kte::kRegexRenderLineLimit) try { std::regex rx(ed.SearchQuery()); - for (auto it = std::sregex_iterator(sline.begin(), sline.end(), rx); - it != std::sregex_iterator(); ++it) { - const auto &m = *it; - std::size_t sx = static_cast(m.position()); - std::size_t ex = sx + static_cast(m.length()); - ranges.emplace_back(sx, ex); - } + kte::ForEachRegexMatch(sline, rx, [&](std::size_t pos, std::size_t len) { + ranges.emplace_back(pos, pos + len); + }); } catch (const std::regex_error &) { // ignore invalid patterns here; status shows error } diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index d932782..cb7798d 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -8,6 +8,7 @@ #include "ErrorRecovery.h" #include "PieceTable.h" +#include "RegexGuard.h" #include "UndoTree.h" #include "syntax/HighlighterEngine.h" #include "syntax/HighlighterRegistry.h" @@ -880,3 +881,20 @@ TEST(Audit_Commands_FilesystemErrorsDoNotThrow) (void) ed.ProcessPendingOpens(); ASSERT_EQ(h.Text(), std::string("precious\n")); } + + +// Renderer regex highlighting on a long line with a deeply recursive pattern +// (overflowed an 8 MiB stack at under 10,000 bytes) runs off the main stack. +TEST(Audit_ForEachRegexMatch_LongLine) +{ + std::string line; + while (line.size() < 200000) + line += "word, more words; and so on. "; + std::size_t n = 0, total = 0; + kte::ForEachRegexMatch(line, std::regex("(\\w|\\s|[.,;])+"), [&](std::size_t, std::size_t len) { + ++n; + total += len; + }); + ASSERT_EQ(n, (std::size_t) 1); + ASSERT_EQ(total, line.size()); +} diff --git a/tests/test_swap_recovery_prompt.cc b/tests/test_swap_recovery_prompt.cc index 6f98194..41a55f4 100644 --- a/tests/test_swap_recovery_prompt.cc +++ b/tests/test_swap_recovery_prompt.cc @@ -583,3 +583,42 @@ TEST(SwapRecoveryPrompt_LiveJournalOfOtherSession_LeftAlone) other.SetSwapRecorder(nullptr); other_sm.Detach(&other, true); } + + +// A session locked out of a journal (another session owns it) must not +// delete it when it closes or saves the buffer. +TEST(SwapJournal_SecondSession_DoesNotDeleteOwnersJournal) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("owner"); + const std::string file = (sb.root / "work" / "g.txt").string(); + write_file_bytes(file, "base\n"); + + Editor a; + a.SetDimensions(24, 80); + std::string err; + ASSERT_TRUE(a.OpenFile(file, err)); + a.CurrentBuffer()->insert_text(0, 0, std::string("A")); + a.Swap()->Flush(a.CurrentBuffer()); + const std::string swp = kte::SwapManager::ComputeSwapPathForTests(*a.CurrentBuffer()); + ASSERT_TRUE(std::filesystem::exists(swp)); + + { + Editor b; + b.SetDimensions(24, 80); + b.AddBuffer(Buffer()); + b.RequestOpenFile(file); + (void) b.ProcessPendingOpens(); + b.CurrentBuffer()->insert_text(0, 0, std::string("B")); + b.Swap()->Flush(b.CurrentBuffer()); + ASSERT_TRUE(b.CloseBuffer(b.CurrentBufferIndex())); + } + ASSERT_TRUE(std::filesystem::exists(swp)); + + // A's journal still replays to A's content. + Buffer check; + std::string rerr; + ASSERT_TRUE(check.OpenFromFile(file, err)); + ASSERT_TRUE(kte::SwapManager::ReplayFile(check, swp, rerr)); + ASSERT_EQ(buffer_bytes_via_views(check), std::string("Abase\n")); +}