Bug 208261 - contrib/dma unusable on freebsd.org cluster due to line length limits
Summary: contrib/dma unusable on freebsd.org cluster due to line length limits
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Baptiste Daroussin
URL:
Keywords:
Depends on:
Blocks: 208263
  Show dependency treegraph
 
Reported: 2016-03-24 17:51 UTC by Peter Wemm
Modified: 2018-12-13 21:53 UTC (History)
5 users (show)

See Also:


Attachments
proposed patch fixing local.c and net.c limits (2.22 KB, patch)
2018-05-16 19:32 UTC, Helge Oldach
no flags Details | Diff
proposed patch fixing local.c and net.c limits (corrected) (2.76 KB, patch)
2018-05-17 21:57 UTC, Helge Oldach
no flags Details | Diff
latest patch referred to on comment 17 (2.36 KB, patch)
2018-09-06 08:29 UTC, Helge Oldach
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Wemm freebsd_committer freebsd_triage 2016-03-24 17:51:38 UTC
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.
Comment 1 Mark Felder freebsd_committer 2016-03-24 20:18:51 UTC
Upstream bug: https://github.com/corecode/dma/issues/18
Comment 2 Kevin Bowling freebsd_committer 2016-03-24 23:42:01 UTC
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
Comment 3 Peter Wemm freebsd_committer freebsd_triage 2016-03-25 02:16:03 UTC
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.
Comment 4 Ed Maste freebsd_committer 2016-06-17 16:11:01 UTC
> 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
Comment 5 Ed Maste freebsd_committer 2016-06-17 16:13:16 UTC
> 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.
Comment 6 Ed Maste freebsd_committer 2016-06-17 17:51:44 UTC
Peter, would it be acceptable to you to split overlong body lines (as other MTAs do), still rejecting overlong header lines?
Comment 7 commit-hook freebsd_committer 2017-12-06 22:08:42 UTC
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
Comment 8 commit-hook freebsd_committer 2018-04-09 20:01:03 UTC
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
Comment 9 Helge Oldach 2018-05-16 17:01:15 UTC
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).
Comment 10 Helge Oldach 2018-05-16 17:09:30 UTC
(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
Comment 11 Helge Oldach 2018-05-16 18:03:38 UTC
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().
Comment 12 Helge Oldach 2018-05-16 19:32:56 UTC
Created attachment 193466 [details]
proposed patch fixing local.c and net.c limits
Comment 13 Helge Oldach 2018-05-17 21:45:05 UTC
(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.
Comment 14 Helge Oldach 2018-05-17 21:52:17 UTC
(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".
Comment 15 Helge Oldach 2018-05-17 21:57:04 UTC
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);
Comment 16 Ed Maste freebsd_committer 2018-09-05 18:26:19 UTC
"sizeof(line)+1" is not valid - it writes the nul beyond the end of the buffer
Comment 17 Helge Oldach 2018-09-06 08:24:17 UTC
(In reply to Ed Maste from comment #16)
Absolutely, reminding me to finally upload a patch proposal that I reworked some time ago.
Comment 18 Helge Oldach 2018-09-06 08:29:58 UTC
Created attachment 196909 [details]
latest patch referred to on comment 17