Skip to content

ZMS-217 Fix message-splitter trailing line breaks being incorrectly broken into separate lines#31

Merged
NickOvt merged 3 commits intomasterfrom
ZMS-217-1
May 2, 2025
Merged

ZMS-217 Fix message-splitter trailing line breaks being incorrectly broken into separate lines#31
NickOvt merged 3 commits intomasterfrom
ZMS-217-1

Conversation

@NickOvt
Copy link
Copy Markdown
Contributor

@NickOvt NickOvt commented Apr 29, 2025

  1. Message-splitter would sometimes incorrectly break line breaks, resulting in lines containing only part of the CRLF line-break or a broken character alltogether (just /r or /n or /rbody> instead of /r/n<body>). The issue was especially noticeable if the input chunks fed to the splitter were in the range of 2-8 in size.
  2. Added appropriate tests in message-splitter-tests
  3. Fixed rewriter test to account to new hash. The hash is calculated based on MessageJoiner objects and thus with the new fix the joiner objects differ slightly from the previous logic - resulting in a different hash.

@NickOvt NickOvt merged commit 16eb658 into master May 2, 2025
4 checks passed
@NickOvt NickOvt deleted the ZMS-217-1 branch May 2, 2025 06:53
@andris9 andris9 restored the ZMS-217-1 branch June 29, 2025 13:13
@GioPan04
Copy link
Copy Markdown

GioPan04 commented Jul 3, 2025

Hi,
Now that this has been reverted due to zone-eu/wildduck#781, are there any plans to re implement this?

@andris9
Copy link
Copy Markdown
Member

andris9 commented Jul 3, 2025

@GioPan04 we’ll return to this once @NickOvt is back from vacation. The core issue is that most clients buffer outgoing data and send it in large chunks (for example, 64 kB). PHP, however, writes directly to the TCP socket, so bytes are sent exactly as you output them. With a single 64 kB chunk, it’s very unlikely that the chunk ends on the CRLF boundary that triggers the mailsplit bug. When you send 2 B + 100 B + 24 B instead, the chance that one of those smaller chunks ends on such a boundary, and therefore triggers the bug, becomes much higher.

@GioPan04
Copy link
Copy Markdown

GioPan04 commented Jul 3, 2025

@andris9 Okay, thanks for the info. I'll wait for the fix of @NickOvt then

@NickOvt
Copy link
Copy Markdown
Contributor Author

NickOvt commented Jul 17, 2025

Hello!

@GioPan04 I think that this fix in this PR has not been reverted by the PR you have linked here. The Titanism's PR addresses wildducks message splitter which (although very similar) is a bit different from generic mailsplit. And that PR has not yet been merged (nor properly reviewed/tested).

As Andris has mentioned this PR was made to address the issue of inconsistent chunks (mainly in PHPmailer).

The PR you have mentioned will be reviewed by me probably in the next week or two.

Cheers! 🌟

@GioPan04
Copy link
Copy Markdown

Hi @NickOvt , hope the vacation went well!

Anyway, about this PR, I think I wasn't clear in my last message.

I'm not interested in the wildduck PR, I was referring to it because, if I understood correctly, this PR broke something, and had to be reverted in #32.
I was asking if there were any plans to reimplement this PR

@NickOvt
Copy link
Copy Markdown
Contributor Author

NickOvt commented Jul 17, 2025

Oh I didn't notice the revert at first.
I'll get back to this bug then next week and look at it. Mailsplit is quite complex so it seems my fix fixed something but broke mailparser 😁.

I do suppose I'll reimplement this PR but highly likely differently than before

@GioPan04
Copy link
Copy Markdown

Okay, perfect. Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants