Mail corruption problem found (and fix)

[milton@ecn.purdue.edu (Milton D Miller)]

I have spent some more time looking for the mail problem, and I
think I have found it.  The problem is almost certainly in the new
usputs() code that was added recently (I think it was the 0131
release).  The code does not correctly handle the case of writing
a string with a newline that does not fit in the remaining space
of the current output buffer to a socket in ascii mode.  The problem
does not show up on the more command because it uses the default
line buffering mode (set up by the socket() call), but shows up in
the mailbox and presumably in the smtp client (I haven't double
checked the client).

The quick and dirty fix for those that don't want to mess with the
code but do have the C compiler is to recompile sockuser.c with
-Doldusputs which will then revert to the old, slower version that
called usputc().  While you are at it, check the other bug (the ip
checksum bug) to make sure it is fixed, this one bit me twice
yesterday while trying to write my message, I finally gave up and
moved to the terminal room (I later realized that was what was
catching me).  For those that don't remember, here it is again:
> The fix is straightforward. In iproute.c, function ip_route(),
> change the line
>       ip.checksum += 0x100;
>
> to
>       if(((ip.checksum += 0x100) & 0xff00) == 0)
>               ip.checksum++;  /* End-around carry */

Ok, now for the gory details of the real problem and suggested fix.
There are actually two problems, but the other one won't show up
nearly as often, and has a couple of solutions.  usputs consists
of a big while loop that copies as much data as possible, up to a
newline, to the output buffer, followed by a flush at the end of
the string if the flush character occurs anywhere in the string.
If the socket is in ascii mode and a newline is found in the string,
newline mode is set, a check is made that at least an eoln sequence
will fit in the buffer, then sets the length to copy to the minimum
of the length of the string to the newline and the amount of space
left in the output buffer minus space for the newline.  (After the
copy, if the newline flag is set it outputs a newline and skips
the next character.)  The problem is that it does not just want
the minimum ... if the string including the newline does not fit,
it should copy up to the newline in this iteration (turning the
newline flag back off) then output the rest of the string (which
will probably, but not necessarily, include the newline) on the
next iteration.

The other problem is that usputs(), unlike usputc(), does not
guarantee that there will be room for a full eoln in the next call,
but this assumed by usputc().  The real fix is to remove the
assumption in usputc, as the present code does not fill the buffer
unless the last two characters are a two character eoln sequence.
However, we don't maintain the buffers on block boundarys anyway,
so the quick fix is to add the same flush check at the end of
usputs().  If this bug is not fixed, the result will be overwriting
a byte off the end of the mbuf data space (aka memory corruption
in malloc()d space); it could be triggered presently by the mailbox
reader routine, if the headers are just the right length.  (The
condition is a puts fills the buffer to one character from the end
of the buffer, followed by a putc of a '\n'.)

I have made some sample diffs of a fix below, but I have neither
a PC of my own nor a compiler to test these changes, so they are
untested (they should compile, though).  I also made a trial fix
for the eol input processing mentioned yesterday, again untested.
The diffs follow, src0201/sockuser.c is the code from src0201.zip.
Who wants to be the one to try these out and report how they work?

milton

PS recvchar() should probaby maintain a register copy of &up->eol[0]
for speed, but I didn't want to hide the real changes.  I also found
a few missing commas in the non-ansi code when testing for syntax :-)
------------cut here and feed to patch----------
*** src0201/sockuser.c  Sun Jan 27 10:24:14 1991
--- sockuser.c  Sun Feb 24 17:19:57 1991
***************
*** 187,193 ****
  int arg7,arg8,arg9;
  int arg10,arg11,arg12;
  {
!       return usprintf(Curproc->output,fmt,arg1,arg2,arg3,arg4,arg5,arg6
                arg7,arg8,arg9,arg10,arg11,arg12);
  }
  /* Printf to socket. Doesn't use ANSI vsprintf */
