From 006e1ddb455608047979ac105d46b88d403b12e7 Mon Sep 17 00:00:00 2001 From: CJ van den Berg Date: Tue, 3 Jun 2025 18:14:00 +0200 Subject: [PATCH] fix: remove multithreaded buffer access in project_manager Until we have proper multithreaded buffer lifetime management we should avoid accessing buffers that may have been deleted already. --- src/Project.zig | 29 +++++++++++------------------ src/project_manager.zig | 22 ++++++++++++++-------- src/tui/editor.zig | 22 +++++++++++++++------- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/Project.zig b/src/Project.zig index 682fbcc..489c328 100644 --- a/src/Project.zig +++ b/src/Project.zig @@ -520,6 +520,7 @@ pub fn delete_task(self: *Self, command: []const u8) error{}!void { } pub fn did_open(self: *Self, file_path: []const u8, file_type: []const u8, language_server: []const u8, version: usize, text: []const u8) StartLspError!void { + defer std.heap.c_allocator.free(text); self.update_mru(file_path, 0, 0) catch {}; const lsp = try self.get_or_start_language_server(file_path, language_server); const uri = try self.make_URI(file_path); @@ -529,7 +530,10 @@ pub fn did_open(self: *Self, file_path: []const u8, file_type: []const u8, langu }) catch return error.LspFailed; } -pub fn did_change(self: *Self, file_path: []const u8, version: usize, root_dst_addr: usize, root_src_addr: usize, eol_mode: Buffer.EolMode) LspError!void { +pub fn did_change(self: *Self, file_path: []const u8, version: usize, text_dst: []const u8, text_src: []const u8, eol_mode: Buffer.EolMode) LspError!void { + _ = eol_mode; + defer std.heap.c_allocator.free(text_dst); + defer std.heap.c_allocator.free(text_src); const lsp = try self.get_language_server(file_path); const uri = try self.make_URI(file_path); @@ -545,22 +549,11 @@ pub fn did_change(self: *Self, file_path: []const u8, version: usize, root_dst_a self.allocator.free(scratch); } - const root_dst: Buffer.Root = if (root_dst_addr == 0) return else @ptrFromInt(root_dst_addr); - const root_src: Buffer.Root = if (root_src_addr == 0) return else @ptrFromInt(root_src_addr); - var dizzy_edits = std.ArrayListUnmanaged(dizzy.Edit){}; - var dst = std.ArrayList(u8).init(arena); - var src = std.ArrayList(u8).init(arena); var edits_cb = std.ArrayList(u8).init(arena); const writer = edits_cb.writer(); - { - const frame = tracy.initZone(@src(), .{ .name = "store" }); - defer frame.deinit(); - try root_dst.store(dst.writer(), eol_mode); - try root_src.store(src.writer(), eol_mode); - } - const scratch_len = 4 * (dst.items.len + src.items.len) + 2; + const scratch_len = 4 * (text_dst.len + text_src.len) + 2; const scratch = blk: { const frame = tracy.initZone(@src(), .{ .name = "scratch" }); defer frame.deinit(); @@ -571,7 +564,7 @@ pub fn did_change(self: *Self, file_path: []const u8, version: usize, root_dst_a { const frame = tracy.initZone(@src(), .{ .name = "diff" }); defer frame.deinit(); - try dizzy.PrimitiveSliceDiffer(u8).diff(arena, &dizzy_edits, src.items, dst.items, scratch); + try dizzy.PrimitiveSliceDiffer(u8).diff(arena, &dizzy_edits, text_src, text_dst, scratch); } var lines_dst: usize = 0; var last_offset: usize = 0; @@ -583,7 +576,7 @@ pub fn did_change(self: *Self, file_path: []const u8, version: usize, root_dst_a for (dizzy_edits.items) |dizzy_edit| { switch (dizzy_edit.kind) { .equal => { - scan_char(src.items[dizzy_edit.range.start..dizzy_edit.range.end], &lines_dst, '\n', &last_offset); + scan_char(text_src[dizzy_edit.range.start..dizzy_edit.range.end], &lines_dst, '\n', &last_offset); }, .insert => { const line_start_dst: usize = lines_dst; @@ -592,15 +585,15 @@ pub fn did_change(self: *Self, file_path: []const u8, version: usize, root_dst_a .start = .{ .line = line_start_dst, .character = last_offset }, .end = .{ .line = line_start_dst, .character = last_offset }, }, - .text = dst.items[dizzy_edit.range.start..dizzy_edit.range.end], + .text = text_dst[dizzy_edit.range.start..dizzy_edit.range.end], }); edits_count += 1; - scan_char(dst.items[dizzy_edit.range.start..dizzy_edit.range.end], &lines_dst, '\n', &last_offset); + scan_char(text_dst[dizzy_edit.range.start..dizzy_edit.range.end], &lines_dst, '\n', &last_offset); }, .delete => { var line_end_dst: usize = lines_dst; var offset_end_dst: usize = last_offset; - scan_char(src.items[dizzy_edit.range.start..dizzy_edit.range.end], &line_end_dst, '\n', &offset_end_dst); + scan_char(text_src[dizzy_edit.range.start..dizzy_edit.range.end], &line_end_dst, '\n', &offset_end_dst); try cbor.writeValue(writer, .{ .range = .{ .start = .{ .line = lines_dst, .character = last_offset }, diff --git a/src/project_manager.zig b/src/project_manager.zig index 972e110..b7937db 100644 --- a/src/project_manager.zig +++ b/src/project_manager.zig @@ -147,11 +147,13 @@ pub fn did_open(file_path: []const u8, file_type: *const FileType, version: usiz return send(.{ "did_open", project, file_path, file_type.name, language_server, version, text_ptr, text.len }); } -pub fn did_change(file_path: []const u8, version: usize, root_dst: usize, root_src: usize, eol_mode: Buffer.EolMode) (ProjectManagerError || ProjectError)!void { +pub fn did_change(file_path: []const u8, version: usize, text_dst: []const u8, text_src: []const u8, eol_mode: Buffer.EolMode) (ProjectManagerError || ProjectError)!void { const project = tp.env.get().str("project"); if (project.len == 0) return error.NoProject; - return send(.{ "did_change", project, file_path, version, root_dst, root_src, @intFromEnum(eol_mode) }); + const text_dst_ptr: usize = if (text_dst.len > 0) @intFromPtr(text_dst.ptr) else 0; + const text_src_ptr: usize = if (text_src.len > 0) @intFromPtr(text_src.ptr) else 0; + return send(.{ "did_change", project, file_path, version, text_dst_ptr, text_dst.len, text_src_ptr, text_src.len, @intFromEnum(eol_mode) }); } pub fn did_save(file_path: []const u8) (ProjectManagerError || ProjectError)!void { @@ -322,12 +324,14 @@ const Process = struct { var version: usize = 0; var text_ptr: usize = 0; var text_len: usize = 0; + var text_dst_ptr: usize = 0; + var text_dst_len: usize = 0; + var text_src_ptr: usize = 0; + var text_src_len: usize = 0; var n: usize = 0; var task: []const u8 = undefined; var context: usize = undefined; - var root_dst: usize = 0; - var root_src: usize = 0; var eol_mode: Buffer.EolModeTag = @intFromEnum(Buffer.EolMode.lf); if (try cbor.match(m.buf, .{ "walk_tree_entry", tp.extract(&project_directory), tp.extract(&path), tp.extract(&high), tp.extract(&low) })) { @@ -373,8 +377,10 @@ const Process = struct { } else if (try cbor.match(m.buf, .{ "did_open", tp.extract(&project_directory), tp.extract(&path), tp.extract(&file_type), tp.extract_cbor(&language_server), tp.extract(&version), tp.extract(&text_ptr), tp.extract(&text_len) })) { const text = if (text_len > 0) @as([*]const u8, @ptrFromInt(text_ptr))[0..text_len] else ""; self.did_open(project_directory, path, file_type, language_server, version, text) catch |e| return from.forward_error(e, @errorReturnTrace()) catch error.ClientFailed; - } else if (try cbor.match(m.buf, .{ "did_change", tp.extract(&project_directory), tp.extract(&path), tp.extract(&version), tp.extract(&root_dst), tp.extract(&root_src), tp.extract(&eol_mode) })) { - self.did_change(project_directory, path, version, root_dst, root_src, @enumFromInt(eol_mode)) catch |e| return from.forward_error(e, @errorReturnTrace()) catch error.ClientFailed; + } else if (try cbor.match(m.buf, .{ "did_change", tp.extract(&project_directory), tp.extract(&path), tp.extract(&version), tp.extract(&text_dst_ptr), tp.extract(&text_dst_len), tp.extract(&text_src_ptr), tp.extract(&text_src_len), tp.extract(&eol_mode) })) { + const text_dst = if (text_dst_len > 0) @as([*]const u8, @ptrFromInt(text_dst_ptr))[0..text_dst_len] else ""; + const text_src = if (text_src_len > 0) @as([*]const u8, @ptrFromInt(text_src_ptr))[0..text_src_len] else ""; + self.did_change(project_directory, path, version, text_dst, text_src, @enumFromInt(eol_mode)) catch |e| return from.forward_error(e, @errorReturnTrace()) catch error.ClientFailed; } else if (try cbor.match(m.buf, .{ "did_save", tp.extract(&project_directory), tp.extract(&path) })) { self.did_save(project_directory, path) catch |e| return from.forward_error(e, @errorReturnTrace()) catch error.ClientFailed; } else if (try cbor.match(m.buf, .{ "did_close", tp.extract(&project_directory), tp.extract(&path) })) { @@ -497,11 +503,11 @@ const Process = struct { return project.did_open(file_path, file_type, language_server, version, text); } - fn did_change(self: *Process, project_directory: []const u8, file_path: []const u8, version: usize, root_dst: usize, root_src: usize, eol_mode: Buffer.EolMode) (ProjectError || Project.LspError)!void { + fn did_change(self: *Process, project_directory: []const u8, file_path: []const u8, version: usize, text_dst: []const u8, text_src: []const u8, eol_mode: Buffer.EolMode) (ProjectError || Project.LspError)!void { const frame = tracy.initZone(@src(), .{ .name = module_name ++ ".did_change" }); defer frame.deinit(); const project = self.projects.get(project_directory) orelse return error.NoProject; - return project.did_change(file_path, version, root_dst, root_src, eol_mode); + return project.did_change(file_path, version, text_dst, text_src, eol_mode); } fn did_save(self: *Process, project_directory: []const u8, file_path: []const u8) (ProjectError || Project.LspError)!void { diff --git a/src/tui/editor.zig b/src/tui/editor.zig index fc3c5fa..134d938 100644 --- a/src/tui/editor.zig +++ b/src/tui/editor.zig @@ -571,11 +571,11 @@ pub const Editor = struct { self.syntax = syntax: { const lang_override = file_type orelse tp.env.get().str("language"); var content = std.ArrayListUnmanaged(u8).empty; - defer content.deinit(self.allocator); + defer content.deinit(std.heap.c_allocator); { const frame_ = tracy.initZone(@src(), .{ .name = "store" }); defer frame_.deinit(); - try new_buf.root.store(content.writer(self.allocator), new_buf.file_eol_mode); + try new_buf.root.store(content.writer(std.heap.c_allocator), new_buf.file_eol_mode); } const syn_file_type = blk: { @@ -603,7 +603,7 @@ pub const Editor = struct { file_path, syn_.file_type, self.lsp_version, - try content.toOwnedSlice(self.allocator), + try content.toOwnedSlice(std.heap.c_allocator), new_buf.is_ephemeral(), ) catch |e| self.logger.print("project_manager.did_open failed: {any}", .{e}); @@ -1669,10 +1669,18 @@ pub const Editor = struct { return if (p) |p_| @intFromPtr(p_) else 0; } + fn text_from_root(root_: ?Buffer.Root, eol_mode: Buffer.EolMode) ![]const u8 { + const root = root_ orelse return &.{}; + var text = std.ArrayList(u8).init(std.heap.c_allocator); + defer text.deinit(); + try root.store(text.writer(), eol_mode); + return text.toOwnedSlice(); + } + fn send_editor_update(self: *const Self, old_root: ?Buffer.Root, new_root: ?Buffer.Root, eol_mode: Buffer.EolMode) !void { _ = try self.handlers.msg(.{ "E", "update", token_from(new_root), token_from(old_root), @intFromEnum(eol_mode) }); if (self.syntax) |_| if (self.file_path) |file_path| if (old_root != null and new_root != null) - project_manager.did_change(file_path, self.lsp_version, token_from(new_root), token_from(old_root), eol_mode) catch {}; + project_manager.did_change(file_path, self.lsp_version, try text_from_root(new_root, eol_mode), try text_from_root(old_root, eol_mode), eol_mode) catch {}; } fn send_editor_eol_mode(self: *const Self, eol_mode: Buffer.EolMode, utf8_sanitized: bool) !void { @@ -5809,16 +5817,16 @@ pub const Editor = struct { self.syntax = syntax: { var content = std.ArrayListUnmanaged(u8).empty; - defer content.deinit(self.allocator); + defer content.deinit(std.heap.c_allocator); const root = try self.buf_root(); - try root.store(content.writer(self.allocator), try self.buf_eol_mode()); + try root.store(content.writer(std.heap.c_allocator), try self.buf_eol_mode()); const syn = syntax.create_file_type(self.allocator, file_type, tui.query_cache()) catch null; if (syn) |syn_| if (self.file_path) |file_path| project_manager.did_open( file_path, syn_.file_type, self.lsp_version, - try content.toOwnedSlice(self.allocator), + try content.toOwnedSlice(std.heap.c_allocator), if (self.buffer) |p| p.is_ephemeral() else true, ) catch |e| self.logger.print("project_manager.did_open failed: {any}", .{e});