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
