Re: patch(1): infinite loop on malformed ed script input

On Fri, 03 Apr 2026 14:46:33 +0200,
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).
> 
> 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)
> 

It has interesting side effect.

another_hunk() exists with false when out_of_mem becames true, and old code
just moved to the next hunk. After your diff it thinks that it is a loop and
exists.

Not sure how to reproduce it, nor that old behaviour is right one.

-- 
wbr, Kirill