master process handling patch

[Patrick Goetz]

Sorry for the very delayed response to this message.  I was out of town, 
sick, etc., etc..


On 07/21/2010 05:58 PM, Henrique de Moraes Holschuh wrote:
> I don't think so.  Look at it very carefully.  Still, it was clearly useless
> so it was reverted quite recently (as in the last 48H).
>

This comment is regarding a patch to message_parse_header() in the file
~/imap/message.c

The relevant code snippet (with patch listed as a comment next to 
line-to-be-removed is

  /* Save header value */
  len = hdrend - hdr;
  message_ibuf_ensure(ibuf, len+2);
  strncpy(ibuf->end, hdr, len);     /* strncpy(ibuf->end, hdr, len+1); */
  ibuf->end += len;
  *(ibuf->end)++ = '\r';
  *(ibuf->end)++ = '\n';

You are correct:  the proposed code change copies over one extra out of 
bounds character, but this is then immediately overwritten by
  *(ibuf->end)++ = '\r';

Possible larger problems with this code:
1. Doesn't handle the case of hdr = NULL very well, since \r\n are 
always appended to the end of ibuf even when hdr is NULL.  Of course 
maybe this is what the code writer intended; there's no way of knowing 
because...
2. No comments explaining how the function is supposed to be used.
3. K&R-style function parameters (Seriously?  How long has ANSI C been 
available??)
4. Looking at the code, what would this indentation style be called? (I 
will refrain from suggesting names like WTF.)
5. (minor) Misuse of the term header -- the function should probably be 
called message_parse_field, since it's only parsing one field in the 
header and not the whole header.  Seriously , the function name alone 
confused me for a while until I spent the time to look up RFC 822/bis 
for header specifications.  By RFC 822, each line must be terminated by 
\r\n.  The code "fixes" the line endings at the end of the field, but 
ignores any possible bad line endings in the middle.

I'll take all this up with upstream and maybe fix these functions up myself.


>
> Patches 12 and 13 have been cleaned up (13 was broken up into separate
> patches as well), and documented.


Great!  Are you making these changes in your git repository or in Sven's 
SVN?  I've been going through the patches making notes on what needs to 
be sent upstream and what is questionable, and would like to make a 
final pass through the most up to date version of the code.


>  Now I am going to try fixing the !@#$
> master state machine for good, it is subtly broken, and it has been subtly
> broken for a *LONG* time.  Needs a rather pathological load to cause any
> issues, though.
>

I assume you're working on this in conjunction with upstream and not 
just as another Debian patch?  It's my strong opinion that it is not a 
good idea to introduce code changes at the package patch level.  This is 
almost guaranteed to cause problems down the road, since the upstream 
developers might introduce other changes that conflict or invalidate the 
local code.  Besides, anything like this needs to be vetted against the 
upstream developers.