From fc817d334380301adddd5e5a752fd6280dc7ae5d Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sat, 1 Aug 2026 16:09:53 +0200 Subject: [PATCH 1/4] tests: probe fixture files through the test library Tests reach for the filesystem helpers exported by the patching code to check that a file exists or to make a directory for a fixture. Route those checks through helpers in the library the tests already link, so that the tests do not care how the patching code spells its filesystem access. This keeps a later change to those helpers out of every test file. --- tests/lib/include/patch/test.h | 4 ++ tests/lib/src/test.cpp | 10 ++++ tests/test_basic.cpp | 92 +++++++++++++++++----------------- tests/test_ed.cpp | 2 +- tests/test_normal.cpp | 2 +- tests/test_quoting.cpp | 2 +- 6 files changed, 63 insertions(+), 49 deletions(-) diff --git a/tests/lib/include/patch/test.h b/tests/lib/include/patch/test.h index 1918d4e..e7dde4c 100644 --- a/tests/lib/include/patch/test.h +++ b/tests/lib/include/patch/test.h @@ -15,6 +15,10 @@ namespace Patch { +bool file_exists(const std::string& path); + +bool create_directory(const std::string& path); + void unset_env(const char* name); void set_env(const char* name, const char* value); diff --git a/tests/lib/src/test.cpp b/tests/lib/src/test.cpp index 225600c..3376ec3 100644 --- a/tests/lib/src/test.cpp +++ b/tests/lib/src/test.cpp @@ -17,6 +17,16 @@ namespace Patch { +bool file_exists(const std::string& path) +{ + return filesystem::exists(path); +} + +bool create_directory(const std::string& path) +{ + return filesystem::create_directory(path); +} + enum class Outcome { Passed, Failed, diff --git a/tests/test_basic.cpp b/tests/test_basic.cpp index 3894bbf..700619b 100644 --- a/tests/test_basic.cpp +++ b/tests/test_basic.cpp @@ -398,8 +398,8 @@ Hunk #1 FAILED at 1. EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 1); EXPECT_FILE_EQ("1", to_patch); - EXPECT_FALSE(Patch::filesystem::exists("1.rej")); - EXPECT_FALSE(Patch::filesystem::exists("1.orig")); + EXPECT_FALSE(Patch::file_exists("1.rej")); + EXPECT_FALSE(Patch::file_exists("1.orig")); } COMPAT_TEST(basic_patch_dry_run_remove_file) @@ -482,7 +482,7 @@ COMPAT_TEST(add_file) file.close(); } - EXPECT_FALSE(Patch::filesystem::exists("add")); + EXPECT_FALSE(Patch::file_exists("add")); Process process(patch_path, { patch_path, "-i", "diff.patch", nullptr }); @@ -509,7 +509,7 @@ COMPAT_TEST(add_file_without_dev_null_header) )"; } - EXPECT_FALSE(Patch::filesystem::exists("add")); + EXPECT_FALSE(Patch::file_exists("add")); Process process(patch_path, { patch_path, "-i", "diff.patch", nullptr }); @@ -579,7 +579,7 @@ COMPAT_TEST(add_file_using_basename) file.close(); } - EXPECT_FALSE(Patch::filesystem::exists("e")); + EXPECT_FALSE(Patch::file_exists("e")); Process process(patch_path, { patch_path, "-i", "diff.patch", nullptr }); @@ -608,7 +608,7 @@ COMPAT_TEST(add_file_missing_folders) file.close(); } - EXPECT_FALSE(Patch::filesystem::exists("a")); + EXPECT_FALSE(Patch::file_exists("a")); Process process(patch_path, { patch_path, "-i", "diff.patch", "-p0", nullptr }); @@ -768,11 +768,11 @@ COMPAT_TEST(remove_file_in_folders) EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); - EXPECT_FALSE(Patch::filesystem::exists("a/b/c/d/e")); - EXPECT_FALSE(Patch::filesystem::exists("a/b/c/d")); - EXPECT_FALSE(Patch::filesystem::exists("a/b/c")); - EXPECT_FALSE(Patch::filesystem::exists("a/b")); - EXPECT_TRUE(Patch::filesystem::exists("a")); + EXPECT_FALSE(Patch::file_exists("a/b/c/d/e")); + EXPECT_FALSE(Patch::file_exists("a/b/c/d")); + EXPECT_FALSE(Patch::file_exists("a/b/c")); + EXPECT_FALSE(Patch::file_exists("a/b")); + EXPECT_TRUE(Patch::file_exists("a")); EXPECT_FILE_EQ("a/1", second_file_contents); } @@ -814,7 +814,7 @@ static void remove_file(const char* patch_path, bool expect_removed, const std:: EXPECT_EQ(process.return_code(), 0); if (expect_removed) - EXPECT_FALSE(Patch::filesystem::exists("to_remove")); + EXPECT_FALSE(Patch::file_exists("to_remove")); else EXPECT_FILE_EQ("to_remove", ""); } @@ -856,8 +856,8 @@ COMPAT_TEST(deleting_to_explicit_output_writes_an_empty_file) file << "content\n"; } - Patch::filesystem::create_directory("outside"); - Patch::filesystem::create_directory("outside/deep"); + Patch::create_directory("outside"); + Patch::create_directory("outside/deep"); { Patch::File file("outside/deep/target.txt", std::ios_base::out); file << "old content\n"; @@ -869,8 +869,8 @@ COMPAT_TEST(deleting_to_explicit_output_writes_an_empty_file) EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("outside/deep/target.txt", ""); - EXPECT_TRUE(Patch::filesystem::exists("outside/deep")); - EXPECT_TRUE(Patch::filesystem::exists("outside")); + EXPECT_TRUE(Patch::file_exists("outside/deep")); + EXPECT_TRUE(Patch::file_exists("outside")); EXPECT_FILE_EQ("source.txt", "content\n"); } @@ -897,7 +897,7 @@ COMPAT_TEST(unified_patch_that_empties_file_keeps_empty_file) EXPECT_EQ(process.stdout_data(), "patching file target.txt\n"); EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); - EXPECT_TRUE(Patch::filesystem::exists("target.txt")); + EXPECT_TRUE(Patch::file_exists("target.txt")); EXPECT_FILE_EQ("target.txt", ""); } @@ -1039,7 +1039,7 @@ index 1111111..2222222 100644 EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("newdir/new.txt", "edited\n"); - EXPECT_FALSE(Patch::filesystem::exists("old.txt")); + EXPECT_FALSE(Patch::file_exists("old.txt")); } COMPAT_TEST(git_patch_copying_into_a_directory_which_does_not_exist) @@ -1099,14 +1099,14 @@ index 01e79c3..0000000 file.close(); } - EXPECT_TRUE(Patch::filesystem::exists("a")); + EXPECT_TRUE(Patch::file_exists("a")); Process process(patch_path, { patch_path, "-i", "diff.patch", nullptr }); EXPECT_EQ(process.stdout_data(), "patching file a\n"); EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); - EXPECT_FALSE(Patch::filesystem::exists("a")); + EXPECT_FALSE(Patch::file_exists("a")); } COMPAT_TEST(remove_file_that_has_trailing_garbage) @@ -1181,7 +1181,7 @@ index a45905b..0000000 file.close(); } - EXPECT_TRUE(Patch::filesystem::create_directory("libarchive")); + EXPECT_TRUE(Patch::create_directory("libarchive")); Process process(patch_path, { patch_path, "-i", "diff.patch", nullptr }); @@ -1385,7 +1385,7 @@ index de98044..0f7bc76 100644 static void test_chdir(const char* patch_path, const std::string& folder) { - EXPECT_TRUE(Patch::filesystem::create_directory(folder)); + EXPECT_TRUE(Patch::create_directory(folder)); { Patch::File file(folder + "/diff.patch", std::ios_base::out); @@ -1449,7 +1449,7 @@ rename to another_new EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("another_new", to_patch); - EXPECT_FALSE(Patch::filesystem::exists("orig_file")); + EXPECT_FALSE(Patch::file_exists("orig_file")); } COMPAT_TEST(reverse_rename_no_change) @@ -1477,7 +1477,7 @@ rename to y EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("x", to_patch); - EXPECT_FALSE(Patch::filesystem::exists("y")); + EXPECT_FALSE(Patch::file_exists("y")); } COMPAT_TEST(rename_with_change) @@ -1515,7 +1515,7 @@ index 71ac1b5..fc3102f 100644 EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("test", "a\nb\nc\nd\nf\ng\nh\n"); - EXPECT_FALSE(Patch::filesystem::exists("thing")); + EXPECT_FALSE(Patch::file_exists("thing")); } COMPAT_TEST(copy_no_change) @@ -1634,7 +1634,7 @@ rename to b EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("b", to_patch); - EXPECT_FALSE(Patch::filesystem::exists("a")); + EXPECT_FALSE(Patch::file_exists("a")); } COMPAT_TEST(rename_already_exists_with_content) @@ -1670,7 +1670,7 @@ index de98044..0f673f8 100644 EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("a", "a\n2\nc\n"); - EXPECT_FALSE(Patch::filesystem::exists("b")); + EXPECT_FALSE(Patch::file_exists("b")); } COMPAT_TEST(reverse_rename_already_exists_with_content) @@ -1706,7 +1706,7 @@ index de98044..0f673f8 100644 EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("a", "a\nb\nc\n"); - EXPECT_FALSE(Patch::filesystem::exists("b")); + EXPECT_FALSE(Patch::file_exists("b")); } COMPAT_TEST(backup_rename_patch) @@ -1747,7 +1747,7 @@ rename to b EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("b", to_patch); EXPECT_FILE_EQ("b.orig", ""); - EXPECT_FALSE(Patch::filesystem::exists("a")); + EXPECT_FALSE(Patch::file_exists("a")); EXPECT_FILE_EQ("a.orig", to_patch); } @@ -1778,7 +1778,7 @@ COMPAT_TEST(backup_of_removed_file) EXPECT_EQ(process.return_code(), 0); // The file is removed, so the backup is the only copy left of its content. - EXPECT_FALSE(Patch::filesystem::exists("gone.txt")); + EXPECT_FALSE(Patch::file_exists("gone.txt")); EXPECT_FILE_EQ("gone.txt.orig", to_patch); } @@ -1797,7 +1797,7 @@ COMPAT_TEST(backup_of_removed_file_in_subdirectory) const std::string to_patch = "content to lose\n"; { - Patch::filesystem::create_directory("sub"); + Patch::create_directory("sub"); Patch::File file("sub/nested.txt", std::ios_base::out); file << to_patch; file.close(); @@ -1809,7 +1809,7 @@ COMPAT_TEST(backup_of_removed_file_in_subdirectory) EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); - EXPECT_FALSE(Patch::filesystem::exists("sub/nested.txt")); + EXPECT_FALSE(Patch::file_exists("sub/nested.txt")); EXPECT_FILE_EQ("sub/nested.txt.orig", to_patch); } @@ -1838,8 +1838,8 @@ COMPAT_TEST(no_backup_of_removed_file_without_flag) EXPECT_EQ(process.stderr_data(), ""); EXPECT_EQ(process.return_code(), 0); - EXPECT_FALSE(Patch::filesystem::exists("gone.txt")); - EXPECT_FALSE(Patch::filesystem::exists("gone.txt.orig")); + EXPECT_FALSE(Patch::file_exists("gone.txt")); + EXPECT_FALSE(Patch::file_exists("gone.txt.orig")); } COMPAT_TEST(no_backup_of_removed_file_for_dry_run) @@ -1868,7 +1868,7 @@ COMPAT_TEST(no_backup_of_removed_file_for_dry_run) EXPECT_EQ(process.return_code(), 0); EXPECT_FILE_EQ("gone.txt", to_patch); - EXPECT_FALSE(Patch::filesystem::exists("gone.txt.orig")); + EXPECT_FALSE(Patch::file_exists("gone.txt.orig")); } COMPAT_TEST(backup_on_top_of_existing_file) @@ -2010,7 +2010,7 @@ static void no_backup_if_mismatch(const char* patch_path, const std::vector Date: Sat, 1 Aug 2026 16:10:10 +0200 Subject: [PATCH 2/4] tests: check the result of _wmktemp_s rather than errno _wmktemp_s reports a failure through its (positive) errno_t return value, so comparing it against zero from below never caught an error. --- tests/lib/src/test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/lib/src/test.cpp b/tests/lib/src/test.cpp index 3376ec3..dcb1f98 100644 --- a/tests/lib/src/test.cpp +++ b/tests/lib/src/test.cpp @@ -42,8 +42,9 @@ static std::string make_temp_directory() { #ifdef _WIN32 std::wstring path = L"patch-XXXXXX"; - if (_wmktemp_s(&path[0], path.size() + 1) < 0) - throw std::system_error(errno, std::generic_category(), "Unable to create temporary file name"); + const auto error = _wmktemp_s(&path[0], path.size() + 1); + if (error != 0) + throw std::system_error(error, std::generic_category(), "Unable to create temporary file name"); if (_wmkdir(path.c_str()) < 0) throw std::system_error(errno, std::generic_category(), "Unable to make temporary directory"); return to_narrow(path); From aa57a42697e2025ea0dd9237eee4a3162bb9508a Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sat, 1 Aug 2026 16:10:37 +0200 Subject: [PATCH 3/4] patch: pass the read-only check the permissions to inspect The check fetched the permissions of the output file itself, resolving the whole path by name once more. Instead let the caller say what it found. --- src/patch.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/patch.cpp b/src/patch.cpp index 7b3e1e0..60cc2fe 100644 --- a/src/patch.cpp +++ b/src/patch.cpp @@ -456,9 +456,8 @@ class DeferredWriter { std::vector m_deferred_writes; }; -static bool refuse_read_only_file(std::ostream& out, const Options& options, const std::string& output_file) +static bool refuse_read_only_file(std::ostream& out, const Options& options, const std::string& output_file, filesystem::perms permissions) { - const auto permissions = filesystem::get_permissions(output_file); const auto write_perm_mask = filesystem::perms::group_write | filesystem::perms::owner_write | filesystem::perms::others_write; const bool is_read_only = permissions != filesystem::perms::unknown && (permissions & write_perm_mask) == filesystem::perms::none; @@ -569,7 +568,7 @@ static bool process_parsed_patch(const Options& options, DeferredWriter& deferre return true; } - if (refuse_read_only_file(out, options, output_file)) { + if (refuse_read_only_file(out, options, output_file, filesystem::get_permissions(output_file))) { parse_body_if_needed(); refuse_to_patch(out, mode, output_file, patch, options); return true; From 5c389f2102686a52b81a2b2d875f5b31b6773a61 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sat, 1 Aug 2026 16:11:39 +0200 Subject: [PATCH 4/4] system: do not treat a drive-relative path as absolute "C:\foo" names the root of drive C, but "C:foo" names foo relative to whatever the current directory on C happens to be. Classifying the second as absolute means it is resolved from a root it does not name. Rejecting absolute names was also the only thing keeping a drive-relative name out of a patch, so is_safe_patch_path() now refuses a colon itself. A colon never appears in a Windows file name, since "file:stream" names an alternate data stream of "file" rather than a file of that name. --- src/parser.cpp | 7 +++++++ src/system.cpp | 4 ++-- tests/test_strip.cpp | 6 +++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/parser.cpp b/src/parser.cpp index 7b332a5..fdad437 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -1142,6 +1142,13 @@ bool is_safe_patch_path(const std::string& path) if (filesystem::is_absolute(path)) return false; +#ifdef _WIN32 + // A colon never appears in a Windows file name. "C:foo" names foo relative to the current + // directory of drive C, and "file:stream" names an alternate data stream of "file". + if (path.find(':') != std::string::npos) + return false; +#endif + // Reject any '..' component, including one which would resolve back inside the working directory for (std::size_t begin = 0; begin < path.size();) { std::size_t end = begin; diff --git a/src/system.cpp b/src/system.cpp index fefe012..1ce3588 100644 --- a/src/system.cpp +++ b/src/system.cpp @@ -298,8 +298,8 @@ bool is_absolute(const std::string& path) return true; #ifdef _WIN32 - // A drive letter, with or without a following separator. - return path.size() >= 2 && path[1] == ':'; + // A drive letter, with a following separator. + return path.size() >= 3 && path[1] == ':' && is_seperator(path[2]); #else return false; #endif diff --git a/tests/test_strip.cpp b/tests/test_strip.cpp index 14ab5e6..d0f01a0 100644 --- a/tests/test_strip.cpp +++ b/tests/test_strip.cpp @@ -25,7 +25,9 @@ TEST(is_absolute_path) #ifdef _WIN32 EXPECT_TRUE(Patch::filesystem::is_absolute("\\windows\\file.txt")); EXPECT_TRUE(Patch::filesystem::is_absolute("C:\\file.txt")); - EXPECT_TRUE(Patch::filesystem::is_absolute("C:file.txt")); + EXPECT_TRUE(Patch::filesystem::is_absolute("C:/file.txt")); + EXPECT_FALSE(Patch::filesystem::is_absolute("C:file.txt")); + EXPECT_FALSE(Patch::filesystem::is_absolute("C:")); #else EXPECT_FALSE(Patch::filesystem::is_absolute("C:\\file.txt")); #endif @@ -55,6 +57,8 @@ TEST(is_safe_patch_path) EXPECT_FALSE(Patch::is_safe_patch_path("\\windows\\file.txt")); EXPECT_FALSE(Patch::is_safe_patch_path("C:\\file.txt")); EXPECT_FALSE(Patch::is_safe_patch_path("C:file.txt")); + EXPECT_FALSE(Patch::is_safe_patch_path("file.txt:stream")); + EXPECT_FALSE(Patch::is_safe_patch_path("dir/file.txt:stream")); EXPECT_FALSE(Patch::is_safe_patch_path("dir\\..\\file.txt")); #endif }