Re: bugs in spool_copy_message() (fwd)
On Wed, 2005-08-10 at 11:16 -0700, Joshua Schmidlkofer wrote: > Alan Thew wrote: > > I found this mail (in my own cyrus archive) today after one of our users > > was getting messages rejected which they had sent locally. I get rid of > > NULLs during delivery (before it reaches Cyrus) and assumed it was a bug > > in delivery. In fact Cyrus (2.2.12) still produces: > > > > 554 5.6.0 Message contains NUL characters > > > > when it sees a line longer than 8190 bytes. > > > > I'm not objecting to a limit in line length but it would be useful if > > the error message was relevant. > > > > Thanks > > > > Damn, I just started seeing these WRT to stuff coming in through fetchmail. > This is timely as I at least now know what layer to tshoot. fetchmail doesn't like messages with NUL characters in the headers and I have that problem and have to occassionally go in and manually delete them from my ISP's mailbox. The consensus opinion I received when I last dealt with trying to solve the problem is that my ISP probably shouldn't be accepting those emails to begin with...damn Microsoft SMTP server. ;-) Craig Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: bugs in spool_copy_message() (fwd)
Alan Thew wrote: I found this mail (in my own cyrus archive) today after one of our users was getting messages rejected which they had sent locally. I get rid of NULLs during delivery (before it reaches Cyrus) and assumed it was a bug in delivery. In fact Cyrus (2.2.12) still produces: 554 5.6.0 Message contains NUL characters when it sees a line longer than 8190 bytes. I'm not objecting to a limit in line length but it would be useful if the error message was relevant. Thanks Damn, I just started seeing these WRT to stuff coming in through fetchmail. This is timely as I at least now know what layer to tshoot. thanks, Joshua Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: bugs in spool_copy_message()
On Wed, 27 Oct 2004 11:04:33 -0700 (PDT) Andrew Morgan <[EMAIL PROTECTED]> wrote: > > > On Wed, 27 Oct 2004, Philip Chambers wrote: > > > I have just found two flaws in the code which takes a message into cyrus (typically > > during the DATA phase of LMTP. I am amazed that one has existed for so long. > > > > It means that messages with a line longer that 8190 bytes will be rejected with the > > error "Message contains NUL characters". (Confirmed in testing.) > > I've had some of our users report the "Message contains NUL characters" > bounce message, but I could never figure out why. If you come up with a > patch for this, I'd be very interested in applying it here. Note: We are > still running cyrus-imapd 2.1.16 here... > > Andy In 2.1.16 this code may be in copy_message() in lmtpengine.c, you will need to check. I now have a fix which I believe good and has worked under test. It seems to me that the original code is too complex and can be simplified substantially. To that end I have made a small change to prot_fgets() in lib/prot.c: all calls to prot_fgets() just check for a zero/non-zero result, so I have changed it to return "p" rather than "buf". That means that spool_copy_message() can know exactly where the end of the data is that has been read. The code in spool_copy_message() becomes much simpler: char *end; /* -2: Might need room to add a \r\n\0 set */ while (end = prot_fgets(buf, sizeof(buf)-2, fin)) { p = buf + strlen(buf) - 1; if (p < end-2) { /* strlen stopped before end */ r = IMAP_MESSAGE_CONTAINSNULL; continue; /* need to eat the rest of the message */ } else if (p[0] == '\r') { /* * We were unlucky enough to get a CR just before we ran * out of buffer--put it back. */ prot_ungetc('\r', fin); *p = '\0'; } else if (p[0] == '\n' && (p == buf || p[-1] != '\r')) { /* found an \n without a \r */ p[0] = '\r'; p[1] = '\n'; p[2] = '\0'; } /* Remove any lone CR characters */ ... as before Phil. --- Phil Chambers ([EMAIL PROTECTED]) University of Exeter --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: RE: bugs in spool_copy_message()
On Thu, 28 Oct 2004 09:37:50 +0200 =?iso-8859-1?Q?Brasseur_Val=E9ry?= <[EMAIL PROTECTED]> wrote: > > > > It should be this simple: > > --- spool.c 16 Sep 2004 17:58:54 - 1.6 > > +++ spool.c 27 Oct 2004 20:36:00 - > > @@ -451,7 +451,7 @@ > > p[1] = '\n'; > > p[2] = '\0'; > > } > > - else if (p[0] != '\n') { > > + else if (p[0] != '\n' && (strlen(buf) < sizeof(buf)-2)) { > > /* line contained a \0 not at the end */ > > r = IMAP_MESSAGE_CONTAINSNULL; > > continue; > > > > if the line is too long and there's a NULL further down, the > > next pass(es) > > through the loop will get it. I regret this still leaves holes in the code. The code before this change dealing with \r\0 does not work if one considers it carefully. I have a patch which I will send separately. Phil. --- Phil Chambers ([EMAIL PROTECTED]) University of Exeter --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
RE: bugs in spool_copy_message()
there was some times ago a discussion about cyrus/long line and RFC.. look at thread about "cyrus dealing with long lines" and "bare new lines problems" from avril 2003 on the list ... > -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] Behalf Of Derrick J > Brashear > Sent: Wednesday, October 27, 2004 10:40 PM > To: [EMAIL PROTECTED] > Subject: Re: bugs in spool_copy_message() > > > On Wed, 27 Oct 2004, Derrick J Brashear wrote: > > > Actually, I will look at this this afternoon; I have a > couple other bugs I > > need to look at first. > > It should be this simple: > --- spool.c 16 Sep 2004 17:58:54 - 1.6 > +++ spool.c 27 Oct 2004 20:36:00 - > @@ -451,7 +451,7 @@ > p[1] = '\n'; > p[2] = '\0'; > } > - else if (p[0] != '\n') { > + else if (p[0] != '\n' && (strlen(buf) < sizeof(buf)-2)) { > /* line contained a \0 not at the end */ > r = IMAP_MESSAGE_CONTAINSNULL; > continue; > > if the line is too long and there's a NULL further down, the > next pass(es) > through the loop will get it. > --- > Cyrus Home Page: http://asg.web.cmu.edu/cyrus > Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu > List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html > --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: bugs in spool_copy_message()
On Wed, 27 Oct 2004, Derrick J Brashear wrote: Actually, I will look at this this afternoon; I have a couple other bugs I need to look at first. It should be this simple: --- spool.c 16 Sep 2004 17:58:54 - 1.6 +++ spool.c 27 Oct 2004 20:36:00 - @@ -451,7 +451,7 @@ p[1] = '\n'; p[2] = '\0'; } - else if (p[0] != '\n') { + else if (p[0] != '\n' && (strlen(buf) < sizeof(buf)-2)) { /* line contained a \0 not at the end */ r = IMAP_MESSAGE_CONTAINSNULL; continue; if the line is too long and there's a NULL further down, the next pass(es) through the loop will get it. --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: bugs in spool_copy_message()
Actually, I will look at this this afternoon; I have a couple other bugs I need to look at first. On Wed, 27 Oct 2004, Philip Chambers wrote: I have just found two flaws in the code which takes a message into cyrus (typically during the DATA phase of LMTP. I am amazed that one has existed for so long. It means that messages with a line longer that 8190 bytes will be rejected with the error "Message contains NUL characters". (Confirmed in testing.) The code is in spool_copy_message() in spool.c (used to be in copy_message() in lmtpengine.c. The problems are in the loop: while(prot_fgets(...)). The code after "else if (p[0] == '\r')" ignores the case of a long line which contains \r\0 within it when it is the \0 which fills the buffer. The code will fail to notice the \0. More importantly, a line longer than 8190 characters will be picked up by the else statement (else if (p[0] != '\n') and treated as if it has a \0 in it even though it does not! I am about to work out a fix but, given the importance of this code, I need to spend a lot of time making sure I do not introduce a new bug. As I said, I find it hard to believe that cyrus has been unable to handle long lines for so long! Phil. --- Phil Chambers ([EMAIL PROTECTED]) University of Exeter --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html
Re: bugs in spool_copy_message()
On Wed, 27 Oct 2004, Philip Chambers wrote: > I have just found two flaws in the code which takes a message into cyrus (typically > during the DATA phase of LMTP. I am amazed that one has existed for so long. > > It means that messages with a line longer that 8190 bytes will be rejected with the > error "Message contains NUL characters". (Confirmed in testing.) I've had some of our users report the "Message contains NUL characters" bounce message, but I could never figure out why. If you come up with a patch for this, I'd be very interested in applying it here. Note: We are still running cyrus-imapd 2.1.16 here... Andy --- Cyrus Home Page: http://asg.web.cmu.edu/cyrus Cyrus Wiki/FAQ: http://cyruswiki.andrew.cmu.edu List Archives/Info: http://asg.web.cmu.edu/cyrus/mailing-list.html