Editor: share the SwapManager along with shared buffers
A secondary GUI window shared the primary's buffer vector but attached buffers through its own SwapManager. Closing the window destroyed that manager while buffers still held its recorders, so the next edit called into freed memory; Rehome through the wrong manager also dropped journaling for the primary's buffers. Audit finding M3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
@@ -169,21 +169,21 @@ Editor::AddBuffer(const Buffer &buf)
|
|||||||
// Buffer to a new address. Drain any in-flight swap records first so the
|
// Buffer to a new address. Drain any in-flight swap records first so the
|
||||||
// writer thread never dereferences an address that's about to move, then
|
// writer thread never dereferences an address that's about to move, then
|
||||||
// rehome each attached buffer that actually moved.
|
// rehome each attached buffer that actually moved.
|
||||||
if (swap_ && !bufs.empty())
|
if (Swap() && !bufs.empty())
|
||||||
swap_->Flush();
|
Swap()->Flush();
|
||||||
std::vector<Buffer *> old_addrs;
|
std::vector<Buffer *> old_addrs;
|
||||||
old_addrs.reserve(bufs.size());
|
old_addrs.reserve(bufs.size());
|
||||||
for (auto &b: bufs)
|
for (auto &b: bufs)
|
||||||
old_addrs.push_back(&b);
|
old_addrs.push_back(&b);
|
||||||
bufs.push_back(buf);
|
bufs.push_back(buf);
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
for (std::size_t i = 0; i < old_addrs.size(); ++i) {
|
for (std::size_t i = 0; i < old_addrs.size(); ++i) {
|
||||||
Buffer *new_addr = &bufs[i];
|
Buffer *new_addr = &bufs[i];
|
||||||
if (new_addr != old_addrs[i])
|
if (new_addr != old_addrs[i])
|
||||||
bufs[i].SetSwapRecorder(swap_->Rehome(old_addrs[i], new_addr));
|
bufs[i].SetSwapRecorder(Swap()->Rehome(old_addrs[i], new_addr));
|
||||||
}
|
}
|
||||||
swap_->Attach(&bufs.back());
|
Swap()->Attach(&bufs.back());
|
||||||
bufs.back().SetSwapRecorder(swap_->RecorderFor(&bufs.back()));
|
bufs.back().SetSwapRecorder(Swap()->RecorderFor(&bufs.back()));
|
||||||
}
|
}
|
||||||
if (bufs.size() == 1) {
|
if (bufs.size() == 1) {
|
||||||
curbuf_ = 0;
|
curbuf_ = 0;
|
||||||
@@ -196,21 +196,21 @@ std::size_t
|
|||||||
Editor::AddBuffer(Buffer &&buf)
|
Editor::AddBuffer(Buffer &&buf)
|
||||||
{
|
{
|
||||||
auto &bufs = Buffers();
|
auto &bufs = Buffers();
|
||||||
if (swap_ && !bufs.empty())
|
if (Swap() && !bufs.empty())
|
||||||
swap_->Flush();
|
Swap()->Flush();
|
||||||
std::vector<Buffer *> old_addrs;
|
std::vector<Buffer *> old_addrs;
|
||||||
old_addrs.reserve(bufs.size());
|
old_addrs.reserve(bufs.size());
|
||||||
for (auto &b: bufs)
|
for (auto &b: bufs)
|
||||||
old_addrs.push_back(&b);
|
old_addrs.push_back(&b);
|
||||||
bufs.push_back(std::move(buf));
|
bufs.push_back(std::move(buf));
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
for (std::size_t i = 0; i < old_addrs.size(); ++i) {
|
for (std::size_t i = 0; i < old_addrs.size(); ++i) {
|
||||||
Buffer *new_addr = &bufs[i];
|
Buffer *new_addr = &bufs[i];
|
||||||
if (new_addr != old_addrs[i])
|
if (new_addr != old_addrs[i])
|
||||||
bufs[i].SetSwapRecorder(swap_->Rehome(old_addrs[i], new_addr));
|
bufs[i].SetSwapRecorder(Swap()->Rehome(old_addrs[i], new_addr));
|
||||||
}
|
}
|
||||||
swap_->Attach(&bufs.back());
|
Swap()->Attach(&bufs.back());
|
||||||
bufs.back().SetSwapRecorder(swap_->RecorderFor(&bufs.back()));
|
bufs.back().SetSwapRecorder(Swap()->RecorderFor(&bufs.back()));
|
||||||
}
|
}
|
||||||
if (bufs.size() == 1) {
|
if (bufs.size() == 1) {
|
||||||
curbuf_ = 0;
|
curbuf_ = 0;
|
||||||
@@ -237,10 +237,10 @@ Editor::OpenFile(const std::string &path, std::string &err)
|
|||||||
if (!ok)
|
if (!ok)
|
||||||
return false;
|
return false;
|
||||||
// Ensure swap recorder is attached for this buffer
|
// Ensure swap recorder is attached for this buffer
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
swap_->Attach(&cur);
|
Swap()->Attach(&cur);
|
||||||
cur.SetSwapRecorder(swap_->RecorderFor(&cur));
|
cur.SetSwapRecorder(Swap()->RecorderFor(&cur));
|
||||||
swap_->NotifyFilenameChanged(cur);
|
Swap()->NotifyFilenameChanged(cur);
|
||||||
}
|
}
|
||||||
// Setup highlighting using registry (extension + shebang)
|
// Setup highlighting using registry (extension + shebang)
|
||||||
cur.EnsureHighlighter();
|
cur.EnsureHighlighter();
|
||||||
@@ -298,8 +298,8 @@ Editor::OpenFile(const std::string &path, std::string &err)
|
|||||||
}
|
}
|
||||||
// Add as a new buffer and switch to it
|
// Add as a new buffer and switch to it
|
||||||
std::size_t idx = AddBuffer(std::move(b));
|
std::size_t idx = AddBuffer(std::move(b));
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
swap_->NotifyFilenameChanged(Buffers()[idx]);
|
Swap()->NotifyFilenameChanged(Buffers()[idx]);
|
||||||
}
|
}
|
||||||
SwitchTo(idx);
|
SwitchTo(idx);
|
||||||
// Defensive: ensure any active prompt is closed after a successful open
|
// Defensive: ensure any active prompt is closed after a successful open
|
||||||
@@ -514,26 +514,26 @@ Editor::CloseBuffer(std::size_t index)
|
|||||||
if (index >= bufs.size()) {
|
if (index >= bufs.size()) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
// Always remove swap file when closing a buffer on normal exit.
|
// Always remove swap file when closing a buffer on normal exit.
|
||||||
// Swap files are for crash recovery; on clean close, we don't need them.
|
// Swap files are for crash recovery; on clean close, we don't need them.
|
||||||
// This prevents stale swap files from accumulating (e.g., when used as git editor).
|
// This prevents stale swap files from accumulating (e.g., when used as git editor).
|
||||||
swap_->Detach(&bufs[index], true);
|
Swap()->Detach(&bufs[index], true);
|
||||||
bufs[index].SetSwapRecorder(nullptr);
|
bufs[index].SetSwapRecorder(nullptr);
|
||||||
// Drain in-flight records before the erase-shift below moves other
|
// Drain in-flight records before the erase-shift below moves other
|
||||||
// buffers to new addresses (vector::erase never reallocates, but it does
|
// buffers to new addresses (vector::erase never reallocates, but it does
|
||||||
// move-assign each trailing buffer into the previous slot).
|
// move-assign each trailing buffer into the previous slot).
|
||||||
swap_->Flush();
|
Swap()->Flush();
|
||||||
}
|
}
|
||||||
bufs.erase(bufs.begin() + static_cast<std::ptrdiff_t>(index));
|
bufs.erase(bufs.begin() + static_cast<std::ptrdiff_t>(index));
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
// erase() shifts every buffer after `index` down by one slot in-place
|
// erase() shifts every buffer after `index` down by one slot in-place
|
||||||
// (no reallocation), so the buffer now at slot i used to live at slot
|
// (no reallocation), so the buffer now at slot i used to live at slot
|
||||||
// i+1 (same underlying storage, since data() doesn't move on erase).
|
// i+1 (same underlying storage, since data() doesn't move on erase).
|
||||||
for (std::size_t i = index; i < bufs.size(); ++i) {
|
for (std::size_t i = index; i < bufs.size(); ++i) {
|
||||||
Buffer *new_addr = &bufs[i];
|
Buffer *new_addr = &bufs[i];
|
||||||
Buffer *old_addr = new_addr + 1;
|
Buffer *old_addr = new_addr + 1;
|
||||||
bufs[i].SetSwapRecorder(swap_->Rehome(old_addr, new_addr));
|
bufs[i].SetSwapRecorder(Swap()->Rehome(old_addr, new_addr));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (bufs.empty()) {
|
if (bufs.empty()) {
|
||||||
@@ -564,9 +564,9 @@ Editor::Reset()
|
|||||||
close_confirm_pending_ = false;
|
close_confirm_pending_ = false;
|
||||||
close_after_save_ = false;
|
close_after_save_ = false;
|
||||||
auto &bufs = Buffers();
|
auto &bufs = Buffers();
|
||||||
if (swap_) {
|
if (Swap()) {
|
||||||
for (auto &buf : bufs)
|
for (auto &buf : bufs)
|
||||||
swap_->Detach(&buf, true);
|
Swap()->Detach(&buf, true);
|
||||||
}
|
}
|
||||||
bufs.clear();
|
bufs.clear();
|
||||||
curbuf_ = 0;
|
curbuf_ = 0;
|
||||||
|
|||||||
@@ -607,9 +607,13 @@ public:
|
|||||||
|
|
||||||
// Share another editor's buffer list. When set, this editor operates on
|
// Share another editor's buffer list. When set, this editor operates on
|
||||||
// the provided vector instead of its own. Pass nullptr to detach.
|
// the provided vector instead of its own. Pass nullptr to detach.
|
||||||
void SetSharedBuffers(std::vector<Buffer> *shared)
|
// Buffers hold recorders owned by the SwapManager that attached them, so
|
||||||
|
// editors sharing buffers must also share that manager (shared_swap);
|
||||||
|
// otherwise closing one window destroys recorders the other still uses.
|
||||||
|
void SetSharedBuffers(std::vector<Buffer> *shared, kte::SwapManager *shared_swap = nullptr)
|
||||||
{
|
{
|
||||||
shared_buffers_ = shared;
|
shared_buffers_ = shared;
|
||||||
|
shared_swap_ = shared ? shared_swap : nullptr;
|
||||||
curbuf_ = 0;
|
curbuf_ = 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -617,7 +621,7 @@ public:
|
|||||||
// Swap manager access (for advanced integrations/tests)
|
// Swap manager access (for advanced integrations/tests)
|
||||||
[[nodiscard]] kte::SwapManager *Swap()
|
[[nodiscard]] kte::SwapManager *Swap()
|
||||||
{
|
{
|
||||||
return swap_.get();
|
return shared_swap_ ? shared_swap_ : swap_.get();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@@ -667,6 +671,7 @@ private:
|
|||||||
|
|
||||||
// Swap journaling manager (lifetime = editor)
|
// Swap journaling manager (lifetime = editor)
|
||||||
std::unique_ptr<kte::SwapManager> swap_;
|
std::unique_ptr<kte::SwapManager> swap_;
|
||||||
|
kte::SwapManager *shared_swap_ = nullptr; // set with shared_buffers_; not owned
|
||||||
|
|
||||||
// Kill ring (Emacs-like)
|
// Kill ring (Emacs-like)
|
||||||
std::vector<std::string> kill_ring_;
|
std::vector<std::string> kill_ring_;
|
||||||
|
|||||||
+1
-1
@@ -378,7 +378,7 @@ GUIFrontend::OpenNewWindow_(Editor &primary)
|
|||||||
ws->height = h;
|
ws->height = h;
|
||||||
|
|
||||||
// Secondary editor shares the primary's buffer list
|
// Secondary editor shares the primary's buffer list
|
||||||
ws->editor.SetSharedBuffers(&primary.Buffers());
|
ws->editor.SetSharedBuffers(&primary.Buffers(), primary.Swap());
|
||||||
ws->editor.SetDimensions(primary.Rows(), primary.Cols());
|
ws->editor.SetDimensions(primary.Rows(), primary.Cols());
|
||||||
|
|
||||||
// Open a new untitled buffer and switch to it in the new window.
|
// Open a new untitled buffer and switch to it in the new window.
|
||||||
|
|||||||
@@ -11,6 +11,7 @@
|
|||||||
#include "UndoTree.h"
|
#include "UndoTree.h"
|
||||||
|
|
||||||
#include <chrono>
|
#include <chrono>
|
||||||
|
#include <memory>
|
||||||
#include <string>
|
#include <string>
|
||||||
#include <thread>
|
#include <thread>
|
||||||
|
|
||||||
@@ -225,3 +226,25 @@ TEST(Audit_D8_CircuitBreaker_WindowExpires)
|
|||||||
ASSERT_TRUE(cb.GetState() == kte::CircuitBreaker::State::Closed);
|
ASSERT_TRUE(cb.GetState() == kte::CircuitBreaker::State::Closed);
|
||||||
ASSERT_EQ(cb.GetFailureCount(), (std::size_t) 1);
|
ASSERT_EQ(cb.GetFailureCount(), (std::size_t) 1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
// M3: an editor that shares another's buffers must attach them through the
|
||||||
|
// owner's SwapManager, so destroying it leaves no dangling recorders.
|
||||||
|
TEST(Audit_M3_SharedBuffers_UseOwnersSwapManager)
|
||||||
|
{
|
||||||
|
TestHarness h;
|
||||||
|
Editor &primary = h.EditorRef();
|
||||||
|
|
||||||
|
auto secondary = std::make_unique<Editor>();
|
||||||
|
secondary->SetSharedBuffers(&primary.Buffers(), primary.Swap());
|
||||||
|
ASSERT_TRUE(secondary->Swap() == primary.Swap());
|
||||||
|
secondary->AddBuffer(Buffer());
|
||||||
|
const std::size_t idx = primary.Buffers().size() - 1;
|
||||||
|
secondary.reset();
|
||||||
|
|
||||||
|
// Edits record through the buffer's swap recorder; before the fix it
|
||||||
|
// pointed into the destroyed secondary SwapManager.
|
||||||
|
Buffer &b = primary.Buffers()[idx];
|
||||||
|
b.insert_text(0, 0, "abc");
|
||||||
|
ASSERT_EQ(b.GetLineString(0), std::string("abc"));
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user