diff --git a/include/pup/index/format.hpp b/include/pup/index/format.hpp index e334c683..ee0c14d8 100644 --- a/include/pup/index/format.hpp +++ b/include/pup/index/format.hpp @@ -89,7 +89,7 @@ inline constexpr auto INDEX_MAGIC = std::array { 'P', 'U', 'P', 'I' }; /// `RawFileEntry::name_offset` is semantics-bearing on the same terms: `read_prior_paths` composes /// it into the paths `clean`/`distclean` delete and `reject_shadowed_sources` refuses a build over, /// so a name this reader cannot reproduce fails the record rather than reading as empty (#381). -inline constexpr auto INDEX_VERSION = std::uint32_t { 22 }; +inline constexpr auto INDEX_VERSION = std::uint32_t { 23 }; /// The oldest version whose `RawHeader` and `RawFileEntry` bytes mean what today's mean, so a /// record that old still says which paths it recorded as sources and which as generated even diff --git a/spec/requirements/output-paths.ears.md b/spec/requirements/output-paths.ears.md index 7ec9701f..d86d46c2 100644 --- a/spec/requirements/output-paths.ears.md +++ b/spec/requirements/output-paths.ears.md @@ -17,6 +17,15 @@ requirement. Upstream tup answers both in its parser, which is the enforcement p follows — establishing the rule once, where the path is read, rather than at each site that later acts on a recorded path. +"Resolves" is answered by the platform's own path law, which is the only sense the phrase ever had. +A backslash separates on Windows and is an ordinary character in a POSIX filename, so `..\victim.txt` +names a location above the build root on one and a single file on the other, and both answers are +this area's (issue #388). The same divergence already held for `C:\victim.txt`; what #388 added was +the spellings that carry no `/` at all — `..\`, the UNC `\\host\share\`, and the drive-relative +`C:victim.txt` — each of which reached the record unrecognised until `pup::path` was taught the +separator and root forms the platform's own file APIs already use. Both sides of the divergence are +pinned, because a rule pinned on one platform reads as a defect report against the other. + The second is which path is recorded. Every reader downstream — the deletion pass, the source-shadow guard, the record's own source/generated split — compares recorded paths against each other, and two spellings of one file defeat all of them. So an output is recorded by where it lands, not by how it @@ -38,6 +47,9 @@ How far out of the tree a rule may write. - discharge: test "GraphBuilder rejects an output above the build root under an unsatisfied guard" - discharge: test "GraphBuilder rejects an absolute output path" - discharge: test "Scenario: A rule writing above the build root fails the build instead of overwriting the file there" +- discharge: test "GraphBuilder rejects a backslash escape on Windows and names the file on POSIX" +- discharge: test "GraphBuilder rejects a UNC output on Windows and names the file on POSIX" +- discharge: test "GraphBuilder rejects a drive-relative output on Windows and names the file on POSIX" If a rule declares an output whose path does not resolve to a location inside the build root, then putup shall reject the Tupfile and name that path, whether or not the rule's guards are satisfied. @@ -52,6 +64,7 @@ Which of a path's spellings the record carries. - reference: upstream resolves each path element to a directory node rather than comparing strings, which has the same effect; not read closely enough to claim equivalence - discharge: test "GraphBuilder accepts a parent reference that stays inside the tree" - discharge: test "Scenario: A parent reference inside the tree is recorded canonically" +- discharge: test "GraphBuilder records a backslash output as two components on Windows and one on POSIX" While recording a rule's output, putup shall record the path the output resolves to rather than the path as the rule spelled it. diff --git a/src/core/path.cpp b/src/core/path.cpp index 9f6f4d38..8014db1e 100644 --- a/src/core/path.cpp +++ b/src/core/path.cpp @@ -17,30 +17,56 @@ namespace pup::path { namespace { -// The prefix `..` cannot escape and normalization reproduces verbatim: 1 for "/", 3 for "C:/". +// Windows spells a separator both ways; POSIX spells names with either byte, so `\` is a name byte. +#ifdef _WIN32 +constexpr auto separators = std::string_view { "/\\" }; +#else +constexpr auto separators = std::string_view { "/" }; +#endif + +auto is_separator(char c) -> bool +{ + return separators.find(c) != std::string_view::npos; +} + +// The prefix `..` cannot escape and normalization reproduces verbatim: 1 "/", 3 "C:/", 2 "//" or "C:". auto root_length(std::string_view p) -> std::size_t { if (p.empty()) { return 0; } - if (p[0] == '/') { - return 1; - } #ifdef _WIN32 - if (p.size() >= 3 && p[1] == ':' && (p[2] == '/' || p[2] == '\\')) { + if (p.size() >= 2 && is_separator(p[0]) && is_separator(p[1])) { + return 2; + } + if (p.size() >= 2 && p[1] == ':') { auto c = p[0]; if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')) { - return 3; + return (p.size() >= 3 && is_separator(p[2])) ? 3 : 2; } } #endif + if (is_separator(p[0])) { + return 1; + } return 0; } +// A bare drive prefix names no directory to separate from: "C:x" is not the file "C:/x". +auto separates_from_child(std::string_view p) -> bool +{ + return !p.empty() && (is_separator(p.back()) || (p.back() == ':' && is_root(p))); +} + auto append_root(Buf& out, std::string_view p, std::size_t root_len) -> void { - out.append(p.substr(0, root_len - 1)); - out += '/'; + auto const root = p.substr(0, root_len); + for (auto c : root) { + out += is_separator(c) ? '/' : c; + } + if (!separates_from_child(root)) { + out += '/'; + } } // Used only by relative()'s precondition assertion, which a release build compiles out. @@ -64,7 +90,7 @@ auto join(std::string_view a, std::string_view b) -> StringId auto buf = Buf {}; buf.append(a); - if (a.back() != '/') { + if (!separates_from_child(a)) { buf += '/'; } buf.append(b); @@ -83,16 +109,16 @@ auto parent(std::string_view p) -> std::string_view } auto end = p.size(); - while (end > root_len && end > 1 && p[end - 1] == '/') { + while (end > root_len && end > 1 && is_separator(p[end - 1])) { --end; } if (end <= root_len) { return p.substr(0, root_len); } - auto pos = p.rfind('/', end - 1); + auto pos = p.find_last_of(separators, end - 1); if (pos == std::string_view::npos) { - return {}; + return p.substr(0, root_len); } if (pos < root_len) { return p.substr(0, root_len); @@ -105,9 +131,9 @@ auto filename(std::string_view p) -> std::string_view if (p.empty()) { return {}; } - auto pos = p.rfind('/'); + auto pos = p.find_last_of(separators); if (pos == std::string_view::npos) { - return p; + return p.substr(root_length(p)); } return p.substr(pos + 1); } @@ -155,6 +181,13 @@ auto is_normal(std::string_view p) -> bool return true; } +#ifdef _WIN32 + // normalize emits only '/', so the other separator spelling is by definition not normal. + if (p.find('\\') != std::string_view::npos) { + return false; + } +#endif + auto const root_len = root_length(p); auto const body = p.substr(root_len); if (!body.empty() && body.back() == '/') { @@ -192,7 +225,7 @@ auto normalize(std::string_view p) -> StringId auto absolute = root_len != 0; while (start < p.size()) { - auto end = p.find('/', start); + auto end = p.find_first_of(separators, start); if (end == std::string_view::npos) { end = p.size(); } @@ -248,7 +281,7 @@ auto relative(std::string_view target, std::string_view base) -> StringId auto parts = Vec {}; auto start = std::size_t { 0 }; while (start < p.size()) { - auto end = p.find('/', start); + auto end = p.find_first_of(separators, start); if (end == std::string_view::npos) { end = p.size(); } diff --git a/src/platform/file_io-win32.cpp b/src/platform/file_io-win32.cpp index 9d8c07b3..cc7fd519 100644 --- a/src/platform/file_io-win32.cpp +++ b/src/platform/file_io-win32.cpp @@ -423,6 +423,10 @@ auto create_directories(std::string_view path) -> Result if (path.empty()) { return {}; } + // A drive root answers neither success nor ERROR_ALREADY_EXISTS, so never ask (#388). + if (is_directory(path)) { + return {}; + } auto par = pup::path::parent(path); if (!par.empty() && par != path) { auto r = create_directories(par); @@ -433,6 +437,7 @@ auto create_directories(std::string_view path) -> Result auto wpath = to_wide(path); if (!CreateDirectoryW(wpath.c_str(), nullptr)) { auto err = GetLastError(); + // A file in the way lands here too, so this succeeds where the POSIX half returns -ENOTDIR. if (err != ERROR_ALREADY_EXISTS) { return make_error(ErrorCode::IoError, make_err_msg("Failed to create directory: ", path, err)); } diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index d23c4289..d8b78181 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -2101,6 +2101,87 @@ TEST_CASE("GraphBuilder rejects an absolute output path", "[builder][hierarchy]" CHECK(rejected_inside.error().msg().find("hierarchy") != std::string_view::npos); } +/// One spelling of the escape per test, each answered by the platform's own path law (#388): +/// on Windows these name a location outside the build root, on POSIX they name one file whose +/// name contains a backslash. Pinning only the Windows half would read as a bug report on POSIX. +TEST_CASE("GraphBuilder rejects a backslash escape on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> ..\\victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +TEST_CASE("GraphBuilder rejects a UNC output on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> \\\\host\\share\\victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +TEST_CASE("GraphBuilder rejects a drive-relative output on Windows and names the file on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto result = add_tupfile_from_source(fixture, ": |> echo x > %o |> C:victim.txt\n"); + +#ifdef _WIN32 + REQUIRE_FALSE(result.has_value()); + CHECK(result.error().msg().find("hierarchy") != std::string_view::npos); +#else + CHECK(result.has_value()); +#endif +} + +/// The record's key and the file the OS creates were two different paths for every backslash +/// spelling on Windows: one opaque component here, two components on disk (#388). +TEST_CASE("GraphBuilder records a backslash output as two components on Windows and one on POSIX", "[builder][hierarchy]") +{ + auto fixture = BuilderTestFixture {}; + + auto bs = make_build_graph(); + auto vars = VarDb {}; + auto ctx = EvalContext { .vars = &vars }; + + auto options = BuilderOptions { + .source_root = intern(fixture.root_str()), + .config_root = intern(fixture.root_str()), + .output_root = pup::StringId::Empty, + .config_path = pup::StringId::Empty, + .expand_globs = false, + .validate_inputs = false, + }; + auto builder_state = make_builder(options); + + auto parse_result = parse_tupfile(": |> echo x > %o |> sub\\gen.txt\n", fixture.tupfile_path("")); + REQUIRE(parse_result.success()); + + REQUIRE(add_tupfile(bs, parse_result.tupfile, ctx, builder_state).has_value()); + + auto generated = nodes_of_type(bs.graph, NodeType::Generated); + REQUIRE(generated.size() == 1); +#ifdef _WIN32 + CHECK(sv(get_full_path(bs.graph, generated[0])) == "sub/gen.txt"); + CHECK(sv(get(bs.graph, generated[0])) == "gen.txt"); +#else + CHECK(sv(get_full_path(bs.graph, generated[0])) == "sub\\gen.txt"); + CHECK(sv(get(bs.graph, generated[0])) == "sub\\gen.txt"); +#endif +} + TEST_CASE("GraphBuilder accepts a parent reference that stays inside the tree", "[builder][hierarchy]") { auto fixture = BuilderTestFixture {}; diff --git a/test/unit/test_path.cpp b/test/unit/test_path.cpp index 186f8921..423cc407 100644 --- a/test/unit/test_path.cpp +++ b/test/unit/test_path.cpp @@ -176,7 +176,9 @@ TEST_CASE("path::is_root", "[path]") REQUIRE(is_root("C:/")); REQUIRE(is_root("C:\\")); REQUIRE_FALSE(is_root("C:/a")); - REQUIRE_FALSE(is_root("C:")); + // "C:" is the whole prefix a drive-relative path is rooted on, so it is one (#388). + REQUIRE(is_root("C:")); + REQUIRE(is_root("//")); #endif } @@ -305,6 +307,69 @@ TEST_CASE("path::relative", "[path]") } } +/// A drive prefix is the one root that contains no separator byte, so the splitters cannot honour +/// it incidentally the way `find_last_of` honours "/", "C:/" and "//". Each function answers for +/// itself, and on POSIX "C:a" is an ordinary relative name with a colon in it (#388). +TEST_CASE("path::filename answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(filename("C:a") == "a"); + REQUIRE(filename("C:") == ""); +#else + REQUIRE(filename("C:a") == "C:a"); + REQUIRE(filename("C:") == "C:"); +#endif +} + +TEST_CASE("path::parent answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(parent("C:a") == "C:"); +#else + REQUIRE(parent("C:a") == ""); +#endif +} + +TEST_CASE("path::join answers a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(sv(join("C:", "a")) == "C:a"); +#else + REQUIRE(sv(join("C:", "a")) == "C:/a"); +#endif +} + +/// stem and extension read the name through filename, so the clamp reaches them without their +/// own edit -- pinned rather than argued. +TEST_CASE("path::stem and extension answer a separator-less root", "[path]") +{ +#ifdef _WIN32 + REQUIRE(stem("C:a.txt") == "a"); +#else + REQUIRE(stem("C:a.txt") == "C:a"); +#endif + REQUIRE(extension("C:a.txt") == ".txt"); +} + +#ifndef _WIN32 + +/// The other half of the asymmetry #388 settled: a backslash is an ordinary character in a POSIX +/// filename, so none of these spellings names anything but a file whose name contains one. +TEST_CASE("path::treats a backslash as an ordinary filename character", "[path][posix]") +{ + REQUIRE(sv(normalize("..\\victim.txt")) == "..\\victim.txt"); + REQUIRE(sv(normalize("a\\b")) == "a\\b"); + REQUIRE(filename("a\\b") == "a\\b"); + REQUIRE(parent("a\\b") == ""); + REQUIRE(is_normal("a\\b")); + REQUIRE_FALSE(is_absolute("\\victim.txt")); + REQUIRE_FALSE(is_absolute("\\\\host\\share\\x")); + REQUIRE_FALSE(is_absolute("C:a")); + REQUIRE_FALSE(is_absolute("C:\\a")); +} + +#endif + #ifdef _WIN32 TEST_CASE("path::normalize keeps the drive root", "[path][windows]") @@ -351,12 +416,87 @@ TEST_CASE("path::normalize keeps the drive root", "[path][windows]") } } +/// `C:` and `C:a` are rooted on a drive, not under the build root, so they answer this the same +/// way `C:/a` does; the earlier `REQUIRE_FALSE` pinned the hole #388 was filed for. TEST_CASE("path::is_absolute on drive-absolute paths", "[path][windows]") { REQUIRE(is_absolute("C:/a")); REQUIRE(is_absolute("C:\\a")); - REQUIRE_FALSE(is_absolute("C:")); - REQUIRE_FALSE(is_absolute("C:a")); + REQUIRE(is_absolute("C:")); + REQUIRE(is_absolute("C:a")); +} + +TEST_CASE("path::normalize reads a backslash as a separator", "[path][windows]") +{ + SECTION("between names") + { + REQUIRE(sv(normalize("a\\b")) == "a/b"); + REQUIRE(sv(normalize("a\\b/c")) == "a/b/c"); + } + + SECTION("a parent reference spelled with a backslash resolves") + { + REQUIRE(sv(normalize("a\\..\\b")) == "b"); + REQUIRE(sv(normalize("..\\victim.txt")) == "../victim.txt"); + REQUIRE(sv(normalize("a\\..\\..\\victim.txt")) == "../victim.txt"); + } + + SECTION("dot segments and repeated separators") + { + REQUIRE(sv(normalize("a\\.\\\\b")) == "a/b"); + REQUIRE(sv(normalize("a\\")) == "a"); + } +} + +TEST_CASE("path::normalize keeps a UNC path rooted", "[path][windows]") +{ + REQUIRE(sv(normalize("\\\\host\\share\\victim.txt")) == "//host/share/victim.txt"); + REQUIRE(sv(normalize("\\\\host\\share")) == "//host/share"); + + // Where the parent references resolve to does not matter: staying rooted is what refuses it. + REQUIRE(is_absolute(sv(normalize("\\\\host\\share\\..\\..\\victim.txt")))); +} + +TEST_CASE("path::normalize keeps a drive-relative prefix unseparated", "[path][windows]") +{ + // "C:a" names a's location on C's own current directory; "C:/a" is a different file. + REQUIRE(sv(normalize("C:a\\b")) == "C:a/b"); + REQUIRE(sv(normalize("C:")) == "C:"); +} + +TEST_CASE("path::is_absolute on the remaining Windows root spellings", "[path][windows]") +{ + REQUIRE(is_absolute("\\\\host\\share\\x")); + REQUIRE(is_absolute("\\victim.txt")); + REQUIRE_FALSE(is_absolute("a\\b")); + REQUIRE_FALSE(is_absolute("..\\victim.txt")); +} + +TEST_CASE("path::parent and filename read a backslash as a separator", "[path][windows]") +{ + REQUIRE(parent("a\\b\\c") == "a\\b"); + REQUIRE(filename("a\\b\\c") == "c"); + REQUIRE(stem("a\\b.txt") == "b"); + REQUIRE(extension("a\\b.txt") == ".txt"); +} + +TEST_CASE("path::is_normal rejects a backslash-separated spelling", "[path][windows]") +{ + REQUIRE_FALSE(is_normal("a\\b")); + REQUIRE_FALSE(is_normal("a\\")); + REQUIRE(is_normal("a/b")); +} + +TEST_CASE("path::join does not double a backslash separator", "[path][windows]") +{ + REQUIRE(sv(join("a\\", "b")) == "a\\b"); + REQUIRE(sv(join("x", "\\\\host\\share\\b")) == "\\\\host\\share\\b"); + REQUIRE(sv(join("x", "C:b")) == "C:b"); +} + +TEST_CASE("path::relative reads a backslash as a separator", "[path][windows]") +{ + REQUIRE(sv(relative("a/b/c", "a/b")) == "c"); } TEST_CASE("path::parent on drive-absolute paths", "[path][windows]") @@ -396,7 +536,18 @@ auto check_normalize_laws(std::string const& p, std::string_view root) -> void REQUIRE(once == twice); + // Splitting a normal path and rejoining it reproduces it. This is what reaches the three + // functions that honour a root only by finding a separator inside it (#388). + REQUIRE(sv(join(parent(once), filename(once))) == once); + auto const absolute = is_absolute(p); + + // Stated apart from the rejoin above, which cannot see this: join("", x) returns x, so an + // under-clamped parent and the whole name it leaves behind cancel exactly (#388). + if (absolute) { + REQUIRE(is_absolute(parent(once))); + } + if (absolute) { REQUIRE(is_absolute(once)); REQUIRE(std::string_view { once }.starts_with(root)); @@ -465,5 +616,17 @@ TEST_CASE("path::normalize obeys its laws", "[path]") { check_laws_under_root("C:/"); } + + SECTION("drive-relative paths") + { + check_laws_under_root("C:"); + } + + // The root a UNC path normalizes onto. The authority that follows it is ordinary components, + // which is why the laws are stated against "//" and not against "//host/share" (#388). + SECTION("UNC-rooted paths") + { + check_laws_under_root("//"); + } #endif } diff --git a/test/unit/test_platform_file_io.cpp b/test/unit/test_platform_file_io.cpp index 9e9d9521..88210dc4 100644 --- a/test/unit/test_platform_file_io.cpp +++ b/test/unit/test_platform_file_io.cpp @@ -632,3 +632,46 @@ SCENARIO("canonical resolves a symlinked prefix before cancelling what follows i } } #endif + +/// Untagged so Windows CI, which filters `~[e2e]~[shell]`, is where this runs: a root always +/// exists, and only there does asking the OS to create one fail the build (#388). +TEST_CASE("create_directories succeeds for what already exists", "[platform][file_io]") +{ + auto const cwd_id = pup::platform::current_directory(); + REQUIRE(cwd_id.has_value()); + auto const cwd = std::string { pup::global_pool().get(*cwd_id) }; + + SECTION("an existing directory") + { + CHECK(create_directories(cwd).has_value()); + } + + SECTION("the root the directory sits under") + { + auto root = std::string_view { cwd }; + while (!pup::path::is_root(root)) { + auto const par = pup::path::parent(root); + if (par.empty() || par == root) { + break; + } + root = par; + } + REQUIRE(pup::path::is_root(root)); + INFO("root: " << root); + CHECK(create_directories(root).has_value()); + } + +#ifdef _WIN32 + SECTION("a backslash-spelled directory whose parent walk reaches the root") + { + auto spelled = cwd; + for (auto& c : spelled) { + if (c == '/') { + c = '\\'; + } + } + INFO("spelled: " << spelled); + CHECK(create_directories(spelled).has_value()); + } +#endif +}