Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/textproto: does not accept multiline error message #10230

Closed
DSpeichert opened this issue Mar 24, 2015 · 5 comments
Closed

net/textproto: does not accept multiline error message #10230

DSpeichert opened this issue Mar 24, 2015 · 5 comments

Comments

@DSpeichert
Copy link
Contributor

Summary

(r *Reader) ReadResponse(expectCode int) does not properly read all lines of a single error message (here's why: https://github.com/jnwhiteh/golang/blob/master/src/net/textproto/reader.go#L251).

Use case

The above behavior causes a multitude of problems. In my use case case I use net.smtp to open a single SMTP session to Gmail MX server, and then I repeat multiple times (below is a gist of it) for every message I have in a queue for this server (to avoid opening & closing a separate connection for every message).:

for ....... {
    if err = c.Reset(); err != nil {return err}
    if err = c.Mail(); err != nil {return err}
    if err = c.Rcpt(); err != nil {return err}
    if err = c.Data(); err != nil {return err}
}

Assuming that a recipient's email address doesn't exist, Gmail would return a multiline 550 error message, like this one:

550 5.1.1 The email account that you tried to reach does not exist. Please try
550 5.1.1 double-checking the recipient's email address for typos or
550 5.1.1 unnecessary spaces. Learn more at
550 5.1.1 http://support.google.com/mail/bin/answer.py?answer=6596 k3si4281809wjy.17 - gsmtp

However, err returned by c.Rcpt() would only have the first line.
Following lines will be returned by c.Reset in the following iteration of the loop, line by line!
This completely prevents me from reusing the connection, as there is no reliable way of flushing the remaining error message lines to get a "clean" smtp.Client. If c.Reset() doesn't do that, what will?

Workaround

I have written the following patch which fixes the problem. It passes the current reader test for a multi-line message but there is no test for a multi-line error (and I did not write one yet). The logic seems correct and I have tested this with smtp.Client - the problem mentioned above goes away and c.Rcpt() correctly returns a multiline error and smtp.Client may be reused for another message.

--- /root/reader.go     2015-03-24 01:51:58.000000000 +0100
+++ /usr/local/go/src/net/textproto/reader.go   2015-03-24 03:14:01.818576915 +0100
@@ -248,21 +248,43 @@
 //
 func (r *Reader) ReadResponse(expectCode int) (code int, message string, err error) {
        code, continued, message, err := r.readCodeLine(expectCode)
-       for err == nil && continued {
-               line, err := r.ReadLine()
-               if err != nil {
-                       return 0, "", err
-               }
+       if err == nil {
+               for err == nil && continued {
+                       line, err := r.ReadLine()
+                       if err != nil {
+                               return 0, "", err
+                       }

-               var code2 int
-               var moreMessage string
-               code2, continued, moreMessage, err = parseCodeLine(line, expectCode)
-               if err != nil || code2 != code {
-                       message += "\n" + strings.TrimRight(line, "\r\n")
-                       continued = true
-                       continue
+                       var code2 int
+                       var moreMessage string
+                       code2, continued, moreMessage, err = parseCodeLine(line, expectCode)
+                       if err != nil || code2 != code {
+                               message += "\n" + strings.TrimRight(line, "\r\n")
+                               continued = true
+                               continue
+                       }
+                       message += "\n" + moreMessage
+               }
+       } else {
+               var moreErrMessage string
+               for continued {
+                       line, err2 := r.ReadLine()
+                       if err2 != nil {
+                               break
+                       }
+                       var code2 int
+                       var moreMessage string
+                       code2, continued, moreMessage, err2 = parseCodeLine(line, 0)
+                       if err2 != nil || code2 != code {
+                               moreErrMessage += "\n" + strings.TrimRight(line, "\r\n")
+                               continued = true
+                               continue
+                       }
+                       moreErrMessage += "\n" + moreMessage
+               }
+               if moreErrMessage != "" {
+                       err = &Error{code, message+moreErrMessage}
                }
-               message += "\n" + moreMessage
        }
        return
 }
@mikioh mikioh changed the title net/textproto does not accept multiline error message net/textproto: does not accept multiline error message Mar 24, 2015
@minux minux added this to the Go1.5Maybe milestone Mar 24, 2015
@minux
Copy link
Member

minux commented Mar 24, 2015 via email

@rsc rsc removed the repo-main label Apr 14, 2015
@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jul 15, 2015
@DSpeichert
Copy link
Contributor Author

Change submitted: https://go-review.googlesource.com/#/c/18172/

@gopherbot
Copy link

CL https://golang.org/cl/18172 mentions this issue.

@RalphCorderoy
Copy link

I haven't seen anyone mention that Gmail's SMTP reply given at the top of this issue of a long message spread over multiple "550␣" lines is wrong, violating the SMTP RFC; only the last line should have a space after the 550, the rest having a minus sign. Shouldn't Gmail change?

@DSpeichert
Copy link
Contributor Author

@RalphCorderoy I'm not sure how I got that quoted message back in March. I've ran another test and got a properly-formatted error message which I included in the unit test for this CL. At least now everything seems good on Gmail's side when it comes to following RFCs.

@rsc rsc closed this as completed in 8ae584f Jan 8, 2016
@golang golang locked and limited conversation to collaborators Jan 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants