From 11ce0dca12760b1e727ef0bc6a7b02f0c89b50c4 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 23 Sep 2026 07:50:51 +0800 Subject: [PATCH 1/4] graph: refuse a node whose parent id names no file node add_file_node took a non-zero parent_dir that get_file_node could not resolve (an id never minted, a command-tagged id, a slot holding another node) and filed the node's path under the source root, while still inserting its name into dir_children[node_id::index(parent_dir)]. With the tag stripped, that bucket belongs to some other node; with an index past the new node's slot the write is out of bounds, since dir_children is resized only to the new node's index and Vec::operator[] is unchecked. The new test observed that as a SIGSEGV before the fix; the issue had predicted it from the source without running it. The parent is now resolved before anything is interned or minted, and an unresolvable one is InvalidNodeId, the code add_edge already returns for an unresolvable endpoint. get_file_node is the predicate rather than validate_node_id, because the latter accepts a live command id. No production caller reaches the error: every parent_dir comes from ensure_file_node or find_by_dir_name on the same graph, or is 0, and the index loader builds an Index, never a Graph. A parent that resolves but is not a directory is still accepted; no caller passes one. Verified red-then-green: before the fix the three sections SIGSEGV, accept the command-id parent, and return DuplicateNode instead of InvalidNodeId; after, make check passes (182542 assertions, 957 cases). Ref: https://github.com/typeless/putup/issues/492 Co-Authored-By: Claude Opus 5.5 (1M context) --- src/graph/dag.cpp | 15 ++++++++------- test/unit/test_graph.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 197ea98b..6cc87444 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -88,15 +88,16 @@ auto validate_node_id(Graph const& graph, NodeId id) -> bool auto add_file_node(Graph& graph, FileNode node) -> Result { + auto parent_path = PathId::SourceRoot; + if (node.parent_dir != 0) { + auto const* parent = get_file_node(std::as_const(graph), node.parent_dir); + if (!parent) { + return make_error(ErrorCode::InvalidNodeId, "Invalid parent directory node ID"); + } + parent_path = parent->path_id; + } auto const parent_idx = node_id::index(node.parent_dir); if (!is_empty(node.name)) { - auto parent_path = PathId::SourceRoot; - if (node.parent_dir != 0) { - auto const* parent = get_file_node(std::as_const(graph), node.parent_dir); - if (parent) { - parent_path = parent->path_id; - } - } node.path_id = graph.paths.intern(parent_path, node.name); } diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index b9d90361..6f1347a2 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1682,6 +1682,44 @@ TEST_CASE("add_file_node refuses a second node at an occupied path", "[graph][pa } } +TEST_CASE("add_file_node refuses a parent id that names no file node", "[graph][path_pool]") +{ + auto bs = make_build_graph(); + auto& g = bs.graph; + + auto src_dir = add_file_node(g, FileNode { .type = NodeType::Directory, .name = intern("src") }); + REQUIRE(src_dir.has_value()); + auto root_foo = add_file_node(g, FileNode { .name = intern("foo.c") }); + REQUIRE(root_foo.has_value()); + auto const next_id = g.next_file_id; + + SECTION("an id no node was minted under is an error") + { + auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = pup::NodeId { 999 } }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(g.next_file_id == next_id); + REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "bar.c") == std::nullopt); + } + + SECTION("a command id is an error even when its index matches a directory") + { + auto const cmd = pup::node_id::make_command(pup::node_id::index(*src_dir)); + auto orphan = add_file_node(g, FileNode { .name = intern("bar.c"), .parent_dir = cmd }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(find_by_dir_name(g, *src_dir, "bar.c") == std::nullopt); + } + + SECTION("the refusal leaves the source root's entry of the same name alone") + { + auto orphan = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = pup::NodeId { 999 } }); + REQUIRE_FALSE(orphan.has_value()); + REQUIRE(orphan.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(find_by_dir_name(g, pup::NodeId { 0 }, "foo.c") == *root_foo); + } +} + TEST_CASE("FileNode path_id populated by add_file_node", "[graph][path_pool]") { auto bs = make_build_graph(); From dd4770b72692c919e9424e4d3ce6b3744d71bc5c Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 23 Sep 2026 08:24:41 +0800 Subject: [PATCH 2/4] glob: match files, never directories glob_expand added every directory entry whose name matched the pattern, in both the flat and the recursive walk, and the builder then bound each matched directory as a File node. A rule globbing `dir/**` took the subdirectories of dir as inputs typed File, and anything later created under one of them was parented to a file. Upstream expands a rule's glob in nl_add_path through tup_db_select_node_dir_glob with directories excluded, so only normal and generated files match. glob_expand now skips an entry the directory read reports as a directory; the recursive walk still descends into it. Found when the directory check in the next commit refused putup's own test runner, whose shards take `| ../e2e/fixtures/**`. A symlink to a directory still matches, as before: the POSIX read types it Symlink. Verified red-then-green: `gdir_*` returned { "gdir_sub", "gdir_top.txt" } and `**` returned the subdirectory too; both now return the files only. Bootstrap scripts regenerate unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- spec/requirements/glob-expansion.ears.md | 8 +++++ src/parser/glob.cpp | 4 +-- test/unit/test_glob.cpp | 37 ++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/spec/requirements/glob-expansion.ears.md b/spec/requirements/glob-expansion.ears.md index 0872d123..63d2afa9 100644 --- a/spec/requirements/glob-expansion.ears.md +++ b/spec/requirements/glob-expansion.ears.md @@ -90,6 +90,14 @@ Which paths a pattern names. putup shall expand a glob to both the source files on disk and the files produced by the project's rules. +### REQ-GLOB-FILES-ONLY + +- conformance: tup-conformant +- reference: upstream `nl_add_path` expands a rule's glob through `tup_db_select_node_dir_glob` with directories excluded, so only normal and generated files match +- discharge: test "Scenario: glob expansion matches files and never directories" + +putup shall expand a glob to the files it matches and never to a directory whose name it matches. + ### REQ-GLOB-DEDUP - conformance: unclassified diff --git a/src/parser/glob.cpp b/src/parser/glob.cpp index 1f765d4d..4141a04e 100644 --- a/src/parser/glob.cpp +++ b/src/parser/glob.cpp @@ -232,7 +232,7 @@ auto glob_expand( if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') { return false; } - if (glob.matches(rel_path)) { + if (!entry.is_dir && glob.matches(rel_path)) { auto result_id = dir_part.empty() ? pool.intern(rel_path) : pup::path::join(dir_part, rel_path); results.push_back(result_id); } @@ -246,7 +246,7 @@ auto glob_expand( if (!options.include_hidden && !name_sv.empty() && name_sv[0] == '.') { continue; } - if (glob.matches(name_sv)) { + if (!entry.is_dir && glob.matches(name_sv)) { auto result_id = dir_part.empty() ? pool.intern(name_sv) : pup::path::join(dir_part, name_sv); results.push_back(result_id); } diff --git a/test/unit/test_glob.cpp b/test/unit/test_glob.cpp index 6af772ae..7b7e067b 100644 --- a/test/unit/test_glob.cpp +++ b/test/unit/test_glob.cpp @@ -313,3 +313,40 @@ SCENARIO("glob expansion orders matches by path, not by interning order", "[glob } } } + +SCENARIO("glob expansion matches files and never directories", "[glob]") +{ + GIVEN("a directory holding a file and a subdirectory that itself holds a file") + { + auto tmp = TempDir {}; + fs::create_directory(tmp.path() / "gdir_sub"); + tmp.create_file("gdir_top.txt"); + tmp.create_file("gdir_sub/gdir_leaf.txt"); + + auto expanded = [&](std::string_view pattern) { + auto matches = glob_expand(pattern, tmp.path().string()); + REQUIRE(matches.has_value()); + auto result = std::vector {}; + for (auto id : *matches) { + result.push_back(sv(id)); + } + return result; + }; + + WHEN("a pattern that the subdirectory's name also matches is expanded") + { + THEN("only the file is a match") + { + REQUIRE(expanded("gdir_*") == std::vector { "gdir_top.txt" }); + } + } + + WHEN("a recursive pattern is expanded") + { + THEN("the files at every depth match and the subdirectory does not") + { + REQUIRE(expanded("**") == std::vector { "gdir_sub/gdir_leaf.txt", "gdir_top.txt" }); + } + } + } +} From 9274c9c2c080b90d14e0255473cd083a612d4e74 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 23 Sep 2026 08:24:52 +0800 Subject: [PATCH 3/4] graph: refuse a path under a node that is not a directory A Tupfile could put an output or a group under a source file, a generated file or a group. add_file_node checked only that the parent id resolved, and ensure_file_node returns whatever node a path already has when asked for a directory. So `: sub |> ... ` followed by an output `sub/x.o` ran the command and failed in the shell, and a group `sub/` under the file sub built with no diagnostic at all. Upstream refuses all three in find_dir_tupid_dt_pg ("Unable to output to a different directory because 'sub' is a normal file"). add_file_node is the one place a node acquires a parent, so it now refuses any parent that is not a Directory or GeneratedDir, naming the path and what occupies it. The code is InvalidNodeId, as for an unresolvable parent: the id is not a valid parent. The check trusts node types, and one producer typed them wrong: resolve_input_node created a File node for any existing source or config path, directories included. With the check alone, `: sub |> ls %f ...` over a real directory followed by any path under sub was refused where upstream accepts it. It now asks the disk whether the path is a directory. Incremental behaviour of a directory input is unchanged: against the installed pre-change binary, a build, a null build and an edit of a file under the directory rebuild the same commands. Not covered: putup learns a source path's type only once a rule names it, so an output named under a file before any rule reads that file creates a Directory node and still fails in the shell. Upstream knows from its scan. Verified red-then-green: before the check, the three e2e cases ran the command, ran it and failed, and built; before the input fix, the directory case was refused. make check passes (182569 assertions in 959 test cases), and a cross-model review's 20 Tupfile fixtures exit the same under the old and the new binary. Co-Authored-By: Claude Opus 5.5 (1M context) --- spec/requirements/output-paths.ears.md | 10 +++ src/graph/builder.cpp | 4 +- src/graph/dag.cpp | 12 ++++ .../Tupfile.fixture | 1 + .../output_under_non_directory/Tupfile.ini | 0 .../output_under_non_directory/srcdir/keep.c | 1 + .../fixtures/output_under_non_directory/sub | 1 + .../output_under_non_directory/tup.config | 0 test/unit/test_e2e.cpp | 65 +++++++++++++++++++ test/unit/test_graph.cpp | 26 +++++++- 10 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 test/e2e/fixtures/output_under_non_directory/Tupfile.fixture create mode 100644 test/e2e/fixtures/output_under_non_directory/Tupfile.ini create mode 100644 test/e2e/fixtures/output_under_non_directory/srcdir/keep.c create mode 100644 test/e2e/fixtures/output_under_non_directory/sub create mode 100644 test/e2e/fixtures/output_under_non_directory/tup.config diff --git a/spec/requirements/output-paths.ears.md b/spec/requirements/output-paths.ears.md index ca73c39f..2e1b2121 100644 --- a/spec/requirements/output-paths.ears.md +++ b/spec/requirements/output-paths.ears.md @@ -54,6 +54,16 @@ How far out of the tree a rule may write. 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. +### REQ-OUTPUT-UNDER-DIRECTORY + +- conformance: tup-conformant +- reference: upstream refuses the same class in `find_dir_tupid_dt_pg` (`src/tup/create_name_file.c`), which accepts only a directory or a generated directory as a path component of an output or a group and reports "Unable to output to a different directory"; upstream knows a source path's type from its scan, where putup learns it only once a rule names the path, so an output named before any rule reads the file is not refused +- discharge: test "add_file_node refuses a parent that is not a directory node" +- discharge: test "Scenario: A rule cannot put an output or a group under a path that is not a directory" + +If a rule names an output or a group under a path that the graph holds as a file, a generated file +or a group, then putup shall reject the Tupfile and name that path and what it is. + ## Group: canonicality Which of a path's spellings the record carries. diff --git a/src/graph/builder.cpp b/src/graph/builder.cpp index d5dbad46..cf5d72c5 100644 --- a/src/graph/builder.cpp +++ b/src/graph/builder.cpp @@ -387,14 +387,14 @@ auto resolve_input_node( auto source_path = pool.get(pup::path::join(str(ctx.options.source_root), normalized_path)); if (pup::platform::exists(source_path)) { auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot); - return ensure_file_node(ctx.state->graph, path_id, NodeType::File); + return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(source_path) ? NodeType::Directory : NodeType::File); } if (!is_empty(ctx.options.config_root) && str(ctx.options.config_root) != str(ctx.options.source_root)) { auto config_path_sv = pool.get(pup::path::join(str(ctx.options.config_root), normalized_path)); if (pup::platform::exists(config_path_sv)) { auto path_id = ctx.state->graph.paths.intern_path(normalized_path, pool, PathId::SourceRoot); - return ensure_file_node(ctx.state->graph, path_id, NodeType::File); + return ensure_file_node(ctx.state->graph, path_id, pup::platform::is_directory(config_path_sv) ? NodeType::Directory : NodeType::File); } } diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index 6cc87444..fbc30363 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -94,6 +94,18 @@ auto add_file_node(Graph& graph, FileNode node) -> Result if (!parent) { return make_error(ErrorCode::InvalidNodeId, "Invalid parent directory node ID"); } + if (parent->type != NodeType::Directory && parent->type != NodeType::GeneratedDir) { + auto const parent_spelling = global_pool().get(materialize_path(graph, parent->path_id)); + auto err = Buf {}; + err.fmt( + "Unable to create '{}/{}' because the {} '{}' is not a directory", + parent_spelling, + global_pool().get(node.name), + node_type_name(parent->type), + parent_spelling + ); + return make_error(ErrorCode::InvalidNodeId, err.view()); + } parent_path = parent->path_id; } auto const parent_idx = node_id::index(node.parent_dir); diff --git a/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture b/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture new file mode 100644 index 00000000..618ace6f --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/Tupfile.fixture @@ -0,0 +1 @@ +: |> echo hi > %o |> ok.txt diff --git a/test/e2e/fixtures/output_under_non_directory/Tupfile.ini b/test/e2e/fixtures/output_under_non_directory/Tupfile.ini new file mode 100644 index 00000000..e69de29b diff --git a/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c b/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c new file mode 100644 index 00000000..b4c38db6 --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/srcdir/keep.c @@ -0,0 +1 @@ +int keep; diff --git a/test/e2e/fixtures/output_under_non_directory/sub b/test/e2e/fixtures/output_under_non_directory/sub new file mode 100644 index 00000000..59768066 --- /dev/null +++ b/test/e2e/fixtures/output_under_non_directory/sub @@ -0,0 +1 @@ +not a directory diff --git a/test/e2e/fixtures/output_under_non_directory/tup.config b/test/e2e/fixtures/output_under_non_directory/tup.config new file mode 100644 index 00000000..e69de29b diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index aac7da5c..77b48fdc 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -15893,3 +15893,68 @@ SCENARIO("Numbered input flags name the basename and the basename without extens } } } + +SCENARIO("A rule cannot put an output or a group under a path that is not a directory", "[e2e][parse]") +{ + GIVEN("a project whose source tree holds a file named sub and a directory named srcdir") + { + auto f = E2EFixture { "output_under_non_directory" }; + + WHEN("a rule reads the source directory and others read and write paths inside it") + { + f.write_file("Tupfile", ": srcdir |> ls %f > %o |> list.txt\n: srcdir/keep.c |> cp %f %o |> keep.o\n: |> echo x > %o |> srcdir/x.o\n"); + auto result = f.build(); + + THEN("the build succeeds") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("srcdir/x.o") == "x\n"); + } + } + + WHEN("a rule reads the source file and another outputs into it as if it were a directory") + { + f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt\n: |> echo hi > %o |> sub/x.o\n"); + auto result = f.build(); + + THEN("the Tupfile is refused before any command runs") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); + REQUIRE(result.stdout_output.find("echo hi") == std::string::npos); + } + } + + WHEN("a rule outputs under another rule's output") + { + f.write_file("Tupfile", ": |> echo a > %o |> gen\n: |> echo b > %o |> gen/x\n"); + auto result = f.build(); + + THEN("the Tupfile is refused and names the generated file") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the generated file 'gen' is not a directory") != std::string::npos); + } + } + + WHEN("a rule puts a group under the source file") + { + f.write_file("Tupfile", ": sub |> cat %f > %o |> a.txt sub/\n: sub/ |> echo used > %o |> b.txt\n"); + auto result = f.build(); + + THEN("the Tupfile is refused where the build used to succeed") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("the file 'sub' is not a directory") != std::string::npos); + } + } + } +} diff --git a/test/unit/test_graph.cpp b/test/unit/test_graph.cpp index 6f1347a2..d1213389 100644 --- a/test/unit/test_graph.cpp +++ b/test/unit/test_graph.cpp @@ -1682,7 +1682,7 @@ TEST_CASE("add_file_node refuses a second node at an occupied path", "[graph][pa } } -TEST_CASE("add_file_node refuses a parent id that names no file node", "[graph][path_pool]") +TEST_CASE("add_file_node refuses a parent that is not a directory node", "[graph][path_pool]") { auto bs = make_build_graph(); auto& g = bs.graph; @@ -1711,6 +1711,30 @@ TEST_CASE("add_file_node refuses a parent id that names no file node", "[graph][ REQUIRE(find_by_dir_name(g, *src_dir, "bar.c") == std::nullopt); } + SECTION("a parent that is a file rather than a directory is an error naming what it is") + { + auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *root_foo }); + REQUIRE_FALSE(child.has_value()); + REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId); + REQUIRE(pup::global_pool().get(child.error().message).find("the file 'foo.c' is not a directory") != std::string_view::npos); + REQUIRE(g.next_file_id == next_id); + } + + SECTION("a parent that is a group is an error") + { + auto group = add_file_node(g, FileNode { .type = NodeType::Group, .name = intern(""), .parent_dir = *src_dir }); + REQUIRE(group.has_value()); + auto child = add_file_node(g, FileNode { .name = intern("x.o"), .parent_dir = *group }); + REQUIRE_FALSE(child.has_value()); + REQUIRE(child.error().code == pup::ErrorCode::InvalidNodeId); + } + + SECTION("the build root is a directory a node may sit under") + { + auto child = add_file_node(g, FileNode { .type = NodeType::Generated, .name = intern("x.o"), .parent_dir = pup::BUILD_ROOT_ID }); + REQUIRE(child.has_value()); + } + SECTION("the refusal leaves the source root's entry of the same name alone") { auto orphan = add_file_node(g, FileNode { .name = intern("foo.c"), .parent_dir = pup::NodeId { 999 } }); From 1fd6b5ade56f5eb757c1dd313404af86a24840dd Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 23 Sep 2026 08:24:59 +0800 Subject: [PATCH 4/4] graph: delete resolve_phi_node, which nothing calls It returned INVALID_NODE_ID when the phi or its condition did not resolve, and INVALID_NODE_ID is also SOURCE_ROOT_ID, so a failed lookup read as the source root. No source, header or test calls it; it had external linkage with no declaration, and DESIGN.md listed it. Deleting it removes the ambiguous default instead of re-encoding it. add_phi_node and get_phi_node have no production caller either, but they are the phi model DESIGN.md describes, so they stay. Co-Authored-By: Claude Opus 5.5 (1M context) --- DESIGN.md | 1 - src/graph/dag.cpp | 15 --------------- 2 files changed, 16 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index f73fc106..c84bf4b5 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -712,7 +712,6 @@ auto get_phi_node(Graph&, id) -> PhiNode*; // nullptr for non-phi ID // Phi-node model helpers auto add_condition_node(Graph&, ConditionNode) -> Result; auto add_phi_node(Graph&, PhiNode) -> Result; -auto resolve_phi_node(Graph const&, phi_id) -> NodeId; // Returns active output auto is_guard_satisfied(Graph const&, NodeId) -> bool; // String access helpers (resolve StringId -> string_view) diff --git a/src/graph/dag.cpp b/src/graph/dag.cpp index fbc30363..e2e8d9d8 100644 --- a/src/graph/dag.cpp +++ b/src/graph/dag.cpp @@ -507,21 +507,6 @@ auto get_phi_node(Graph& graph, NodeId id) -> PhiNode* return const_cast(get_phi_node(std::as_const(graph), id)); } -auto resolve_phi_node(Graph const& graph, NodeId phi_id) -> NodeId -{ - auto const* phi = get_phi_node(graph, phi_id); - if (!phi) { - return INVALID_NODE_ID; - } - - auto const* cond = get_condition_node(graph, phi->condition); - if (!cond) { - return INVALID_NODE_ID; - } - - return cond->current_value ? phi->then_output : phi->else_output; -} - auto is_guard_satisfied(Graph const& graph, NodeId id) -> bool { auto const* cmd = get_command_node(graph, id);