go-imap icon indicating copy to clipboard operation
go-imap copied to clipboard

Append possibly returning EOF on reader close

Open kkuehlz opened this issue 5 years ago • 3 comments

This issue was discovered in: https://todo.sr.ht/~sircmpwn/aerc2/269

aerc calls Append with an io.Pipe and passes the PipeReader to Append. Users reported a EOF being returned as an error to aerc. I suspect this is caused by the Reader being closed (aerc closes the write immediately after it completes i separate thread) and returning io.EOF to aerc. If this is the case, the EOF error should not be propagated back to aerc.

kkuehlz avatar Nov 30 '19 23:11 kkuehlz

Here is when go-imap client actually reads the literal (io.Reader): https://github.com/emersion/go-imap/blob/cfc6c581cf592705a35ccc6caf0c7bf73e4f5804/write.go#L154-L160

Relevant part from aerc code is:

func (imapw *IMAPWorker) handleAppendMessage(msg *types.AppendMessage) {
        if err := imapw.client.Append(msg.Destination, msg.Flags, msg.Date,
                &appendLiteral{
                        Reader: msg.Reader,
                        Length: msg.Length,
                }); err != nil {

                imapw.worker.PostMessage(&types.Error{
                        Message: types.RespondTo(msg),
                        Error:   err,
                }, nil)
        } else {
                imapw.worker.PostMessage(&types.Done{types.RespondTo(msg)}, nil)
        }
}

I am not sure how that might interfere with PipeReader.Close, does aerc wrap it with the correct msg.Length? I suspect that io.EOF may happen if the actual amount of data sent to the pipe is smaller than the value reported by Len().

foxcpp avatar Dec 01 '19 01:12 foxcpp

Here is the test case for go-imap/client with Append using io.Pipe, it fails with Append returning io.EOF. Note that literalWrap.L > len(msg).

Code
type literalWrap struct {
	io.Reader
	L int
}

func (lw literalWrap) Len() int {
	return lw.L
}

func TestClient_Append_Pipe(t *testing.T) {
	c, s := newTestClient(t)
	defer s.Close()

	setClientState(c, imap.AuthenticatedState, nil)

	msg := "Hello World!\r\nHello Gophers!\r\n"
	date := time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC)
	flags := []string{imap.SeenFlag, imap.DraftFlag}

	pr, pw := io.Pipe()

	done := make(chan error, 1)
	go func() {
		done <- c.Append("INBOX", flags, date, literalWrap{pr, 35})

		// The buffer is not flushed on error, force it so io.ReadFull can
		// continue.
		c.conn.Flush()
	}()

	tag, _ := s.ScanCmd()
	s.WriteString("+ send literal\r\n")

	pw.Write([]byte(msg))
	pw.Close()

	b := make([]byte, 30)
	if _, err := io.ReadFull(s, b); err != nil {
		t.Error(err)
	} else if string(b) != msg {
		t.Error("Bad literal:", string(b))
	}

	s.WriteString(tag + " OK APPEND completed\r\n")

	if err := <-done; err != nil {
		t.Fatalf("c.Append() = %v", err)
	}
}

foxcpp avatar Dec 01 '19 02:12 foxcpp

Thanks for investigating this so quickly!

kkuehlz avatar Dec 01 '19 20:12 kkuehlz

I believe this can be closed, as this was an aerc bug.

emersion avatar Mar 23 '23 12:03 emersion