Add closeMutex to avoid crash v1 PROPOSED

y0ast: 1
 Add closeMutex to avoid crash

 1 files changed, 7 insertions(+), 2 deletions(-)

[PATCH] Add closeMutex to avoid crash Export this patch

Currently the terminal widget can be closed while it's also being
updated leading to a SIGSEGV: https://todo.sr.ht/~sircmpwn/aerc2/449.

This patch adds the minimal amount of locking to make that impossible.
---
 widgets/terminal.go | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/widgets/terminal.go b/widgets/terminal.go
index 0277521..8618b1e 100644
--- a/widgets/terminal.go
+++ b/widgets/terminal.go
@@ -9,7 +9,7 @@ import (
 	"git.sr.ht/~sircmpwn/aerc/lib/ui"
 
 	"github.com/creack/pty"
-	"github.com/ddevault/go-libvterm"
+	vterm "github.com/ddevault/go-libvterm"
 	"github.com/gdamore/tcell/v2"
 )
 
@@ -105,6 +105,7 @@ type Terminal struct {
 	damage      []vterm.Rect // protected by damageMutex
 	damageMutex sync.Mutex
 	writeMutex  sync.Mutex
+	closeMutex  sync.Mutex // Only necessary around cmd/pty changes

Reto

6 days ago

closeMutex is a strange name if you then describe it as "cmd/pty changes lock"

Next

 
 	OnClose func(err error)
 	OnEvent func(event tcell.Event) bool
@@ -171,6 +172,7 @@ func (term *Terminal) flushTerminal() {
 }
 
 func (term *Terminal) Close(err error) {
+	term.closeMutex.Lock()
 	if term.closed {
 		return

Reto

6 days ago

If you reach this the lock never gets released

Next

 	}
@@ -184,10 +186,11 @@ func (term *Terminal) Close(err error) {
 		term.cmd.Wait()
 		term.cmd = nil
 	}
-	if !term.closed && term.OnClose != nil {
+	if term.OnClose != nil {
 		term.OnClose(err)
 	}
 	term.closed = true
+	term.closeMutex.Unlock()
 	term.ctx.HideCursor()
 }
 
@@ -227,6 +230,7 @@ func (term *Terminal) Draw(ctx *ui.Context) {

Reto

6 days ago

(Here we need the full function, pasted it below)

Next

 
 	term.ctx = ctx // gross
 
+	term.closeMutex.Lock()
 	if !term.closed {
 		winsize := pty.Winsize{
 			Cols: uint16(ctx.Width()),
@@ -266,6 +270,7 @@ func (term *Terminal) Draw(ctx *ui.Context) {
 			return

Reto

6 days ago

If you reach this the lock never gets released

Reto

6 days ago

If you reach this the lock never gets released

Reto

6 days ago

If you reach this the lock never gets released

Reto

6 days ago

If you reach this the lock never gets released
> }
> }
> term.closeMutex.Unlock()
PS: Can't we force all close events to go over the UI thread?
Why do we even have asynchronous close events for the terminal?
If we could synchronize this it may be much simpler to understand / maintain.

And yes, that's just an idea I'm throwing into the void, I didn't look at the
code all to closely.

Cheers,
Reto

 		}
 	}
+	term.closeMutex.Unlock()
 
 	screen := term.vterm.ObtainScreen()
 
-- 
2.24.3 (Apple Git-128)

y0ast

15 days ago

Upon further testing, this patch sadly leads to a deadlock, will investigate and put up a v2.

Reto

6 days ago

Hi Jozzle,

Are you sure this isn't deadlocking?
Because it should, based on how you wrote it ;)