[Home]

Summary:ASTERISK-15407: [patch] Asterisk produces malformed email files for voicemail
Reporter:John Covert (jcovert)Labels:
Date Opened:2010-01-06 09:29:57.000-0600Date Closed:2010-03-31 14:34:51
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_voicemail
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 16557_v1.diff
( 1) 20100308__issue16557__1.4.diff.txt
( 2) 20100308__issue16557__1.6.0.diff.txt
( 3) 20100308__issue16557__trunk.diff.txt
( 4) 20100326__issue16557.diff.txt
Description:Asterisk is inserting an extra <cr> into the voicemail attachment.  This causes some mail relays to fail to forward the SMTP message.

While the correct line termination in an SMTP message IS <cr><lf>, the conversion from the Unix <lf> line terminator to <cr><lf> is handled by sendmail.  When sendmail gets a file which (mostly) has just <lf> terminators up until the attachment, it sees the <cr><lf> in the attachment portion and converts that to <cr><cr><lf>.  This confuses some mail relays down the road and prevents delivery.

This is a problem in all versions, and the fix is trivial:

At line 418 of the current head, there appears

#define eol "\r\n"

this should just be "\n"

The resulting SMTP message will still have <cr><lf> once it has been processed by the sendmail command.

"eol" is used ONLY in "ochar" which is used only in "base_encode".
Comments:By: Leif Madsen (lmadsen) 2010-01-06 10:16:12.000-0600

My only thought is, how does this affect other mail delivery transports other than sendmail?
By: Elazar Broad (ebroad) 2010-01-06 12:04:01.000-0600

http://www.smartertools.com/forums/t/20287.aspx

Edit: I have a SmarterMail instance to test on, though I use ssmtp on my Asterisk box instead of sendmail.


By: John Covert (jcovert) 2010-01-06 12:32:37.000-0600

Generally, even if it's another transport mechanism, the "sendmail" program (or a program by the same name supplied with the other transport) is still used by asterisk to submit the file, and should be responsible in all cases for converting Unix file format to what's needed going out.
By: Leif Madsen (lmadsen) 2010-01-06 13:19:41.000-0600

OK, that makes sense to me :) Just wanted to ask the question! Thanks for the response and patch.
By: Tilghman Lesher (tilghman) 2010-01-06 13:42:41.000-0600

Uh, what version of sendmail is doing this body conversion?  AFAIK, _no_ MTA should be messing with the body of the message at all.
By: John Covert (jcovert) 2010-01-06 14:43:54.000-0600

The RFC requires that what goes over the wire have <cr><lf> as line terminators.

Any Unix MTA which accepts a Unix file as input (typically by accepting "standard input") must change <lf> to <cr><lf> as it is streamed over the network connection.  The insertion of the extra <cr> by routine ochar is incorrect.  My suggested change makes the messages compliant with the RFC.  See the use of "MUST" in capital letters below.

RFC 2821             Simple Mail Transfer Protocol            April 2001


2.3.7 Lines

  SMTP commands and, unless altered by a service extension, message
  data, are transmitted in "lines".  Lines consist of zero or more data
  characters terminated by the sequence ASCII character "CR" (hex value
  0D) followed immediately by ASCII character "LF" (hex value 0A).
  This termination sequence is denoted as <CRLF> in this document.
  Conforming implementations MUST NOT recognize or generate any other
  character or character sequence as a line terminator.  Limits MAY be
  imposed on line lengths by servers (see section 4.5.3).

  In addition, the appearance of "bare" "CR" or "LF" characters in text
  (i.e., either without the other) has a long history of causing
  problems in mail implementations and applications that use the mail
  system as a tool.  SMTP client implementations MUST NOT transmit
  these characters except when they are intended as line terminators
  and then MUST, as indicated above, transmit them only as a <CRLF>
  sequence.
By: Elazar Broad (ebroad) 2010-01-08 11:39:48.000-0600

sSMTP(sendmail alternative) on Slackware behaves the same. Each mime line ends with 0x0d 0x0d 0x0a(wireshark dump), which ends up with a truncated attachment at the destination(SmarterMail). sSMTP has code to convert \n to \r\n so it can play nice with qmail.

A little research suggests that sendmail may replace LF with CRLF if it receives mixed terminators. One thing I noticed is that ENDL, which is used for the rest of the message body, is \n unless you are using IMAP storage(line 4214). So \n for headers and body, and \r\n for mime lines may cause sendmail to convert \r\n \r\r\n since it initially sees \n. Additionally, based on an e-mail thread on postfix-users about an identical issue with php mail(), postfix seems to exhibit the same behavior.
By: Bryant Zimmerman (zktech) 2010-02-20 10:58:34.000-0600

I have been having the same issue with wav e-mail attachments being cropped from SmarterMail as well as some mobile mail clients.  The suggested modification to app/app_voicemail.c appears to fix the issue. Is there any reason that this fix should not be moved into the SVN and released with the next offical build?
By: Leif Madsen (lmadsen) 2010-02-23 09:44:33.000-0600

