search

voodoodyne / subethasmtp

SubEtha SMTP is a Java library for receiving SMTP mail
Other
344 stars 138 forks source link

CharTerminatedInputStream swallows the last CRLF #39

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
CharTerminatedInputStream, which is used in the DATA command, removes the CRLF 
ending of the last line of the message body. This is incorrect, actually it (or 
some other class) should add a CRLF if it is missing. See RFC 5321 4.1.1.4. 
second paragraph.

I attach a test case, which currently fails on the second assertion.

Original issue reported on code.google.com by hontvari@flyordie.com on 24 Feb 2011 at 9:38

Attachments:

GoogleCodeExporter commented 9 years ago 
I started to think about how this has not caused errors until now, and the 
reason (at least for me) is that SmartClient compensates for the lost CRLF. But 
this is not correct either, because it unconditionally adds a CRLF, even if it 
is not necessary. 

On the other my comment about adding a missing CRLF is not correct too, the RFC 
refers to the originating SMTP system, i.e. the mail client, not an SMTP 
server. SubEthaSMTP could not even receive the data if the last CRLF is 
missing, so it has no chance to correct it.

Original comment by hontvari@flyordie.com on 24 Feb 2011 at 10:10

GoogleCodeExporter commented 9 years ago 
CharTerminatedInputStream was copied unmodified from Apache JAMES many years 
ago.  If you genuinely thing it has an issue, maybe compare against the latest 
at Apache?

Original comment by lhori...@gmail.com on 24 Feb 2011 at 10:31

GoogleCodeExporter commented 9 years ago 
The almost identical, likely only differently formatted version in Apache James 
3.0 m2 does the same, it swallows the last CR LF. I have to fix it one way or 
the other, not necessarily in SubEthaSMTP. What do you prefer?

Original comment by hontvari@flyordie.com on 25 Feb 2011 at 12:43

GoogleCodeExporter commented 9 years ago 
I attached a patch. It fixes both that the last CR LF is removed when receiving 
the message content and that SmartClient adds a CR LF even if it should not. On 
the other hand SmartClient still adds a CR LF if it is missing, but only then.

Incompatible changes: 
-I have removed CharTerminatedInputStream, it is not used anywhere in 
SubEthaSMTP now.
-The replacement class, DotTerminatedInputStream throws EOFException if the 
data stream unexpectedy ends with EOF. The original exception was a strange 
java.net.ProtocolException. Both are IOException, so this is unlikely to cause 
any problem.

Other:
-SmartClient source is reformatted. I used the Eclipse formatting settings 
which you sent sometime, and it seems to work now. I am not sure which is the 
correct format, but as I see the class was formatted differently than the rest 
of the project. So maybe the new formatting is the correct one. If you want I 
remove the new formatting.
-This patch may conflict with my previous timeout patch. Please apply that 
first, it works well at us for quite a long time. Then I will see whether I 
have to recreate this patch. Maybe this patch should be added only after the 
next release. By the way, if you want I can help with that.

Original comment by hontvari@flyordie.com on 26 Feb 2011 at 11:52

GoogleCodeExporter commented 9 years ago 
I attached a patch. It fixes both that the last CR LF is removed when receiving 
the message content and that SmartClient adds a CR LF even if it should not. On 
the other hand SmartClient still adds a CR LF if it is missing, but only then.

Incompatible changes: 
-I have removed CharTerminatedInputStream, it is not used anywhere in 
SubEthaSMTP now.
-The replacement class, DotTerminatedInputStream throws EOFException if the 
data stream unexpectedy ends with EOF. The original exception was a strange 
java.net.ProtocolException. Both are IOException, so this is unlikely to cause 
any problem.

Other:
-SmartClient source is reformatted. I used the Eclipse formatting settings 
which you sent sometime, and it seems to work now. I am not sure which is the 
correct format, but as I see the class was formatted differently than the rest 
of the project. So maybe the new formatting is the correct one. If you want I 
remove the new formatting.
-This patch may conflict with my previous timeout patch. Please apply that 
first, it works well at us for quite a long time. Then I will see whether I 
have to recreate this patch. Maybe this patch should be added only after the 
next release. By the way, if you want I can help with that.

Original comment by hontvari@flyordie.com on 26 Feb 2011 at 11:52

GoogleCodeExporter commented 9 years ago 
Patch attached

Original comment by hontvari@flyordie.com on 26 Feb 2011 at 11:53

Attachments:

GoogleCodeExporter commented 9 years ago 
You should now be a committer - welcome aboard :-)

Original comment by lhori...@gmail.com on 26 Feb 2011 at 5:03

GoogleCodeExporter commented 9 years ago

Original comment by hontvari@flyordie.com on 1 Mar 2011 at 9:23