viewer: avoid crashes on opening invalid messages

When an error occurs during the opening of a message because its contents cannot be parsed, the PartSwitcher object is left to nil and the err field is set to the reported error. This defers the error reporting after the viewer tab is displayed but it is not handled in all sub functions which assume that switcher cannot be nil. Error: runtime error: invalid memory address or nil pointer dereference git.sr.ht/~rjarry/aerc/app.(*PartSwitcher).Show(...) /build/aerc/src/aerc/app/partswitcher.go:77 git.sr.ht/~rjarry/aerc/app.(*MessageViewer).Show(...) /build/aerc/src/aerc/app/msgviewer.go:409 git.sr.ht/~rjarry/aerc/lib/ui.(*Tabs).selectPriv(...) /build/aerc/src/aerc/lib/ui/tab.go:181 git.sr.ht/~rjarry/aerc/lib/ui.(*Tabs).Add(...) /build/aerc/src/aerc/lib/ui/tab.go:75 git.sr.ht/~rjarry/aerc/app.(*Aerc).NewTab(...) /build/aerc/src/aerc/app/aerc.go:511 git.sr.ht/~rjarry/aerc/app.NewTab(...) /build/aerc/src/aerc/app/app.go:61 git.sr.ht/~rjarry/aerc/commands/account.ViewMessage.Execute.func1(...) /build/aerc/src/aerc/commands/account/view.go:71 git.sr.ht/~rjarry/aerc/lib.NewMessageStoreView.func1(...) /build/aerc/src/aerc/lib/messageview.go:80 git.sr.ht/~rjarry/aerc/lib.NewMessageStoreView(...) /build/aerc/src/aerc/lib/messageview.go:124 git.sr.ht/~rjarry/aerc/commands/account.ViewMessage.Execute(...) /build/aerc/src/aerc/commands/account/view.go:52 git.sr.ht/~rjarry/aerc/commands.ExecuteCommand(...) /build/aerc/src/aerc/commands/commands.go:205 main.execCommand(...) Remove that private err field and return an explicit error when the message cannot be opened to enforce handling of the error by the caller. When the msg argument is nil (only used in split viewer), return an empty message viewer object and ensure that all code paths that read the switcher or msg fields perform a nil check before accessing it. Link: https://lists.sr.ht/~rjarry/aerc-devel/%[email protected]%3E Link: https://lists.sr.ht/~rjarry/aerc-devel/%[email protected]%3E Link: https://lists.sr.ht/~rjarry/aerc-devel/%[email protected]%3E Reported-by: Benjamin Braun <[email protected]> Reported-by: Filip <[email protected]> Reported-by: Sarthak Bhan <[email protected]> Signed-off-by: Robin Jarry <[email protected]>
rjarry · Oct 22, 2024 · 7069217 · 7069217
1 parent 29f95c6
commit 7069217
Show file tree

Hide file tree

Showing 9 changed files with 69 additions and 30 deletions.
diff --git a/app/account.go b/app/account.go
@@ -665,7 +665,12 @@ func (acct *AccountView) updateSplitView(msg *models.MessageInfo) {
 					PushError(err.Error())
 					return
 				}
-				acct.split = NewMessageViewer(acct, view)
+				viewer, err := NewMessageViewer(acct, view)
+				if err != nil {
+					PushError(err.Error())
+					return
+				}
+				acct.split = viewer
 				switch acct.splitDir {
 				case config.SPLIT_HORIZONTAL:
 					acct.grid.AddChild(acct.split).At(1, 1)
@@ -711,7 +716,7 @@ func (acct *AccountView) Split(n int) {
 
 	acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.UiConfig())).Span(2, 1)
 	acct.grid.AddChild(ui.NewBordered(acct.msglist, ui.BORDER_BOTTOM, acct.UiConfig())).At(0, 1)