Only reason is because it needs to be reviewed and committed. As you may have noticed we have over 600 open issues, so time and resources are pretty much the only reason for delay. Your patience is greatly appreciated!
By: Bryant Zimmerman (zktech) 2010-02-23 10:51:13.000-0600

Thanks for your response.

I wanted to make sure that there was not some know reason why the patch was not moving forward. Let me know how I can help test or push this and other patches forward. I appreciate all the hard work the developers are doing.

Thanks Bryant / zktech
By: Leif Madsen (lmadsen) 2010-02-24 09:52:18.000-0600

Just noticed there is no patch attached. If someone could make the patch and attach it to this issue, that would certainly help a lot. Thanks!
By: Bryant Zimmerman (zktech) 2010-02-24 09:56:53.000-0600

What is the process for doing this? If there are some directions some where I will be happy to make the patch.
By: Leif Madsen (lmadsen) 2010-02-24 10:20:18.000-0600

Basically, checkout a vanilla copy of Asterisk from subversion:

svn co http://svn.asterisk.org/svn/asterisk/trunk

If it applies to multiple branches (1.4, 1.6.x, etc...) and the patch needs to be changed to apply, also attach those patches (i.e. maybe the functionality change needs to be different between versions, or one version needs extra changes for IMAP and ODBC functionality, etc...)

Make the changes to the trunk and/or branches and then run:

svn diff > ~/__20100224-some-descriptive-patch-name.patch.txt

Then upload it to this issue and click the checkbox that says this is code or documentation contribution.

