Skip to content

Fix segfault on manifest include cycles#2746

Open
philwo wants to merge 1 commit intoninja-build:masterfrom
philwo:include-cycles
Open

Fix segfault on manifest include cycles#2746
philwo wants to merge 1 commit intoninja-build:masterfrom
philwo:include-cycles

Conversation

@philwo
Copy link
Copy Markdown

@philwo philwo commented Mar 19, 2026

Circular include/subninja directives (e.g. a.ninja includes b.ninja which includes a.ninja) caused unbounded recursion and a stack overflow. Track which files are currently being parsed and report an error when a cycle is detected.

Copilot AI review requested due to automatic review settings March 19, 2026 23:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prevents stack overflows/segfaults when parsing manifests that contain circular include/subninja directives by tracking the set of files currently being parsed and failing fast on cycles.

Changes:

  • Add an active_includes_ tracking set (owned by the root parser, shared with subparsers) and use it to detect include cycles.
  • Introduce an RAII helper to ensure active-include entries are removed on all parse exits (including errors).
  • Add unit tests validating error reporting for include cycles (including self-cycles).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/manifest_parser.cc Adds active-include tracking + cycle detection during include/subninja parsing.
src/manifest_parser.h Introduces members to own/share the active include set across nested parsers.
src/manifest_parser_test.cc Adds tests asserting correct diagnostics for include cycles.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +59 to +63
string canon = filename;
uint64_t slash_bits;
CanonicalizePath(&canon, &slash_bits);
ScopedActiveInclude scoped_include(active_includes_, canon);

Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle tracking inserts the current parser's filename into active_includes_. Because ParseTest() always uses the synthetic filename "input", any test/perf usage that tries to include input (a real file named "input") will now be reported as an include cycle even though the root input isn't that file. Consider excluding the synthetic root name from active_includes_ (or otherwise distinguishing in-memory parses from file-backed parses) so ParseTest() doesn't reserve a plausible path.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's.. true, but even though I explored a couple of ideas how to fix this, I didn't really like any of them, as they added complexity to non-test code to address a hypothetical case.

Currently no test triggers this issue, and if someone would attempt to add one that accidentally ran into it, it would visibly fail and be easy to fix (just don't call the manifest file in the test input).

Thus, I'm leaning towards not addressing this now in this PR unless the maintainers strongly feel about it, or someone has a good idea how to solve this in a clean way.

This problem would also go away if we removed the ParseTest method completely and instead used the fs_.Create("build.ninja", "<contents>") way to write manifests used by the tests into the in-memory VFS, which is what some of the tests already do. But that would be a larger change that we should do separetely, I think.

Circular include/subninja directives (e.g. a.ninja includes b.ninja
which includes a.ninja) caused unbounded recursion and a stack
overflow. Track which files are currently being parsed and report
an error when a cycle is detected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants