Repository: jhillyerd/enmime
Pull request #240 (2022-04-08 09:26:08)

Maximal number of errors recorded in Part limited
Hi,

I use enmime to parse emails in my service. It happened that instead of valid mime structure, client sent several MBs of base64 data as input. In such case, enmime consumes a lot of memory (several GBs). It's because `readHeader` function adds many many warnings in order to "Attempt to detect and repair a non-indented continuation of previous line" as comment says.

In the end, `readHeader` calls `ReadMIMEHeader`, which doesn't find valid MIME structure and fails with error and all these warnings are freed. However the memory peak remains and can lead to swapping.

To make it more robust, I'm adding a `MaxPartErrors` global public variable, which does not allow to record more than limit errors in one part. Default value is 1000 in my PR, but it can be also 0 if you desire, which means no limit and thus there will be no change in current behavior.

Best regards
Pavel



iredmail: Maybe default `1000` is too many. :)
pavelbazika: Ok, it was meant primarily to avoid extreme memory consumption, what about 100?
jhillyerd: I think 100 is probably reasonable, in that it's not likely to provide any useful feedback beyond that.  For example, my Inbucket project displays these errors to the user to help them improve the quality of the email they send.

In my mind, this should probably be blocked by #90, so they it can be controlled without a global.  Let me think on accepting the PR without that though, since I expect 99% of users will never change it.
iredmail: for backward compatible, default value should be unlimited which is same as current release.
jhillyerd: That's good point, let's give it a high or infinite default for now.  I think we are about due to release 0.9.4.  I need to look at #90 more and determine if that should be before or after a 1.0.
pavelbazika: I'll give it default value 0, which means infinite. One more question, should I rather hide the global limit variable behind a getter/setter pair? Could be handy in the future, as you plan some options struct.
jhillyerd: No need for get/set pair: my plan for options is that they'd would be passed in with each call to decode; they wouldn't be global.

Patch:
diff --git a/error.go b/error.go
index 26a366a..1a94a09 100644
--- a/error.go
+++ b/error.go
@@ -25,6 +25,9 @@ const (
 	ErrorMissingRecipient = "no recipients (to, cc, bcc) set"
 )
 
+// MaxPartErrors limits number of part parsing errors, errors after the limit are ignored. 0 means unlimited.
+var MaxPartErrors = 0
+
 // Error describes an error encountered while parsing.
 type Error struct {
 	Name   string // The name or type of error encountered, from Error consts.
@@ -48,22 +51,25 @@ func (e *Error) String() string {
 
 // addWarning builds a severe Error and appends to the Part error slice.
 func (p *Part) addError(name string, detailFmt string, args ...interface{}) {
-	p.Errors = append(
-		p.Errors,
-		&Error{
-			name,
-			fmt.Sprintf(detailFmt, args...),
-			true,
-		})
+	p.addProblem(&Error{
+		name,
+		fmt.Sprintf(detailFmt, args...),
+		true,
+	})
 }
 
 // addWarning builds a non-severe Error and appends to the Part error slice.
 func (p *Part) addWarning(name string, detailFmt string, args ...interface{}) {
-	p.Errors = append(
-		p.Errors,
-		&Error{
-			name,
-			fmt.Sprintf(detailFmt, args...),
-			false,
-		})
+	p.addProblem(&Error{
+		name,
+		fmt.Sprintf(detailFmt, args...),
+		false,
+	})
+}
+
+// addProblem adds general *Error to the Part error slice.
+func (p *Part) addProblem(err *Error) {
+	if (MaxPartErrors == 0) || (len(p.Errors) < MaxPartErrors) {
+		p.Errors = append(p.Errors, err)
+	}
 }
diff --git a/part.go b/part.go
index 7bf56cc..2a59b6b 100644
--- a/part.go
+++ b/part.go
@@ -309,11 +309,7 @@ func (p *Part) base64CorruptInputCheck(err error) error {
 	switch errors.Cause(err).(type) {
 	case base64.CorruptInputError:
 		p.Content = nil
-		p.Errors = append(p.Errors, &Error{
-			Name:   ErrorMalformedBase64,
-			Detail: err.Error(),
-			Severe: true,
-		})
+		p.addError(ErrorMalformedBase64, err.Error())
 		return nil
 	default:
 		return err


Test patch:
diff --git a/error_test.go b/error_test.go
index 85c9638..fb49a0a 100644
--- a/error_test.go
+++ b/error_test.go
@@ -130,3 +130,42 @@ func TestErrorEnvelopeWarnings(t *testing.T) {
 		})
 	}
 }
+
+func TestErrorLimit(t *testing.T) {
+	// Backup global variable
+	originalMaxPartErros := MaxPartErrors
+	defer func() {
+		MaxPartErrors = originalMaxPartErros
+	}()
+
+	addThreeErrors := func() int {
+		part := &Part{}
+		part.addError("test1", "test1")
+		part.addError("test2", "test2")
+		part.addError("test3", "test3")
+
+		return len(part.Errors)
+	}
+
+	// Check unlimited
+	var errCount int
+	MaxPartErrors = 0
+	errCount = addThreeErrors()
+	if errCount != 3 {
+		t.Errorf("Expected unlimited errors (3), got %d", errCount)
+	}
+
+	// Check limit
+	MaxPartErrors = 1
+	errCount = addThreeErrors()
+	if errCount != 1 {
+		t.Errorf("Expected limited errors (1), got %d", errCount)
+	}
+
+	// Check limit matching count
+	MaxPartErrors = 3
+	errCount = addThreeErrors()
+	if errCount != 3 {
+		t.Errorf("Expected limited errors (3), got %d", errCount)
+	}
+}