QuitAll crashes micro

[Neko-Box-Coder]

Description

When you have a file open, performing QuitAll crashes micro with a segfault.

Micro encountered an error: runtime.errorString runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:336 (0x48b8d8)
runtime/panic.go:335 (0x48b8a6)
github.com/micro-editor/micro/v2/internal/buffer/buffer.go:691 (0x883dd4)
github.com/micro-editor/micro/v2/internal/buffer/buffer.go:690 (0x883daf)
github.com/micro-editor/micro/v2/internal/buffer/buffer.go:506 (0x8e6f78)
github.com/micro-editor/micro/v2/internal/buffer/buffer.go:504 (0x8e6f33)
github.com/micro-editor/micro/v2/internal/action/actions.go:1969 (0x8cc4b7)
github.com/micro-editor/micro/v2/internal/action/bufpane.go:567 (0x8d3ecd)
github.com/micro-editor/micro/v2/internal/action/tab.go:224 (0x8d22a6)
github.com/micro-editor/micro/v2/internal/action/bufpane.go:183 (0x8d20d9)
github.com/micro-editor/micro/v2/internal/action/bufpane.go:33 (0x8d2071)
github.com/micro-editor/micro/v2/internal/action/bufpane.go:546 (0x8d3c39)
github.com/micro-editor/micro/v2/internal/action/bufpane.go:464 (0x8d3729)
github.com/micro-editor/micro/v2/internal/action/tab.go:337 (0x8e5105)
github.com/micro-editor/micro/v2/internal/action/tab.go:141 (0x8e4245)
github.com/micro-editor/micro/v2/cmd/micro/micro.go:543 (0x90f72a)
github.com/micro-editor/micro/v2/cmd/micro/micro.go:482 (0x90f27b)
internal/runtime/atomic/types.go:194 (0x453c75)
runtime/asm_amd64.s:1771 (0x490f01)

If you can reproduce this error, please report it at https://github.com/micro-editor/micro/issues
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x90e61f]

Doing some quick bisecting reveals that the commit ef57bdc from #3997 had introduced this regression.

Adding QuitAll to a keybind and running that on any opened file will trigger this behavior.

@JoeKar

Environment

• Version: master
• OS: Linux
• Terminal: WezTerm

[JoeKar]

Doing some quick bisecting reveals that the commit ef57bdc from #3997 had introduced this regression.

I'd say it was uncovered by ef57bdc. 😜
But thank you already for bisecting this issue. 👍

[molloyzak13]

reply_target: #4131

traced the nil dereference. the crash chain is:

CloseOpenBuffers (buffer.go:504) iterates OpenBuffers calling buf.Fini() on each, then OpenBuffers[i] = nil. Fini calls b.Shared() (buffer.go:528). Shared (buffer.go:690) ranges over OpenBuffers and reads buf.SharedBuffer for every entry, including the ones already nilled out by earlier iterations.

so on the second buffer: OpenBuffers[0] is nil, Shared loops over it, buf is nil, buf.SharedBuffer panics.

quickest fix is adding a nil guard in Shared before the field access. alternative is clearing OpenBuffers after the loop instead of inside it, which is cleaner since Shared only needs to know if another live buffer shares the backing data.

[JoeKar]

[...] alternative is clearing OpenBuffers after the loop instead of inside it, which is cleaner since Shared only needs to know if another live buffer shares the backing data.

From my perspective it is correct to invalidate the buffer, since it isn't available after Fini() any longer. Currently it only affects the iteration of open buffers in Shared(), but it might affect more in the future and all of them should care about the nil.

[dmaluka]

What's wrong with an obvious fix like this:

diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go
index d1f8db4b..b5f5c6ee 100644
--- a/internal/buffer/buffer.go
+++ b/internal/buffer/buffer.go
@@ -502,11 +502,9 @@ func NewBuffer(r io.Reader, size int64, path string, btype BufType, cmd Command)
 
 // CloseOpenBuffers removes all open buffers
 func CloseOpenBuffers() {
-	for i, buf := range OpenBuffers {
-		buf.Fini()
-		OpenBuffers[i] = nil
+	for _, buf := range OpenBuffers {
+		buf.Close()
 	}
-	OpenBuffers = OpenBuffers[:0]
 }
 
 // Close removes this buffer from the list of open buffers

to properly remove the closed buffer from OpenBuffers on each iteration (instead of leaving it in OpenBuffers in an invalid state and working around problems causes by that)?

Well, actually it already used to work this way, until it was changed #2898 which apparently was needed to fix the issue #2892.

But it is not clear to me how exactly it fixed #2892. The description in #2898 doesn't seem to clearly explain that. @JoeKar "...which is modified directly in case Close() is called for the related buffer" - what does this mean exactly? What "related buffer"? Are you sure #2898 actually fixed #2892? What was the root cause of #2892?