eliminating MAXHEADERSIZE

[Julien ÉLIE]

Hi Florian,

I sometimes have security warnings with your signed mails.
Windows Mail complains that the expeditor is not the right one:
mismatch between "zedat.fu-berlin.de" and "CIS.FU-Berlin.DE".



> yes, and I think it's a good idea to call it something other than
> MAXHEADERSIZE - how about MAXARTLINELENGTH, or is there a reason why
> we'd speak of a line size rather than a line length?

MAXARTLINELENGTH is fine.  Adopted.



> Please be careful when carrying out the substitutions, though, I'm not
> really familiar with all the occurrences and I think I was wrong when I
> said above that innd/art.c:848 is the only place where it really means line
> length; I think innd/art.c:1438 should also become MAXARTLINELENGTH and
> not MED_BUFFER.

You're totally right.  We're checking the Xref: header field length
at line 1438.



Incidentally, isn't there two bugs in this function?

* When resizing the buffer, extra place for a CRLF that may be added
  by a continuation line is not taken into account.  Consequently,
  when p[0] = '\r' and p[1] = '\n' are used, a segfault may occur!

* When comparing to MAXARTLINELENGTH, the final CRLF is not taken into
  account ("+2" is missing in the count), so the generated Xref: header
  field might end up with 1002 bytes!


OK, these are very edge cases.  I bet they never happened in the wild
but better fix them!


Suggested fix:


@@ -1429,14 +1430,16 @@
       continue;
     }
     ngp->Filenum = ngp->Last;
-    /*  len  ' ' "news_groupname"  ':' "#" "\r\n" */
-    if (len + 1 + ngp->NameLength + 1 + 10 + 2 > data->XrefBufLength) {
-      data->XrefBufLength += MAXHEADERSIZE;
+    /*  len  ' ' "news_groupname"  ':' "#" "\r\n"
+        plus an extra 2 bytes for "\r\n" in case of a continuation line. */
+    if (len + 1 + ngp->NameLength + 1 + 10 + 2 + 2 > data->XrefBufLength) {
+      data->XrefBufLength += MED_BUFFER;
       data->Xref = xrealloc(data->Xref, data->XrefBufLength);
       p = data->Xref + len;
     }
-    if (linelen + 1 + ngp->NameLength + 1 + 10 > MAXHEADERSIZE) {
-      /* line exceeded */
+    /* Trailing CRLF is counted in the maximum length. */
+    if (linelen + 1 + ngp->NameLength + 1 + 10 + 2 > MAXARTLINELENGTH) {
+      /* Line exceeded. */
       sprintf(p, "\r\n %s:%lu", ngp->Name, ngp->Filenum);
       buflen = strlen(p);
       linelen = buflen - 2;
@@ -1449,10 +1452,11 @@
     p += buflen;
   }
   /* p[0] is replaced with '\r' to be wireformatted when stored.  p[1] needs to
-     be '\n' */
+     be '\n'.  We have enough place to modify p here (checked during the
+     reallocation above). */
   p[0] = '\r';
   p[1] = '\n';
-  /* data->XrefLength includes trailing "\r\n" */
+  /* data->XrefLength includes trailing "\r\n". */





I also see that supporting 64-bit article numbers without latent bugs
is far from easy...
This "+10" in the code is a great pitfall...

-- 
Julien ÉLIE

« Tous les champignons sont comestibles. Certains, une fois seulement. »