patch(1): don't assume a match if we run out of context to check

[kevans]

Summary

Patches with very little context (-U0 and -U1) could get misapplied if
the file to be patched changes and a hunk is no longer applicable. Matching with
fuzz would be attempted and default to a match when we unexpectedly ran out of
context.

PR: 74127

[pfg]

Looks reasonable .. thanks!

[kevans]

Ping @emaste

[emaste]

Approved. Could we add a simple test case too?

[kevans]

Yeah, but that one's going to take a little bit of time, unfortunately. We currently have no tests for patch(1) at all, so I've got it on a list somewhere to write a bunch of patch(1) tests and include this as well as the other bug I recently fixed.

[emaste]

Or even a small example just in this Phabricator review that demonstrates the issue?

[pfg]

In D12631#267078, @kevans wrote:

Yeah, but that one's going to take a little bit of time, unfortunately. We currently have no tests for patch(1) at all, so I've got it on a list somewhere to write a bunch of patch(1) tests and include this as well as the other bug I recently fixed.

FWIW, Joerg Schilling has a testsuite for his fork of the patch utility:

https://sourceforge.net/p/schillix-on/schillix-on/ci/default/tree/usr/src/cmd/patch/

I can get you in touch if you want to keep playing with patch(1), otherwise, bugzilla already carries a testcase.

[kevans]

Sure; full example:

$ cat <<EOF > file.c
# This program is used to compress log files
if {![info exists Log(compressProg)]} {
        set Log(compressProg) gzip
}

# Flush interval
if {![info exists Log(flushInterval)]} {
        set Log(flushInterval) [expr {60 * 1000}]
}

# This is used to turn on an alternate debug log file
if {![info exist Log(debug_log)]} {
        set Log(debug_log) 0
}
EOF

$ cat <<EOF > bad.diff
file.c
@@ -5,1 +5,1 @@
-       set Log(compressProg) /usr/local/bin/gzip
+       set Log(compressProg) /usr/bin/gzip
EOF

$ patch file.c bad.diff

This patch will succeed, and you will end up with this file.c (bad):

# This program is used to compress log files
if {![info exists Log(compressProg)]} {
        set Log(compressProg) gzip
}
       set Log(compressProg) /usr/bin/gzip
# Flush interval
if {![info exists Log(flushInterval)]} {
        set Log(flushInterval) [expr {60 * 1000}]
}

# This is used to turn on an alternate debug log file
if {![info exist Log(debug_log)]} {
        set Log(debug_log) 0
}

With this fix, patch fails with the following output:

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|file.c
--------------------------
Patching file file.c using Plan A...
Hunk #1 failed at 5.
1 out of 1 hunks failed--saving rejects to file.c.rej
done

It's worth noting that redoing bad.diff with -Un (w/ n > 1) and current patch(1) causes patch file.c bad.diff to fail correctly.

[kevans]

In D12631#267099, @pfg wrote:

In D12631#267078, @kevans wrote:

Yeah, but that one's going to take a little bit of time, unfortunately. We currently have no tests for patch(1) at all, so I've got it on a list somewhere to write a bunch of patch(1) tests and include this as well as the other bug I recently fixed.

FWIW, Joerg Schilling has a testsuite for his fork of the patch utility:

https://sourceforge.net/p/schillix-on/schillix-on/ci/default/tree/usr/src/cmd/patch/

I can get you in touch if you want to keep playing with patch(1), otherwise, bugzilla already carries a testcase.

I think it would probably be a good idea to get in touch with Joerg on this one, if you wouldn't mind. =)

[antoine]

Could you revert the change and request an exp-run ? We are seeing some patch failures in the ports tree

[kevans]

exp-run has been requested in PR 223545

[pfg]

head/usr.bin/patch/patch.c

1029

Just noticed: shouldn't there by a space between any and more here? ;).

[pfg]

As seen in the exp-run, there are patches out there that will break for this case.
I think we should skip the check when the -f, --force flag is specified.
Perhaps just changing the check to:

if (!force && pline > pat_lines)

will do the trick.

[kevans]

All of the patches I've looked at so far (granted, I haven't looked at all of them) have been patches with context so far removed that it seems dangerous to allow this to continue. They're patches that apply by coincidence (because this defaults to a match) that have just fortunately not caused any noticeable runtime differences, so I'm not sure we should consider it advantageous to have -f skip this.

[cem]

I think we should consider just fixing the bad patches, then re-applying this change.

[pfg]

In D12631#270553, @cem wrote:

I think we should consider just fixing the bad patches, then re-applying this change.

Well, leaving the check by default will force the patches to be fixed. However, the conversion is easier by having an option to apply the older patch (POLA).
The --force flags is indeed clear:

-f, --force

	     Forces patch to assume that the user knows	exactly	what he	or she
	     is	doing, and to not ask any questions. ...

[kevans]

FWIW: @antoine started fixing up patches earlier today.

I think this would be ok, as long as we can eventually drop this application of force at some point down the road. These are patches that likely just haven't been checked for correctness in a long time since they still applied, so I do doubt whether it can be called a case of the user really knowing what they're doing. I can put that concern aside though if we don't have to carry this baggage forever.

[kevans]

Can we come to a consensus on this, before I re-commit? @antoine has fixed up the non-applicable patches in the ports tree, so those should not be an issue any longer. I go back and forth on how much damage might have been done; other BSDs seem to also have this same behavior, but GNU patch will not let these patches be mis-applied as we do.

head/usr.bin/patch/patch.c

1029

*nod*

[pfg]

In D12631#272110, @kevans wrote:

Can we come to a consensus on this, before I re-commit? @antoine has fixed up the non-applicable patches in the ports tree, so those should not be an issue any longer. I go back and forth on how much damage might have been done; other BSDs seem to also have this same behavior, but GNU patch will not let these patches be mis-applied as we do.

I am OK with whatever you think is more sensible.

[emaste]

As long as the ports tree is good let's get this back in.