We have encountered show-stopper bugs in the freebsd.org cluster when trying to use dma as a drop-in sendmail replacement. This one lead to lost commit mail. mail.c: char line[1000]; Neither sendmail or postfix (or the rest of the internet) enforce this RFC2822 limit, at least for body text. dma bounces the email. We were unable to use dma as a drop-in replacement and had to revert.
Upstream bug: https://github.com/corecode/dma/issues/18
What is the desire from the project for this? I was thinking of changing it to sys/sbuf.h so it can be tweaked in the config. Interesting excerpt from sendmail book: "In using V8 sendmail's mc configuration, the default for the smtp, dsmtp, esmtp, and smtp8 delivery agents is 990. The default for the relay delivery agent is 2040. The default for all other delivery agents is 0." Postfix seems to default to 2048
I wouldn't be surprised if this has uncovered problems in our tools. We did hit it a bunch of times with the commit mail diff generator at 1024 though.
> I wouldn't be surprised if this has uncovered problems in our tools. Yeah, it looks like the commit mail generator ought to limit the line length at https://svnweb.freebsd.org/base/svnadmin/hooks/scripts/mailer.py?annotate=198826#l1117
> What is the desire from the project for this? > > I was thinking of changing it to sys/sbuf.h so it can be tweaked in the config. I replied in the upstream bug asking about either a config file option to set a (non-compliant) maximum line length or enable line splitting. I prefer the latter, but want to see what DMA authors think.
Peter, would it be acceptable to you to split overlong body lines (as other MTAs do), still rejecting overlong header lines?
A commit references this bug: Author: bapt Date: Wed Dec 6 22:08:35 UTC 2017 New revision: 326641 URL: https://svnweb.freebsd.org/changeset/base/326641 Log: Split body of mails not respecting RFC2822 For mails which has a body not respecting RFC2822 (which often happen with crontabs) try to split by words finding the last space before 1000's character If no spaces are found then consider the mail to be malformed anyway PR: 208261 Changes: head/contrib/dma/mail.c
A commit references this bug: Author: emaste Date: Mon Apr 9 20:00:07 UTC 2018 New revision: 332336 URL: https://svnweb.freebsd.org/changeset/base/332336 Log: MFC r326641 by bapt: Split body of mails not respecting RFC2822 For mails which has a body not respecting RFC2822 (which often happen with crontabs) try to split by words finding the last space before 1000's character If no spaces are found then consider the mail to be malformed anyway PR: 208261 Changes: _U stable/11/ stable/11/contrib/dma/mail.c
Please MFC. This artificial limit is a PITA as it's way to low (for example 'make world' spills out some 20 lines with >10.000 length).
(In reply to Helge Oldach from comment #9) Sorry. Nonsense. Please ignore. Already MFC'ed. This patch does not seem to be effective for local delivery: May 16 06:51:21 dephbgsa11dtk59 dma[75a4f.8018280a0]: local delivery failed: corrupted queue file May 16 06:51:21 dephbgsa11dtk59 dma[75a4f.8018280a0]: delivery failed, bouncing as 75925
I think we're hitting the 1000 limit again in local.c. This happens when a line is split into precisely 1000 characters including trailing \n. When such a line is read from the queue file (or upon immediate local delivery), deliver_local() chokes at this point: if (fgets(line, sizeof(line), it->mailf) == NULL) break; linelen = strlen(line); if (linelen == 0 || line[linelen - 1] != '\n') { Since we are reading exactly 1000 bytes, we don't get a trailing \0 within line[0..999]. strlen(line) therefore goes beyond the actual length of line until a \0 is located, likely somewhere on the stack. When it doesn't find a \n immediately preceding the \0, we're getting an error. Actually this is more like a security issue as we are reading beyond the allocated length of line into some random stack area. This must not happen. A quick fix would be to limit strlen: linelen = strnlen(line, sizeof(line)); The same issue exists in net.c in deliver_to_host().
Created attachment 193466 [details] proposed patch fixing local.c and net.c limits
(In reply to Helge Oldach from comment #11) There are actually a few more things broken. Firstly, fgets() semantics is not correctly implemented. fgets() actually reads at most *one character less* than the maximum specified as second parameter. Hence, we want to specify one more than the buffer size in order to really fill the buffer. This is important when we split lines into chunks of 1000 (== 999 useful characters plus one \n) when we want to fill a buffer of 1000 length. Instead of if (fgets(line, sizeof(line), it->mailf) == NULL) we want to have if (fgets(line, sizeof(line)+1, it->mailf) == NULL) Again, applicable to both net.c and local.c.
(In reply to Helge Oldach from comment #13) Secondly, the "line length minus 10" logic when splitting long lines is pretty odd: - A line of 999 bytes plus \n will not be split (1000 bytes) - A line of 1000 bytes plus \n will be split into two lines (991 bytes including \n and 10 bytes including \n). - A line of 1001 bytes plus \n will be split into two lines (991 bytes including \n and 11 bytes including \n). "Natural" behavior would be that too long lines are just always wrapped around at the latest position and the remainder spilling over to the next line: - A line of 999 bytes plus \n will not be split (1000 bytes) - A line of 1000 bytes plus \n will be split into two lines (1000 bytes including \n and 1 bytes plus \n). - A line of 1001 bytes plus \n will be split into two lines (1000 bytes including \n and 2 bytes plus \n). And so on. To achieve that we need to change the "minus 10" logic into "minus 1".
Created attachment 193502 [details] proposed patch fixing local.c and net.c limits (corrected) This patch fixes all glitches mentioned above. With this patch applied, we can deliver output from the following perl script both in queue mode as well as local and network delivery correctly and with correct wraparound after line length 1000: #!/usr/local/bin/perl print "\n"; map { printf "%04d:%s\n", $_, substr "1234567890"x500, 5, $_-6 } (980..1010);
"sizeof(line)+1" is not valid - it writes the nul beyond the end of the buffer
(In reply to Ed Maste from comment #16) Absolutely, reminding me to finally upload a patch proposal that I reworked some time ago.
Created attachment 196909 [details] latest patch referred to on comment 17
Getting back to this, with dma in the base system a mail with a 998 char line will be successfully delivered: cat <<EOF | /usr/libexec/dma -t From: test <text@example.org> To: [valid recipient] Subject: Testing 998 chars: $(/usr/libexec/flua -e 'print(string.rep("x", 998))') EOF Changing to 999 results in a bounce: --- This is the DragonFly Mail Agent v0.11+ at [host]. There was an error delivering your mail to <[recipient]>. corrupted queue file Message headers follow. --- sending 400 9-char words with spaces between $(/usr/libexec/flua -e 'print(string.rep("123456789 ", 400))') is successful, and results in the long line split in four Will review the proposed patches attached to this PR
A few discoveries: - line splitting was added in b0b2d05fd0609d504236e5429cef421a35237bd6 with the commit message stating "try to split by words finding the last space before 1000's character" - as Helge Oldach notes the line is actually split at: len = MAX_LINE_RFC822 - 10 (spaces do not factor in) - bounce I noticed at 999 is also reported by Helge Oldach
Created attachment 231403 [details] my local patches against stable/13 These are my local patches against stable/13. For me, it does the Right Thing when it comes to long lines. Note that main has imported newer snapshots and it looks like this patch requires a small bit of mungling to make it applicable to main. Going forward I suggest to first merge the imports from main to stable/13.
I had a quick walk through the upstream https://github.com/corecode/dma which is also in main. It seems this issue is fixed in a very different way in the meantime. I suspect that just merging main into stable/13 (and stable/12) will suffice.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=1a0dde338df8b493d74dcb2f7bbaaa6c02cab371 commit 1a0dde338df8b493d74dcb2f7bbaaa6c02cab371 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-01-28 14:57:44 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-01-28 15:02:43 +0000 dma: limit lines to 998 characters Per RFC2822 the maximum transmitted line length is "998 characters... excluding the CRLF." In a file the maximum is 999 with the \n included. Previously mail containing a line with exactly 999 characters would bounce. PR: 208261 Reported by: Helge Oldach MFC after: 1 week Sponsored by: The FreeBSD Foundation contrib/dma/mail.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
(In reply to Helge Oldach from comment #22) I believe this is still not addressed upstream, and https://github.com/corecode/dma/issues/18 is open for this. bapt@ has a change that splits at whitespace in https://github.com/bapt/dma/commit/6325f4d9475751dffa5f1329d8c536c2cace3780. I just committed the minimal change to reduce the limit to 999 (incl the \n) to address the issue in comment #19. We can converge on an ideal solution upstream.
(In reply to Ed Maste from comment #24) This fix should be applied again after import of the January snapshot...
(In reply to Helge Oldach from comment #25) 1a0dde338df8 is after the update (which was d045091ea25c)
I just replied on the github issue thread. https://github.com/corecode/dma/issues/18 It may be a pedantic overkill solution, especially for locally delivered mail
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=62a323ecbcdc14e12d9403ee1dac5ddd9ad857af commit 62a323ecbcdc14e12d9403ee1dac5ddd9ad857af Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-01-28 14:57:44 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-02-05 16:26:39 +0000 dma: limit lines to 998 characters Per RFC2822 the maximum transmitted line length is "998 characters... excluding the CRLF." In a file the maximum is 999 with the \n included. Previously mail containing a line with exactly 999 characters would bounce. PR: 208261 Reported by: Helge Oldach MFC after: 1 week Sponsored by: The FreeBSD Foundation (cherry picked from commit 1a0dde338df8b493d74dcb2f7bbaaa6c02cab371) contrib/dma/mail.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5c1ee92b0ebab49577d065baf7f37a8ffcde4259 commit 5c1ee92b0ebab49577d065baf7f37a8ffcde4259 Author: Ed Maste <emaste@FreeBSD.org> AuthorDate: 2022-01-28 14:57:44 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2022-02-05 16:26:42 +0000 dma: limit lines to 998 characters Per RFC2822 the maximum transmitted line length is "998 characters... excluding the CRLF." In a file the maximum is 999 with the \n included. Previously mail containing a line with exactly 999 characters would bounce. PR: 208261 Reported by: Helge Oldach MFC after: 1 week Sponsored by: The FreeBSD Foundation (cherry picked from commit 1a0dde338df8b493d74dcb2f7bbaaa6c02cab371) contrib/dma/mail.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
^Triage: now committed to all supported branches.