<<< Date Index >>>     <<< Thread Index >>>

Re: [Mutt] #2995: Mutt is folding subject line folds using a tab,



#2995: Mutt is folding subject line folds using a tab, which appears to be 
against
RFC 2822
----------------------+-----------------------------------------------------
  Reporter:  frnkblk  |       Owner:  mutt-dev                  
      Type:  defect   |      Status:  new                       
  Priority:  minor    |   Milestone:  1.6                       
 Component:  mutt     |     Version:  1.5.17                    
Resolution:           |    Keywords:  header folding white-space
----------------------+-----------------------------------------------------

Comment(by Derek Martin):

 {{{
 It is hard to believe this ticket has caused as much difficulty as it
 has... or that this bug has existed for almost exactly two years. :)

 On Tue, Mar 17, 2009 at 02:18:59PM -0000, Mutt wrote:

 This function was added in Mutt-1.5.14, which was released in April of
 2007.  As far as I can tell, Mutt did not insert a tab before that.
 This bug was reported in November of 2007, roughly 7 months later.
 Rocco asked why no one had noticed this before then...  I think the
 most likely explanation is a combination of the fact that until
 roughly then, (relatively) not many people were running those releases
 of Mutt, with the fact that most humans tend to write short subject
 lines (the subject is the header where this matters most, AFAICT).  I
 didn't spend the time to track it down, but I'd be willing to bet that
 the bug was reported shortly after Debian took a version of mutt
 containing this change...


 I don't see why this is so difficult.  Granted the function is hairy
 (which is a problem in and of itself), but it shouldn't be difficult
 to simply insert a newline before some whitespace.  However, that's
 not what it does... it does this:

       /* Space padding, but only if necessary */
       if (!first && *cp != '\t' && *cp != ' ')
       {
         if (fputc ('\t', fp) == EOF)
           return -1;
         col += 8 - (col % 8);
       }
     }

 If you read RFC (2)822, this seems clearly wrong.  The only character
 you are allowed to insert for line wrapping is a newline, which MUST
 be inserted before whitespace.  It doesn't say anything about
 *changing* existing whitespace.  Looking at this function, it appears
 to me that it has at least 4 fairly serious problems:

 1. It's long and hairy.  Everyone who's looked at it has said they
 would be loathe to try to modify it.  This is a pretty good sign that
 the function needs to be rewritten and probably broken down into
 smaller logical pieces.

 2. It uses iswspace() to determine if the next character is a
 whitespace character.  This is almost certainly wrong; RFC 2822
 defines WSP in headers as either ' ' or '\t'.  No other characters are
 considered WSP.  However, iswspace() probably evaluates to true in
 many other instances in various locales.  Probably a function
 is_rfc822_wsp() is needed (or a macro, though I really hate them).

 3. It is inserting a tab, where there was none, and where no RFC says
 you should insert one.  It is also *removing a space* in many cases.
 Who said you could do that?

 4. If the newline which was inserted for line wrapping was inserted
 correctly in the first place, this code should NEVER execute, since
 the next character after the inserted newline (which was from the
 original header) MUST be either ' ' or '\t'.  RFC 2822 says this:

    Each header field is logically a single line of characters
    comprising the field name, the colon, and the field body.  For
    convenience however, and to deal with the 998/78 character
    limitations per line, the field body portion of a header field can
    be split into a multiple line representation; this is called
    "folding".The general rule is that wherever this standard allows
    for folding white space (not simply WSP characters), a CRLF may be
    inserted before any WSP.

 It does not say "a CRLF and arbitrary additional WSP may be inserted
 before any WSP."   It absolutely doesn't say that you can change
 whitespace with arbitrary other whitespace.  It says you can insert a
 CRLF.  That's it.  Wherever WSP exists in the header, you can insert a
 CRLF before it.  That's not what mutt is doing, and what mutt
 interacts badly with other software that folds and unfolds correctly.

 FWIW Rocco's comments in earlier ticket updates about spaces being
 removed between encoded words in RFC 2047 are interesting, but
 irrelevant. RFC 2047 says white space between two encoded words must
 be ignored to account for the limit of 75 characters of any one
 encoded word, i.e. you need to be able to put them together into a
 single decoded encoded-word, if that is what they are to represent.
 Space between an encoded word and any unencoded word must not be
 ignored, as it is genuine space.  But we are not dealing with encoded
 words in this ticket, so RFC 2047 is not relevant.

 This code is wrong...  For the love of all that is good and decent in
 the world, please fix it, or let someone else fix it. :)

 FWIW, if I wrote this (which I could possibly do, maybe as soon as
 this weekend), I think the algorithm should (very) roughly be:

   1. start at the beginning of value (which is also called cp)
   2. get the next token (either WSP or MB-character), with the number
      bytes consumed from value, if there's one to get.
   3. If the token causes the line length to be greater than wraplen,
      and there are still chars left in value, or if there are no more
      characters left in value, insert '\n'.
   4. Add the token to the ouput buffer (if there was one).
   5. move the pointer that points to value up by the number of bytes
      consumed.
   6. If there are no more characters, print the buffer.

 I think the function should be divided in two: one that does step 2
 above (essentially the stuff in the big if block in the exiting
 function), and one that does the rest.  The block of code I quoted
 above should be removed entirely.

 A couple of other things to note:

 It looks to me like calling foldingstrfcpy() may be meant to fix
 this problem.  However, the '\t' is written directly to the file,
 instead of into the buffer, so it can't fix it.  Otherwise, it's not
 clear to me why the function is called (strncpy() would seem to
 be just as good here).  In fact, this seems to do the wrong thing; any
 tabs which the user *wanted* (ignoring the fact that this will
 essentially never be true) will be converted to spaces.

 Someone commented that this code may be shared by both the compose and
 the display code.  If that's the case, fixing it right probably
 requires an option to the function that controls whether the
 folded line is indented with a tab or not, for pretty display.

 If we're in an encoded word, but we find a '\n' (which is not allowed
 in an encoded word), we just assume that we were not really in an
 encoded word.  This seems possibly wrong, though maybe it isn't.  At
 the least, it seems like maybe mutt ought to warn the user that the
 header may be malformed (though I suppose if it is, they'll figure
 it out).  Is it possible this could be a worse bug?  Space is also not
 allowed in an encoded word, but there is no checking of this condition
 in that section of the function...

 Also while I can see why one might want to pass in the header's value
 as a const, there's no need to create the alias cp to value.  You can
 still increment the pointer even though the stuff it points to is
 constant.  Once the alias is created, the variable value is never
 referred to again...  In fact, the way the code is written is
 detrimental:  Generally, if you try to modify *cp the compiler will
 not complain, but your program will either segfault or let you do it!
 If you try to modify *value, you will get a compiler error.  [At
 least, this is how gcc behaves on my system.]

 Lastly the variables last and wrapped appear to be initialized
 twice; once in the variable declaration section, and again a bit
 further down in the code.
 }}}

-- 
Ticket URL: <http://dev.mutt.org/trac/ticket/2995#comment:>
Mutt <http://www.mutt.org/>
The Mutt mail user agent