From be3a0c5043ce6f6dd57ad55276e9b67447be6867 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Tue, 3 Dec 2019 18:55:09 +0000 Subject: [PATCH] MFC r351836, r351866, r354328: patch(1) /dev/null testing+improvement r351836: patch(1): add some basic tests Summary: - basic: test application of patches created by diff -u at the beginning/middle/end of file, which have differing amounts of context before and after chunks being added - limited_ctx: stems from PR 74127 in which a rogue line was getting added when the patch should have been rejected. Similar behavior was reproducible with larger contexts near the beginning/end of a file. See r326084 for details - file_creation: patch sourced from /dev/null should create the file - file_nodupe: said patch sourced from /dev/null shouldn't dupe the contents when re-applied (personal vendetta, WIP, see comment) - file_removal: this follows from nodupe; the reverse of a patch sourced from /dev/null is most naturally deleting the file, as is expected based on GNU patch behavior (WIP) r351866: patch(1): fix the file removal test, strengthen it a bit To remain compatible with GNU patch, we should ensure that once we're removing empty files after a reversed /dev/null patch we don't remove files that have been modified. GNU patch leaves these intact and just reverses the hunk that created the file, effectively implying --remove-empty-files for reversed /dev/null patches. r354328: patch(1): give /dev/null patches special treatment We have a bad habit of duplicating contents of files that are sourced from /dev/null and applied more than once... take the more sane (in most ways) GNU route and complain if the file exists and offer reversal options. This still falls short a little bit as selecting "don't reverse, apply anyway" will still give you duplicated file contents. There's probably other issues as well, but awareness is the first step to happiness. --- etc/mtree/BSD.tests.dist | 2 + usr.bin/patch/Makefile | 5 + usr.bin/patch/patch.1 | 8 +- usr.bin/patch/patch.c | 142 +++++++++++++++++++- usr.bin/patch/pch.c | 27 +++- usr.bin/patch/pch.h | 2 + usr.bin/patch/tests/Makefile | 12 ++ usr.bin/patch/tests/PR74127-cline.diff | 4 + usr.bin/patch/tests/PR74127-good.diff | 4 + usr.bin/patch/tests/PR74127-repro.diff | 4 + usr.bin/patch/tests/PR74127.in | 14 ++ usr.bin/patch/tests/unified_patch_test.sh | 152 ++++++++++++++++++++++ usr.bin/patch/util.c | 4 +- 13 files changed, 372 insertions(+), 8 deletions(-) create mode 100644 usr.bin/patch/tests/Makefile create mode 100644 usr.bin/patch/tests/PR74127-cline.diff create mode 100644 usr.bin/patch/tests/PR74127-good.diff create mode 100644 usr.bin/patch/tests/PR74127-repro.diff create mode 100644 usr.bin/patch/tests/PR74127.in create mode 100755 usr.bin/patch/tests/unified_patch_test.sh diff --git a/etc/mtree/BSD.tests.dist b/etc/mtree/BSD.tests.dist index 4c630f48600..227b5ed67c2 100644 --- a/etc/mtree/BSD.tests.dist +++ b/etc/mtree/BSD.tests.dist @@ -1010,6 +1010,8 @@ .. opensm .. + patch + .. pr .. printf diff --git a/usr.bin/patch/Makefile b/usr.bin/patch/Makefile index a5f15c706a1..96caa8bb299 100644 --- a/usr.bin/patch/Makefile +++ b/usr.bin/patch/Makefile @@ -1,8 +1,13 @@ # $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $ # $FreeBSD$ +.include + PROG= patch SRCS= backupfile.c inp.c mkpath.c patch.c pch.c util.c +HAS_TESTS= +SUBDIR.${MK_TESTS}+= tests + .include diff --git a/usr.bin/patch/patch.1 b/usr.bin/patch/patch.1 index e8478fc0438..3ad5287dee3 100644 --- a/usr.bin/patch/patch.1 +++ b/usr.bin/patch/patch.1 @@ -21,7 +21,7 @@ .\" .\" $OpenBSD: patch.1,v 1.27 2014/04/15 06:26:54 jmc Exp $ .\" $FreeBSD$ -.Dd August 15, 2015 +.Dd November 3, 2019 .Dt PATCH 1 .Os .Sh NAME @@ -559,8 +559,10 @@ option as needed. .Pp Third, you can create a file by sending out a diff that compares a null file to the file you want to create. -This will only work if the file you want to create does not exist already in -the target directory. +If the file you want to create already exists in the target directory when the +diff is applied, then +.Nm +will identify the patch as potentially reversed and offer to reverse the patch. .Pp Fourth, take care not to send out reversed patches, since it makes people wonder whether they already applied the patch. diff --git a/usr.bin/patch/patch.c b/usr.bin/patch/patch.c index 8342dba688b..a23fc82d3d9 100644 --- a/usr.bin/patch/patch.c +++ b/usr.bin/patch/patch.c @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -103,6 +104,7 @@ static void dump_line(LINENUM, bool); static bool patch_match(LINENUM, LINENUM, LINENUM); static bool similar(const char *, const char *, int); static void usage(void); +static bool handle_creation(bool, bool *); /* true if -E was specified on command line. */ static bool remove_empty_files = false; @@ -147,8 +149,10 @@ static char end_defined[128]; int main(int argc, char *argv[]) { + struct stat statbuf; int error = 0, hunk, failed, i, fd; - bool patch_seen, reverse_seen; + bool out_creating, out_existed, patch_seen, remove_file; + bool reverse_seen; LINENUM where = 0, newwhere, fuzz, mymaxfuzz; const char *tmpdir; char *v; @@ -219,6 +223,12 @@ main(int argc, char *argv[]) reinitialize_almost_everything()) { /* for each patch in patch file */ + if (source_file != NULL && (diff_type == CONTEXT_DIFF || + diff_type == NEW_CONTEXT_DIFF || + diff_type == UNI_DIFF)) + out_creating = strcmp(source_file, _PATH_DEVNULL) == 0; + else + out_creating = false; patch_seen = true; warn_on_invalid_line = true; @@ -226,6 +236,19 @@ main(int argc, char *argv[]) if (outname == NULL) outname = xstrdup(filearg[0]); + /* + * At this point, we know if we're supposed to be creating the + * file and we know if we should be trying to handle a conflict + * between the patch and the file already existing. We defer + * handling it until hunk processing because we want to swap + * the hunk if they opt to reverse it, but we want to make sure + * we *can* swap the hunk without running into memory issues + * before we offer it. We also want to be verbose if flags or + * user decision cause us to skip -- this is explained a little + * more later. + */ + out_existed = stat(outname, &statbuf) == 0; + /* for ed script just up and do it and exit */ if (diff_type == ED_DIFF) { do_ed_script(); @@ -252,9 +275,28 @@ main(int argc, char *argv[]) failed = 0; reverse_seen = false; out_of_mem = false; + remove_file = false; while (another_hunk()) { + assert(!out_creating || hunk == 0); hunk++; fuzz = 0; + + /* + * There are only three cases in handle_creation() that + * results in us skipping hunk location, in order: + * + * 1.) Potentially reversed but -f/--force'd, + * 2.) Potentially reversed but -N/--forward'd + * 3.) Reversed and the user's opted to not apply it. + * + * In all three cases, we still want to inform the user + * that we're ignoring it in the standard way, which is + * also tied to this hunk processing loop. + */ + if (out_creating) + reverse_seen = handle_creation(out_existed, + &remove_file); + mymaxfuzz = pch_context(); if (maxfuzz < mymaxfuzz) mymaxfuzz = maxfuzz; @@ -372,7 +414,6 @@ main(int argc, char *argv[]) /* and put the output where desired */ ignore_signals(); if (!skip_rest_of_patch) { - struct stat statbuf; char *realout = outname; if (!check_only) { @@ -383,7 +424,18 @@ main(int argc, char *argv[]) } else chmod(outname, filemode); - if (remove_empty_files && + /* + * remove_file is a per-patch flag indicating + * whether it's OK to remove the empty file. + * This is specifically set when we're reversing + * the creation of a file and it ends up empty. + * This is an exception to the global policy + * (remove_empty_files) because the user would + * likely not expect the reverse of file + * creation to leave an empty file laying + * around. + */ + if ((remove_empty_files || remove_file) && stat(realout, &statbuf) == 0 && statbuf.st_size == 0) { if (verbose) @@ -444,6 +496,9 @@ reinitialize_almost_everything(void) filearg[0] = NULL; } + free(source_file); + source_file = NULL; + free(outname); outname = NULL; @@ -1084,3 +1139,84 @@ similar(const char *a, const char *b, int len) return true; /* actually, this is not reached */ /* since there is always a \n */ } + +static bool +handle_creation(bool out_existed, bool *remove) +{ + bool reverse_seen; + + reverse_seen = false; + if (reverse && out_existed) { + /* + * If the patch creates the file and we're reversing the patch, + * then we need to indicate to the patch processor that it's OK + * to remove this file. + */ + *remove = true; + } else if (!reverse && out_existed) { + /* + * Otherwise, we need to blow the horn because the patch appears + * to be reversed/already applied. For non-batch jobs, we'll + * prompt to figure out what we should be trying to do to raise + * awareness of the issue. batch (-t) processing suppresses the + * questions and just assumes that we're reversed if it looks + * like we are, which is always the case if we've reached this + * branch. + */ + if (force) { + skip_rest_of_patch = true; + return (false); + } + if (noreverse) { + /* If -N is supplied, however, we bail out/ignore. */ + say("Ignoring previously applied (or reversed) patch.\n"); + skip_rest_of_patch = true; + return (false); + } + + /* Unreversed... suspicious if the file existed. */ + if (!pch_swap()) + fatal("lost hunk on alloc error!\n"); + + reverse = !reverse; + + if (batch) { + if (verbose) + say("Patch creates file that already exists, %s %seversed", + reverse ? "Assuming" : "Ignoring", + reverse ? "R" : "Unr"); + } else { + ask("Patch creates file that already exists! %s -R? [y] ", + reverse ? "Assume" : "Ignore"); + + if (*buf == 'n') { + ask("Apply anyway? [n]"); + if (*buf != 'y') + /* Don't apply; error out. */ + skip_rest_of_patch = true; + else + /* Attempt to apply. */ + reverse_seen = true; + reverse = !reverse; + if (!pch_swap()) + fatal("lost hunk on alloc error!\n"); + } else { + /* + * They've opted to assume -R; effectively the + * same as the first branch in this function, + * but the decision is here rather than in a + * prior patch/hunk as in that branch. + */ + *remove = true; + } + } + } + + /* + * The return value indicates if we offered a chance to reverse but the + * user declined. This keeps the main patch processor in the loop since + * we've taken this out of the normal flow of hunk processing to + * simplify logic a little bit. + */ + return (reverse_seen); +} diff --git a/usr.bin/patch/pch.c b/usr.bin/patch/pch.c index 50e1604b9b9..24e3eea5f29 100644 --- a/usr.bin/patch/pch.c +++ b/usr.bin/patch/pch.c @@ -70,6 +70,8 @@ static LINENUM p_bfake = -1; /* beg of faked up lines */ static FILE *pfp = NULL; /* patch file pointer */ static char *bestguess = NULL; /* guess at correct filename */ +char *source_file; + static void grow_hunkmax(void); static int intuit_diff_type(void); static void next_intuit_at(off_t, LINENUM); @@ -218,7 +220,12 @@ there_is_another_patch(void) bestguess = xstrdup(buf); filearg[0] = fetchname(buf, &exists, 0); } - if (!exists) { + /* + * fetchname can now return buf = NULL, exists = true, to + * indicate to the caller that /dev/null was specified. Retain + * previous behavior for now until this can be better evaluted. + */ + if (filearg[0] == NULL || !exists) { int def_skip = *bestguess == '\0'; ask("No file found--skip this patch? [%c] ", def_skip ? 'y' : 'n'); @@ -403,6 +410,24 @@ intuit_diff_type(void) names[OLD_FILE] = names[NEW_FILE]; names[NEW_FILE] = tmp; } + + /* Invalidated */ + free(source_file); + source_file = NULL; + + if (retval != 0) { + /* + * If we've successfully determined a diff type, stored in + * retval, path == NULL means _PATH_DEVNULL if exists is set. + * Explicitly specify it here to make it easier to detect later + * on that we're actually creating a file and not that we've + * just goofed something up. + */ + if (names[OLD_FILE].path != NULL) + source_file = xstrdup(names[OLD_FILE].path); + else if (names[OLD_FILE].exists) + source_file = xstrdup(_PATH_DEVNULL); + } if (filearg[0] == NULL) { if (posix) filearg[0] = posix_name(names, ok_to_create_file); diff --git a/usr.bin/patch/pch.h b/usr.bin/patch/pch.h index da7a08d4008..53ed71780e2 100644 --- a/usr.bin/patch/pch.h +++ b/usr.bin/patch/pch.h @@ -37,6 +37,8 @@ struct file_name { bool exists; }; +extern char *source_file; + void re_patch(void); void open_patch_file(const char *); void set_hunkmax(void); diff --git a/usr.bin/patch/tests/Makefile b/usr.bin/patch/tests/Makefile new file mode 100644 index 00000000000..41aeee08895 --- /dev/null +++ b/usr.bin/patch/tests/Makefile @@ -0,0 +1,12 @@ +# $FreeBSD$ + +PACKAGE= tests + +ATF_TESTS_SH+= unified_patch_test + +${PACKAGE}FILES+= PR74127-cline.diff +${PACKAGE}FILES+= PR74127-good.diff +${PACKAGE}FILES+= PR74127-repro.diff +${PACKAGE}FILES+= PR74127.in + +.include diff --git a/usr.bin/patch/tests/PR74127-cline.diff b/usr.bin/patch/tests/PR74127-cline.diff new file mode 100644 index 00000000000..551d86dfca8 --- /dev/null +++ b/usr.bin/patch/tests/PR74127-cline.diff @@ -0,0 +1,4 @@ +file.c +@@ -3,1 +3,1 @@ +- set Log(compressProg) /usr/local/bin/gzip ++ set Log(compressProg) /usr/bin/gzip diff --git a/usr.bin/patch/tests/PR74127-good.diff b/usr.bin/patch/tests/PR74127-good.diff new file mode 100644 index 00000000000..fac1726fcb8 --- /dev/null +++ b/usr.bin/patch/tests/PR74127-good.diff @@ -0,0 +1,4 @@ +file.c +@@ -3,1 +3,1 @@ +- set Log(compressProg) gzip ++ set Log(compressProg) /usr/local/bin/gzip diff --git a/usr.bin/patch/tests/PR74127-repro.diff b/usr.bin/patch/tests/PR74127-repro.diff new file mode 100644 index 00000000000..0ad3640ee39 --- /dev/null +++ b/usr.bin/patch/tests/PR74127-repro.diff @@ -0,0 +1,4 @@ +file.c +@@ -5,1 +5,1 @@ +- set Log(compressProg) /usr/local/bin/gzip ++ set Log(compressProg) /usr/bin/gzip diff --git a/usr.bin/patch/tests/PR74127.in b/usr.bin/patch/tests/PR74127.in new file mode 100644 index 00000000000..ed295c477cf --- /dev/null +++ b/usr.bin/patch/tests/PR74127.in @@ -0,0 +1,14 @@ +# 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 +} diff --git a/usr.bin/patch/tests/unified_patch_test.sh b/usr.bin/patch/tests/unified_patch_test.sh new file mode 100755 index 00000000000..afd4b457f8d --- /dev/null +++ b/usr.bin/patch/tests/unified_patch_test.sh @@ -0,0 +1,152 @@ +# +# SPDX-License-Identifier: BSD-2-Clause-FreeBSD +# +# Copyright (c) 2019 Kyle Evans +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. +# +# $FreeBSD$ + +atf_test_case basic +basic_body() +{ + printf "a\nb\nc\nd\ne\nf\ng\nh\ni\n" > foo_full + printf "a\nb\nc\n" > foo_start + printf "g\nh\ni\n" > foo_end + printf "d\ne\nf\n" > foo_middle + + diff -u foo_start foo_full > foo_start2full.diff + diff -u foo_end foo_full > foo_end2full.diff + diff -u foo_middle foo_full > foo_mid2full.diff + + # Check lengths... each should have all 9 lines + 3 line header + atf_check -o inline:"12" -x \ + "cat foo_start2full.diff | wc -l | tr -d '[:space:]'" + atf_check -o inline:"12" -x \ + "cat foo_end2full.diff | wc -l | tr -d '[:space:]'" + atf_check -o inline:"12" -x \ + "cat foo_mid2full.diff | wc -l | tr -d '[:space:]'" + + # Apply the patch! Should succeed + atf_check -o ignore patch foo_start foo_start2full.diff \ + -o foo_start2full + atf_check -o ignore patch foo_end foo_end2full.diff \ + -o foo_end2full + atf_check -o ignore patch foo_middle foo_mid2full.diff \ + -o foo_mid2full + + # And these should all produce equivalent to the original full + atf_check -o ignore diff foo_start2full foo_full + atf_check -o ignore diff foo_end2full foo_full + atf_check -o ignore diff foo_mid2full foo_full +} + +atf_test_case limited_ctx +limited_ctx_head() +{ + atf_set "descr" "Verify correct behavior with limited context (PR 74127)" +} +limited_ctx_body() +{ + + # First; PR74127-repro.diff should not have applied, but it instead + # assumed a match and added the modified line at the offset specified... + atf_check -s not-exit:0 -o ignore -e ignore patch -o _.out \ + "$(atf_get_srcdir)/PR74127.in" \ + "$(atf_get_srcdir)/PR74127-repro.diff" + + # Let's extend that and make sure a similarly ill-contexted diff does + # not apply even with the correct line number + atf_check -s not-exit:0 -o ignore -e ignore patch -o _.out \ + "$(atf_get_srcdir)/PR74127.in" \ + "$(atf_get_srcdir)/PR74127-line.diff" + + # Correct line number and correct old line should always work + atf_check -o ignore -e ignore patch -o _.out \ + "$(atf_get_srcdir)/PR74127.in" \ + "$(atf_get_srcdir)/PR74127-good.diff" +} + +atf_test_case file_creation +file_creation_body() +{ + + echo "x" > foo + diff -u /dev/null foo > foo.diff + rm foo + + atf_check -x "patch -s < foo.diff" + atf_check -o ignore stat foo +} + +# This test is motivated by long-standing bugs that occasionally slip by in +# commits. If a file is created by a diff, patch(1) will happily duplicate the +# contents as many times as you apply the diff. It should instead detect that +# a source of /dev/null creates the file, so it shouldn't exist. Furthermore, +# the reverse of creation is deletion -- hence the next test, which ensures that +# the file is removed if it's empty once the patch is reversed. The size checks +# are scattered throughout to make sure that we didn't get some kind of false +# error, and the first size check is merely a sanity check that should be +# trivially true as this is executed in a sandbox. +atf_test_case file_nodupe +file_nodupe_body() +{ + + echo "x" > foo + diff -u /dev/null foo > foo.diff + + atf_check -o inline:"2\n" stat -f "%z" foo + atf_check -s not-exit:0 -o ignore -x "patch -Ns < foo.diff" + atf_check -o inline:"2\n" stat -f "%z" foo + atf_check -s not-exit:0 -o ignore -x "patch -fs < foo.diff" + atf_check -o inline:"2\n" stat -f "%z" foo +} + +atf_test_case file_removal +file_removal_body() +{ + + echo "x" > foo + diff -u /dev/null foo > foo.diff + + # Check that the file is removed completely if it was sourced from + # /dev/null + atf_check -x "patch -Rs < foo.diff" + atf_check -s not-exit:0 -e ignore stat foo + + # But if it had been modified, we'll only remove the portion that the + # patch would have created. This makes us compatible with GNU patch's + # behavior, at least. Whether that is the sane action or not is a + # question for further study, and then this comment may be removed. + printf "x\ny\n" > foo + atf_check -x "patch -Rs < foo.diff" + atf_check -o inline:"y\n" cat foo +} + +atf_init_test_cases() +{ + atf_add_test_case basic + atf_add_test_case limited_ctx + atf_add_test_case file_creation + atf_add_test_case file_nodupe + atf_add_test_case file_removal +} diff --git a/usr.bin/patch/util.c b/usr.bin/patch/util.c index 39a2eedec35..080a3a266e3 100644 --- a/usr.bin/patch/util.c +++ b/usr.bin/patch/util.c @@ -366,8 +366,10 @@ fetchname(const char *at, bool *exists, int strip_leading) say("fetchname %s %d\n", at, strip_leading); #endif /* So files can be created by diffing against /dev/null. */ - if (strnEQ(at, _PATH_DEVNULL, sizeof(_PATH_DEVNULL) - 1)) + if (strnEQ(at, _PATH_DEVNULL, sizeof(_PATH_DEVNULL) - 1)) { + *exists = true; return NULL; + } name = fullname = t = savestr(at); tab = strchr(t, '\t') != NULL; -- 2.45.0