terminal: avoid races between close and draw

Fix races where a goroutine calls Terminal.Draw and another one calls
Terminal.Close or Terminal.Destroy. The closing thread will eventually
set term.vterm to nil just before the drawing thread calls
term.vterm.Draw(), causing this crash:

Error: runtime error: invalid memory address or nil pointer dereference

goroutine 1 [running]:
panic({0xb09140, 0x10b5860})
        runtime/panic.go:890 +0x263
git.sr.ht/~rockorager/tcell-term.(*VT).Draw(0x0)
        git.sr.ht/~rockorager/tcell-term@v0.8.0/vt.go:424 +0x50
git.sr.ht/~rjarry/aerc/widgets.(*Terminal).draw(0xc001c658b0)
        git.sr.ht/~rjarry/aerc/widgets/terminal.go:116 +0x29
git.sr.ht/~rjarry/aerc/widgets.(*Terminal).Draw(0xc001c658b0, 0xc002b08150)
        git.sr.ht/~rjarry/aerc/widgets/terminal.go:108 +0x1b4
git.sr.ht/~rjarry/aerc/lib/ui.(*Grid).Draw(0xc001d0c360, 0xc0008ddb30)
        git.sr.ht/~rjarry/aerc/lib/ui/grid.go:126 +0x225
git.sr.ht/~rjarry/aerc/widgets.(*Composer).Draw(0xc001c13180, 0xc0008ddb30)
        git.sr.ht/~rjarry/aerc/widgets/compose.go:747 +0x8f
git.sr.ht/~rjarry/aerc/lib/ui.(*TabContent).Draw(0xc0003cc5b0, 0xc0008ddb30)
        git.sr.ht/~rjarry/aerc/lib/ui/tab.go:468 +0x1f4
git.sr.ht/~rjarry/aerc/lib/ui.(*Grid).Draw(0xc0001b2900, 0xc000037050)
        git.sr.ht/~rjarry/aerc/lib/ui/grid.go:126 +0x225
git.sr.ht/~rjarry/aerc/widgets.(*Aerc).Draw(0xc000000180, 0xc000037050)
        git.sr.ht/~rjarry/aerc/widgets/aerc.go:193 +0x209
git.sr.ht/~rjarry/aerc/lib/ui.(*UI).Render(0xc000414040)
        git.sr.ht/~rjarry/aerc/lib/ui/ui.go:105 +0x62
main.main()
        git.sr.ht/~rjarry/aerc/main.go:279 +0xbac

Use an atomic to determine if the terminal is closed or not. Never set
vterm to nil (it is not necessary).

Signed-off-by: Robin Jarry <robin@jarry.cc>
Tested-by: Koni Marti <koni.marti@gmail.com>
Reviewed-by: Tim Culverhouse <tim@timculverhouse.com>

1 files changed, 28 insertions, 27 deletions

diff --git a/widgets/terminal.go b/widgets/terminal.go
index cf0888ee..47bf0e7a 100644
--- a/widgets/terminal.go
+++ b/widgets/terminal.go
@@ -2,6 +2,7 @@ package widgets
 
 import (
 	"os/exec"
+	"sync/atomic"
 
 	"git.sr.ht/~rjarry/aerc/config"
 	"git.sr.ht/~rjarry/aerc/lib/ui"
@@ -12,14 +13,13 @@ import (
 )
 
 type Terminal struct {
-	closed    bool
-	cmd       *exec.Cmd
-	ctx       *ui.Context
-	destroyed bool
-	focus     bool
-	visible   bool
-	vterm     *tcellterm.VT
-	running   bool
+	closed  int32
+	cmd     *exec.Cmd
+	ctx     *ui.Context
+	focus   bool
+	visible bool
+	vterm   *tcellterm.VT
+	running bool
 
 	OnClose func(err error)
 	OnEvent func(event tcell.Event) bool
@@ -42,8 +42,15 @@ func (term *Terminal) Close() {
 	term.closeErr(nil)
 }
 
+// TODO: replace with atomic.Bool when min go version will have it (1.19+)
+const closed int32 = 1
+
+func (term *Terminal) isClosed() bool {
+	return atomic.LoadInt32(&term.closed) == closed
+}
+
 func (term *Terminal) closeErr(err error) {
-	if term.closed {
+	if atomic.SwapInt32(&term.closed, closed) == closed {
 		return
 	}
 	if term.vterm != nil && err == nil {
@@ -51,28 +58,19 @@ func (term *Terminal) closeErr(err error) {
 		term.vterm.Detach()
 		term.vterm.Close()
 	}
-	if !term.closed && term.OnClose != nil {
+	if term.OnClose != nil {
 		term.OnClose(err)
 	}
 	if term.ctx != nil {
 		term.ctx.HideCursor()
 	}
-	term.closed = true
 	ui.Invalidate()
 }
 
 func (term *Terminal) Destroy() {
-	if term.destroyed {
-		return
-	}
-	if term.ctx != nil {
-		term.ctx.HideCursor()
-	}
 	// If we destroy, we don't want to call the OnClose callback
 	term.OnClose = nil
-	term.Close()
-	term.vterm = nil
-	term.destroyed = true
+	term.closeErr(nil)
 }
 
 func (term *Terminal) Invalidate() {
@@ -80,7 +78,7 @@ func (term *Terminal) Invalidate() {
 }
 
 func (term *Terminal) Draw(ctx *ui.Context) {
-	if term.destroyed {
+	if term.isClosed() {
 		return
 	}
 	term.vterm.SetSurface(ctx.View())
@@ -93,7 +91,7 @@ func (term *Terminal) Draw(ctx *ui.Context) {
 		}
 	}
 	term.ctx = ctx
-	if !term.running && !term.closed && term.cmd != nil {
+	if !term.running && term.cmd != nil {
 		term.vterm.Attach(term.HandleEvent)
 		if err := term.vterm.Start(term.cmd); err != nil {
 			log.Errorf("error running terminal: %v", err)
@@ -113,8 +111,11 @@ func (term *Terminal) Show(visible bool) {
 }
 
 func (term *Terminal) draw() {
+	if term.isClosed() {
+		return
+	}
 	term.vterm.Draw()
-	if term.focus && !term.closed && term.ctx != nil {
+	if term.focus && term.ctx != nil {
 		y, x, style, vis := term.vterm.Cursor()
 		if vis {
 			term.ctx.SetCursor(x, y)
@@ -133,7 +134,7 @@ func (term *Terminal) MouseEvent(localX int, localY int, event tcell.Event) {
 	if term.OnEvent != nil {
 		term.OnEvent(ev)
 	}
-	if term.closed {
+	if term.isClosed() {
 		return
 	}
 	e := tcell.NewEventMouse(localX, localY, ev.Buttons(), ev.Modifiers())
@@ -141,7 +142,7 @@ func (term *Terminal) MouseEvent(localX int, localY int, event tcell.Event) {
 }
 
 func (term *Terminal) Focus(focus bool) {
-	if term.closed {
+	if term.isClosed() {
 		return
 	}
 	term.focus = focus
@@ -159,7 +160,7 @@ func (term *Terminal) Focus(focus bool) {
 
 // HandleEvent is used to watch the underlying terminal events
 func (term *Terminal) HandleEvent(ev tcell.Event) {
-	if term.closed || term.destroyed {
+	if term.isClosed() {
 		return
 	}
 	switch ev := ev.(type) {
@@ -183,7 +184,7 @@ func (term *Terminal) Event(event tcell.Event) bool {
 			return true
 		}
 	}
-	if term.closed {
+	if term.isClosed() {
 		return false
 	}
 	return term.vterm.HandleEvent(event)