If you have not already signed the electronic license (you'll see at link at the top of the page in the title bar) do that first. If you have already signed it, then upload away!
By: Bryant Zimmerman (zktech) 2010-02-24 10:38:06.000-0600

My appologizes. I have never done a patch. I got the latest trunk using svn. I made the changes to the source.

at the /usr/src # prompt I type
svn diff > ~/__20100224-voicemail_eol_fix.patch.txt
I get a return of
svn: '.' is not a working copy
svn: Can't open file '.svn/entries' : No such file or directory

I am missing something any input would be appreciated, and sorry again for the newbie questions.
By: Leif Madsen (lmadsen) 2010-02-24 10:44:52.000-0600

Make sure you're in the checked out directory:

cd /usr/src/trunk/
svn diff ...
By: Bryant Zimmerman (zktech) 2010-03-06 09:46:45.000-0600

I finally got the patch to build. The one submited in  16557_v1.diff only covers IMAP storage. This issue is there for any storage format and really needs to be made globally based on what I can see. Sorry for the delay in posting. It took me some time to figure out how to do this as I am new to linux c dev.
By: Elazar Broad (ebroad) 2010-03-08 10:08:30.000-0600

The ifdef in 16557_v1.diff keeps \r\n for IMAP, otherwise it will use \n. The reason is because the same issue could arise with IMAP and mixed terminators, its best to stay consistent.
By: Bryant Zimmerman (zktech) 2010-03-08 13:05:37.000-0600

To restate for my understanding. We should use \n for everything, but IMAP in which we would use \r\n to maintain Linux line termination compliance.

Is this correct? If so that makes more sense.
Thanks for the clarifications.
By: Elazar Broad (ebroad) 2010-03-08 14:39:11.000-0600

For the most part yes, the idea being if we use \n for the message headers and the body(see app_voicemail.c, around line 4220 or so) then we should use \n when MIME encoding the attached voice message as well.
By: Tilghman Lesher (tilghman) 2010-03-08 15:53:15.000-0600

I've uploaded a replacement patch, both for 1.4 and 1.6.0, that covers an additional situation, where if the voicemail notification had a custom message, then the line terminators could have been inconsistent, when the storage method was IMAP.

Also, since ENDL is used elsewhere in the file, and since eol was going to be made equivalent to ENDL, I replaced the 'eol' definitions with ENDL definitions.
By: Tilghman Lesher (tilghman) 2010-03-08 15:54:24.000-0600

Additionally, I'm going to create a test scenario for trunk, to verify this behavior in the future.
By: Elazar Broad (ebroad) 2010-03-17 12:20:16

tilghman, I tested the patch against trunk and it works just fine.
By: Bryant Zimmerman (zktech) 2010-03-18 08:47:05

it appears to be working ok against the 1.6.14 version I am running in production as well.

I alos ran it against my test trunk version system and it seems to work
By: Digium Subversion (svnbot) 2010-03-31 14:09:47

Repository: asterisk
Revision: 255591

U   branches/1.4/apps/app_voicemail.c

------------------------------------------------------------------------
r255591 | tilghman | 2010-03-31 14:09:46 -0500 (Wed, 31 Mar 2010) | 15 lines

Ensure line terminators in email are consistent.

Fixes an issue with certain Mail Transport Agents, where attachments are not
interpreted correctly.

(closes issue ASTERISK-15407)
Reported by: jcovert
Patches:
      20100308__issue16557__1.4.diff.txt uploaded by tilghman (license 14)
      20100308__issue16557__1.6.0.diff.txt uploaded by tilghman (license 14)
      20100308__issue16557__trunk.diff.txt uploaded by tilghman (license 14)
Tested by: ebroad, zktech

Reviewboard: https://reviewboard.asterisk.org/r/544/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=255591
By: Digium Subversion (svnbot) 2010-03-31 14:13:03

Repository: asterisk
Revision: 255592

_U  trunk/
U   trunk/apps/app_voicemail.c

------------------------------------------------------------------------
r255592 | tilghman | 2010-03-31 14:13:02 -0500 (Wed, 31 Mar 2010) | 22 lines

Recorded merge of revisions 255591 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r255591 | tilghman | 2010-03-31 14:09:46 -0500 (Wed, 31 Mar 2010) | 15 lines
 
 Ensure line terminators in email are consistent.
 
 Fixes an issue with certain Mail Transport Agents, where attachments are not
 interpreted correctly.
 
 (closes issue ASTERISK-15407)
  Reported by: jcovert
  Patches:
        20100308__issue16557__1.4.diff.txt uploaded by tilghman (license 14)
        20100308__issue16557__1.6.0.diff.txt uploaded by tilghman (license 14)
        20100308__issue16557__trunk.diff.txt uploaded by tilghman (license 14)
  Tested by: ebroad, zktech
 
 Reviewboard: https://reviewboard.asterisk.org/r/544/
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=255592
By: Digium Subversion (svnbot) 2010-03-31 14:34:30

Repository: asterisk
Revision: 255674

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_voicemail.c

------------------------------------------------------------------------
r255674 | tilghman | 2010-03-31 14:34:30 -0500 (Wed, 31 Mar 2010) | 29 lines

Recorded merge of revisions 255592 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r255592 | tilghman | 2010-03-31 14:13:02 -0500 (Wed, 31 Mar 2010) | 22 lines
 
 Recorded merge of revisions 255591 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r255591 | tilghman | 2010-03-31 14:09:46 -0500 (Wed, 31 Mar 2010) | 15 lines
   
   Ensure line terminators in email are consistent.
   
   Fixes an issue with certain Mail Transport Agents, where attachments are not
   interpreted correctly.
   
   (closes issue ASTERISK-15407)
    Reported by: jcovert
    Patches:
          20100308__issue16557__1.4.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__1.6.0.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__trunk.diff.txt uploaded by tilghman (license 14)
    Tested by: ebroad, zktech
   
   Reviewboard: https://reviewboard.asterisk.org/r/544/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=255674
By: Digium Subversion (svnbot) 2010-03-31 14:34:40

Repository: asterisk
Revision: 255675

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_voicemail.c

------------------------------------------------------------------------
r255675 | tilghman | 2010-03-31 14:34:39 -0500 (Wed, 31 Mar 2010) | 29 lines

Recorded merge of revisions 255592 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r255592 | tilghman | 2010-03-31 14:13:02 -0500 (Wed, 31 Mar 2010) | 22 lines
 
 Recorded merge of revisions 255591 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r255591 | tilghman | 2010-03-31 14:09:46 -0500 (Wed, 31 Mar 2010) | 15 lines
   
   Ensure line terminators in email are consistent.
   
   Fixes an issue with certain Mail Transport Agents, where attachments are not
   interpreted correctly.
   
   (closes issue ASTERISK-15407)
    Reported by: jcovert
    Patches:
          20100308__issue16557__1.4.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__1.6.0.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__trunk.diff.txt uploaded by tilghman (license 14)
    Tested by: ebroad, zktech
   
   Reviewboard: https://reviewboard.asterisk.org/r/544/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=255675
By: Digium Subversion (svnbot) 2010-03-31 14:34:48

Repository: asterisk
Revision: 255676

_U  branches/1.6.2/
U   branches/1.6.2/apps/app_voicemail.c

------------------------------------------------------------------------
r255676 | tilghman | 2010-03-31 14:34:48 -0500 (Wed, 31 Mar 2010) | 29 lines

Recorded merge of revisions 255592 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r255592 | tilghman | 2010-03-31 14:13:02 -0500 (Wed, 31 Mar 2010) | 22 lines
 
 Recorded merge of revisions 255591 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r255591 | tilghman | 2010-03-31 14:09:46 -0500 (Wed, 31 Mar 2010) | 15 lines
   
   Ensure line terminators in email are consistent.
   
   Fixes an issue with certain Mail Transport Agents, where attachments are not
   interpreted correctly.
   
   (closes issue ASTERISK-15407)
    Reported by: jcovert
    Patches:
          20100308__issue16557__1.4.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__1.6.0.diff.txt uploaded by tilghman (license 14)
          20100308__issue16557__trunk.diff.txt uploaded by tilghman (license 14)
    Tested by: ebroad, zktech
   
   Reviewboard: https://reviewboard.asterisk.org/r/544/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=255676