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

On 05/04/2026 21:08, Kirill A. Korinsky wrote:
> 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.
> 

You are completely right there. Reproducing it in real life seems highly 
unlikely, but the issue is there. Right now, I don't see how to fix that 
without touching the main loop, but that would be a way bigger patch.