-	acct.split = NewMessageViewer(acct, nil)
+	acct.split, _ = NewMessageViewer(acct, nil)
 	acct.grid.AddChild(acct.split).At(1, 1)
 	msg, err := acct.SelectedMessage()
 	if err != nil {
@@ -740,7 +745,7 @@ func (acct *AccountView) Vsplit(n int) {
 
 	acct.grid.AddChild(ui.NewBordered(acct.dirlist, ui.BORDER_RIGHT, acct.UiConfig())).At(0, 0)
 	acct.grid.AddChild(ui.NewBordered(acct.msglist, ui.BORDER_RIGHT, acct.UiConfig())).At(0, 1)
-	acct.split = NewMessageViewer(acct, nil)
+	acct.split, _ = NewMessageViewer(acct, nil)
 	acct.grid.AddChild(acct.split).At(0, 2)
 	msg, err := acct.SelectedMessage()
 	if err != nil {

diff --git a/app/msglist.go b/app/msglist.go
@@ -314,7 +314,11 @@ func (ml *MessageList) MouseEvent(localX int, localY int, event vaxis.Event) {
 							PushError(err.Error())
 							return
 						}
-						viewer := NewMessageViewer(acct, view)
+						viewer, err := NewMessageViewer(acct, view)
+						if err != nil {
+							PushError(err.Error())
+							return
+						}
 						NewTab(viewer, msg.Envelope.Subject)
 					})
 			}

