patch mishandles corrupt context diff; writes NULs into .rej file

[Jim Meyering]

Hi Paul,

This is with patch-2.5.9.

Applying patch to a corrupt context diff can result in a .rej file
containing NUL bytes:

The problem arises when the input is a context diff and the second
byte on a line of context is not a space or tab.
Patch then happily gobbles/ignores that bogus byte and outputs
the rest of the line with an additional (NUL) byte at the end.

Create an "original" input file:
  $ seq 7 > f

Create a corrupt context-diff patch:
  $ diff -c <(seq 7) <(seq 3; seq 5 7)|sed 's/4/X/;s/ 7/7 yyy/' > d

Apply it (patch fails, as expected, due to 4/X mismatch):
  $ patch f d
  patching file f
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file f.rej
  [Exit 1]

Show the NUL bytes:
  $ cat -A f.rej
  ***************$
  *** 1,7 ****$
    1$
    2$
    3$
  - X$
    5$
    6$
     yyy$
  address@hidden 1,6 ----$
    1$
    2$
    3$
    5$
    6$
     yyy$
  ^@

FYI, the problem seems to be in pch.c:

            case ' ':
                s = buf + 1;
                chars_read--;
                if (*s == '\n' && canonicalize) {
                    strcpy (s, "\n");
                    chars_read = 2;
                }
                if (*s == ' ' || *s == '\t') {
                    s++;
                    chars_read--;
                } else if (repl_beginning && repl_could_be_missing) {
                    repl_missing = true;
                    goto hunk_done;
                }
                some_context = true;
                context++;
                if (repl_beginning)
                    repl_copiable++;
                else
                    ptrn_copiable++;
                chars_read -=
                  (1 < chars_read
                   && p_end == (repl_beginning ? p_max : p_ptrn_lines)
                   && incomplete_line ());
                p_len[p_end] = chars_read;
                if (! (p_line[p_end] = savebuf (buf + 2, chars_read))) {
                    p_end--;
                    return -1;
                }
                break;

In this case, "buf + 2" is one byte too far into the buffer,
so the recorded length, though correct, includes the trailing NUL.
Since "s" is incremented, but never used thereafter, and since
it seems to be just what I want, how about this (unidiff!) patch:

2007-08-26  Jim Meyering  <address@hidden>

        * pch.c (another_hunk): Avoid an off-by-one error that would
        result in NUL bytes in .rej files.

--- pch.c       2003-05-20 16:03:17.000000000 +0200
+++ pch.c       2007-08-26 13:23:49.525282692 +0200
@@ -1130,7 +1130,7 @@ another_hunk (enum diff difftype, bool r
                   && p_end == (repl_beginning ? p_max : p_ptrn_lines)
                   && incomplete_line ());
                p_len[p_end] = chars_read;
-               if (! (p_line[p_end] = savebuf (buf + 2, chars_read))) {
+               if (! (p_line[p_end] = savebuf (s, chars_read))) {
                    p_end--;
                    return -1;
                }

Alternatively, and perhaps better, would be to warn about
the corrupt patch and exit right away, but I'm not sure this
is an officially corrupt patch since patch is rather liberal
in what it accepts: should that space in column 2 really be optional
for context diffs?