--- 187,193 ----
  int arg7,arg8,arg9;
  int arg10,arg11,arg12;
  {
!       return usprintf(Curproc->output,fmt,arg1,arg2,arg3,arg4,arg5,arg6,
                arg7,arg8,arg9,arg10,arg11,arg12);
  }
  /* Printf to socket. Doesn't use ANSI vsprintf */
***************
*** 214,220 ****
                 */
                withargs = 1;
                buf = mallocw(SOBUF);
!               sprintf(buf,fmt,arg1,arg2,arg3,arg4,arg5,arg6,arg7
                 arg8,arg9,arg10,arg11,arg12);
                len = strlen(buf);
        }
--- 214,220 ----
                 */
                withargs = 1;
                buf = mallocw(SOBUF);
!               sprintf(buf,fmt,arg1,arg2,arg3,arg4,arg5,arg6,arg7,
                 arg8,arg9,arg10,arg11,arg12);
                len = strlen(buf);
        }
***************
*** 316,333 ****
                 && (cp = strchr(buf,'\n')) != NULLCHAR){
                        newline = 1;

!                       /* Ensure space for the eol sequence */
!                       if(bp->cnt + eol_len >= bp->size){
!                               /* Not enough room; flush and
!                                * go back for another buffer
                                 */
                                if(usflush(s) == -1)
                                        return EOF;
                                continue;
                        }
-                       /* Copy only up to newline, allowing space for eol */
-                       clen = cp - buf;
-                       clen  = min(clen,bp->size - bp->cnt - eol_len);
                } else {
                        newline = 0;
                        clen = min(len,bp->size - bp->cnt);
--- 316,340 ----
                 && (cp = strchr(buf,'\n')) != NULLCHAR){
                        newline = 1;

!                       /* Copy only up to newline, allowing space for eol */
!                       clen = cp - buf;
!                       if (clen > bp->size - bp->cnt - eol_len) {
!                           if (clen) {
!                               /* MDMII: copy what will fit, but don't
!                                * force the break into a newline.
                                 */
+                               newline = 0;
+                               clen  = min(clen,bp->size - bp->cnt);
+                           }
+                           else {
+                               /* Not enough room for the whole eoln,
+                                * flush and go back for another buffer
+                                */
                                if(usflush(s) == -1)
                                        return EOF;
                                continue;
+                           }
                        }
                } else {
                        newline = 0;
                        clen = min(len,bp->size - bp->cnt);
***************
*** 351,357 ****
                if((bp->cnt >= bp->size) && usflush(s) == -1)
                        return EOF;
        }
!       if(doflush && usflush(s) == -1)
                return EOF;

        return 0;
--- 358,367 ----
                if((bp->cnt >= bp->size) && usflush(s) == -1)
                        return EOF;
        }
!       /* MDMII: second problem quick fix:
!        * always ensure enough space for eoln is left in the buffer
!        */
!       if((doflush || bp->cnt >= bp->size - 2) && usflush(s) == -1)
                return EOF;

        return 0;
***************
*** 414,423 ****
                return c;

        /* This is the first char of a eol sequence. If the eol sequence is
!        * more than one char long, eat the next character in the input stream.
         */
!       if(up->eol[1] != '\0'){
!               (void)rrecvchar(s);
        }
        return '\n';
  }
--- 424,446 ----
                return c;

        /* This is the first char of a eol sequence. If the eol sequence is
!        * more than one char long, check the next character.  If it doesn't
!        * match put it back and return this character, otherwise return \n.
         */
!       if(up->eol[1] != '\0') {
!               if ((c = rrecvchar(s)) == EOF)
!                       return up->eol[0];      /* return the EOF next time */
!
!               if (c != up->eol[1]) {
!                       /* this is not a complete EOL sequence, return
!                        * the first character and save the second for
!                        * next time  (MDMII)
!                        */
!                       up->ibuf = pushdown(up->ibuf, 1);
!                       up->ibuf->data[0] = c;
!
!                       return up->eol[0];
!               }
        }
        return '\n';
  }