diff --git a/app/msgviewer.go b/app/msgviewer.go
@@ -50,7 +50,6 @@ var _ ProvidesMessages = (*MessageViewer)(nil)
 
 type MessageViewer struct {
 	acct     *AccountView
-	err      error
 	grid     *ui.Grid
 	switcher *PartSwitcher
 	msg      lib.MessageView
@@ -59,12 +58,9 @@ type MessageViewer struct {
 
 func NewMessageViewer(
 	acct *AccountView, msg lib.MessageView,
-) *MessageViewer {
+) (*MessageViewer, error) {
 	if msg == nil {
-		return &MessageViewer{
-			acct: acct,
-			err:  fmt.Errorf("(no message selected)"),
-		}
+		return &MessageViewer{acct: acct}, nil
 	}
 	hf := HeaderLayoutFilter{
 		layout: HeaderLayout(config.Viewer.HeaderLayout),
@@ -130,13 +126,7 @@ func NewMessageViewer(
 	switcher := &PartSwitcher{}
 	err := createSwitcher(acct, switcher, msg)
 	if err != nil {
-		return &MessageViewer{
-			acct:     acct,
-			err:      err,
-			grid:     grid,
-			msg:      msg,
-			uiConfig: acct.UiConfig(),
-		}
+		return nil, err
 	}
 
 	borderStyle := acct.UiConfig().GetStyle(config.STYLE_BORDER)
@@ -161,7 +151,7 @@ func NewMessageViewer(
 	}
 	switcher.uiConfig = mv.uiConfig
 
-	return mv
+	return mv, nil
 }
 
 func fmtHeader(msg *models.MessageInfo, header string,
@@ -276,17 +266,17 @@ func createSwitcher(
 }
 
 func (mv *MessageViewer) Draw(ctx *ui.Context) {
-	if mv.err != nil {
+	if mv.switcher == nil {
 		style := mv.acct.UiConfig().GetStyle(config.STYLE_DEFAULT)
 		ctx.Fill(0, 0, ctx.Width(), ctx.Height(), ' ', style)
-		ctx.Printf(0, 0, style, "%s", mv.err.Error())
+		ctx.Printf(0, 0, style, "%s", "(no message selected)")
 		return
 	}
 	mv.grid.Draw(ctx)
 }
 
 func (mv *MessageViewer) MouseEvent(localX int, localY int, event vaxis.Event) {
-	if mv.err != nil {
+	if mv.switcher == nil {
 		return
 	}
 	mv.grid.MouseEvent(localX, localY, event)
@@ -338,6 +328,9 @@ func (mv *MessageViewer) MarkedMessages() ([]models.UID, error) {
 }
 
 func (mv *MessageViewer) ToggleHeaders() {
+	if mv.switcher == nil {
+		return
+	}
 	switcher := mv.switcher
 	switcher.Cleanup()
 	config.Viewer.ShowHeaders = !config.Viewer.ShowHeaders
@@ -354,6 +347,9 @@ func (mv *MessageViewer) ToggleKeyPassthrough() bool {
 }
 
 func (mv *MessageViewer) SelectedMessagePart() *PartInfo {
+	if mv.switcher == nil {
+		return nil
+	}
 	part := mv.switcher.SelectedPart()
 	return &PartInfo{
 		Index: part.index,
@@ -364,6 +360,9 @@ func (mv *MessageViewer) SelectedMessagePart() *PartInfo {
 }
 
 func (mv *MessageViewer) AttachmentParts(all bool) []*PartInfo {
+	if mv.switcher == nil {
+		return nil
+	}
 	return mv.switcher.AttachmentParts(all)
 }
 
@@ -398,15 +397,22 @@ func (mv *MessageViewer) Close() {
 }
 
 func (mv *MessageViewer) Event(event vaxis.Event) bool {
-	return mv.switcher.Event(event)
+	if mv.switcher != nil {
+		return mv.switcher.Event(event)
+	}
+	return false
 }
 
 func (mv *MessageViewer) Focus(focus bool) {
-	mv.switcher.Focus(focus)
+	if mv.switcher != nil {
+		mv.switcher.Focus(focus)
+	}
 }
 
 func (mv *MessageViewer) Show(visible bool) {
-	mv.switcher.Show(visible)
+	if mv.switcher != nil {
+		mv.switcher.Show(visible)
+	}
 }
 
 type PartViewer struct {

diff --git a/commands/account/next.go b/commands/account/next.go
@@ -77,7 +77,11 @@ func (np NextPrevMsg) Execute(args []string) error {
 						app.PushError(err.Error())
 						return
 					}
-					nextMv := app.NewMessageViewer(acct, view)
+					nextMv, err := app.NewMessageViewer(acct, view)
+					if err != nil {
+						app.PushError(err.Error())
+						return
+					}
 					app.ReplaceTab(mv, nextMv,
 						nextMsg.Envelope.Subject, true)
 				})

diff --git a/commands/account/view.go b/commands/account/view.go
@@ -61,7 +61,11 @@ func (v ViewMessage) Execute(args []string) error {
 				app.PushError(err.Error())
 				return
 			}
-			viewer := app.NewMessageViewer(acct, view)
+			viewer, err := app.NewMessageViewer(acct, view)
+			if err != nil {
+				app.PushError(err.Error())
+				return
+			}
 			data := state.NewDataSetter()
 			data.SetAccount(acct.AccountConfig())
 			data.SetFolder(acct.Directories().SelectedDirectory())

diff --git a/commands/eml.go b/commands/eml.go
@@ -49,7 +49,11 @@ func (e Eml) Execute(args []string) error {
 					app.PushError(err.Error())
 					return
 				}
-				msgView := app.NewMessageViewer(acct, view)
+				msgView, err := app.NewMessageViewer(acct, view)
+				if err != nil {
+					app.PushError(err.Error())
+					return
+				}
 				app.NewTab(msgView,
 					view.MessageInfo().Envelope.Subject)
 			})

diff --git a/commands/msg/delete.go b/commands/msg/delete.go
@@ -92,7 +92,11 @@ func (d Delete) Execute(args []string) error {
 								app.PushError(err.Error())
 								return
 							}
-							nextMv := app.NewMessageViewer(acct, view)
+							nextMv, err := app.NewMessageViewer(acct, view)
+							if err != nil {
+								app.PushError(err.Error())
+								return
+							}
 							app.ReplaceTab(mv, nextMv, next.Envelope.Subject, true)
 						})
 				}

diff --git a/commands/msg/move.go b/commands/msg/move.go
@@ -246,7 +246,11 @@ func handleDone(
 					app.PushError(err.Error())
 					return
 				}
-				nextMv := app.NewMessageViewer(acct, view)
+				nextMv, err := app.NewMessageViewer(acct, view)
+				if err != nil {
+					app.PushError(err.Error())
+					return
+				}
 				app.ReplaceTab(mv, nextMv, next.Envelope.Subject, true)
 			})
 	default:

diff --git a/commands/reload.go b/commands/reload.go
@@ -126,10 +126,14 @@ func (r Reload) Execute(args []string) error {
 		if !ok {
 			return
 		}
-		reloaded := app.NewMessageViewer(
+		reloaded, err := app.NewMessageViewer(
 			mv.SelectedAccount(),
 			mv.MessageView(),
 		)
+		if err != nil {
+			app.PushError(err.Error())
+			return
+		}
 		app.ReplaceTab(mv, reloaded, tab.Name, false)
 	})