Renaud Allard <[email protected]> wrote:
> This is the stall detection part from the earlier patch(1)
> report, split out for separate discussion. The fd leak
> fix (patch.c fclose) has ok op@.
>
> Problem: when intuit_diff_type() detects an ed script in
> malformed input, do_ed_script() may fail to advance the
> file position. On the next iteration, intuit_diff_type()
> finds the same pattern at the same position and the main
> loop repeats indefinitely. After hundreds of iterations,
> malloc detects heap corruption and aborts.
>
> Fix: track p_base across calls. If it hasn't advanced
> since the last call, the patch machinery failed to consume
> whatever was detected. Stop and treat the rest as garbage.
>
> The alternative of skipping one line and retrying was
> discussed. It guarantees reaching EOF but risks silent
> corruption: intuit_diff_type picks up any line starting
> with "@@ -" as a unified diff, and extracts filenames
> from "---"/"+++" lines. Garbled input could match these
> patterns, causing patch to write to unintended files or
> create files with arbitrary names. Stopping seems safer.
>
> The same bug is present in NetBSD, FreeBSD, and
> DragonFlyBSD (shared Larry Wall codebase).
as said on the other thread (our mails must have being crossed) I think
I'm overall fine with this. not a fan of the static var but that's what
we have available.
my worry was that we were stopping too early, since patch(1) usually
tries a bit hard to make sense out of garbage, but this is not the case.
we only re-enter here without advancing if we've discovered something
that then we've failed to process. At this layer, there's not much we
can do.
we could "fix" the ed code eventually to skip the whole ed patch, but as
a guardrail I think we still need something like this.
> Index: usr.bin/patch/pch.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/patch/pch.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 pch.c
> --- usr.bin/patch/pch.c 12 Jul 2023 15:45:34 -0000 1.66
> +++ usr.bin/patch/pch.c
> @@ -180,6 +180,7 @@ static char *posix_name(const struct fil
> bool
> there_is_another_patch(void)
> {
> + static off_t prev_p_base = -1;
> bool exists = false;
>
> if (p_base != 0 && p_base >= p_filesize) {
> @@ -190,6 +191,12 @@ there_is_another_patch(void)
> if (verbose)
> say("Hmm...");
> diff_type = intuit_diff_type();
> + if (diff_type && p_base == prev_p_base) {
> + if (verbose)
> + say(" Ignoring the trailing garbage.\ndone\n");
> + return false;
> + }
> + prev_p_base = p_base;
> if (!diff_type) {
> if (p_base != 0) {
> if (verbose)
