Emails with multipart that was not properly closed may cause the parser to go into an infinite loop. In that case, io.Copy in findTextParts may return a de-facto EOF that is not *the* io.EOF. (Same with NextPart, this is not handled in this commit as I don't have a test case).
Introduce an arbitrary limit of parts to prevent this from happening in the future. Also, add another error handler for this special unexpected EOF. Signed-off-by: Franciszek Stachura <[email protected]> --- Example from netdev: https://lore.kernel.org/netdev/[email protected]/ pkg/mail/content.go | 7 +++++- pkg/mail/content_test.go | 7 ++++++ ...28-multipart-without-closing-boundary.mbox | 24 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 pkg/mail/testdata/mail/0028-multipart-without-closing-boundary.mbox diff --git a/pkg/mail/content.go b/pkg/mail/content.go index abf102b..b5801b1 100644 --- a/pkg/mail/content.go +++ b/pkg/mail/content.go @@ -26,7 +26,8 @@ func findTextParts(m *mail.Reader) []textPart { var results []textPart var buf strings.Builder - for { + // Arbitrary limit of parts to prevent an infinite loop + for range 1024 { part, err := m.NextPart() if errors.Is(err, io.EOF) { break @@ -57,6 +58,10 @@ func findTextParts(m *mail.Reader) []textPart { buf.Reset() if n, err := io.Copy(&buf, part.Body); err != nil { + if strings.Contains(err.Error(), "EOF") { + break + } + log.Warnf("failed to read part body: %s", err) continue } else if n == 0 { diff --git a/pkg/mail/content_test.go b/pkg/mail/content_test.go index 45771a7..9eb4c58 100644 --- a/pkg/mail/content_test.go +++ b/pkg/mail/content_test.go @@ -149,3 +149,10 @@ func TestInvalidCharset(t *testing.T) { assert.NotEmpty(t, diff, "expected diff despite invalid charset") assert.NotEmpty(t, comment, "expected comment despite invalid charset") } + +func TestMultipartWithoutClosingBoundary(t *testing.T) { + m := openTestMail(t, "mail/0028-multipart-without-closing-boundary.mbox") + diff, comment := FindPatchContent(m) + assert.Empty(t, diff, "expected no diff") + assert.NotEmpty(t, comment, "expected a comment despite multipart without closing boundary") +} diff --git a/pkg/mail/testdata/mail/0028-multipart-without-closing-boundary.mbox b/pkg/mail/testdata/mail/0028-multipart-without-closing-boundary.mbox new file mode 100644 index 0000000..bfe99f4 --- /dev/null +++ b/pkg/mail/testdata/mail/0028-multipart-without-closing-boundary.mbox @@ -0,0 +1,24 @@ +From [email protected] Wed Jun 20 13:35:48 2018 +From: Stephen Finucane <[email protected]> +To: "[email protected]" <[email protected]> +Subject: Re: [PATCH] parsemail: ignore html part of multi-part comments +Date: Wed, 20 Jun 2018 13:35:37 +0000 +Message-ID: <db5pr03mb18774049a0e62d211988ec8ca3...@db5pr03mb1877.eurprd03.prod.outlook.com> +References: <[email protected]> +In-Reply-To: <[email protected]> +Content-Type: multipart/alternative; + boundary="_000_DB5PR03MB18774049A0E62D211988EC8CA3770DB5PR03MB1877eurp_" +MIME-Version: 1.0 + + +--_000_DB5PR03MB18774049A0E62D211988EC8CA3770DB5PR03MB1877eurp_ +Content-Type: text/plain; charset="iso-8859-1" +Content-Transfer-Encoding: 8bit + +Yup, this looks sensible to me. Replying from Outlook's awful HTML editor to get +a sample comment to test with. + +Stephen + + +--_000_DB5PR03MB18774049A0E62D211988EC8CA3770DB5PR03MB1877eurp_ -- 2.55.0 _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
