Skip to content

Commit f436c66

Browse files
committed
fix(toolchain): the compatibility fallback read a file this change deleted
Three defects, all found by CI in configurations this developer machine does not have. Each is the same shape: a repair placed where the control flow, or the machine state, does not reach it. 1. baked_runtime_binding read gcc's `specs` and clang's `.cfg` -- files mcpp used to write and no longer does. On a machine whose toolchain was installed after that change they are simply absent, so no binding resolved, so no payload paths, so no `--dynamic-linker`, so the artifact took the HOST loader and the hermetic check rejected it: /lib64/ld-linux-x86-64.so.2 (outside the sandbox) Every existing machine still has those files from before, which is exactly why local verification was green and CI was not. The binding now also comes from the compiler's own PT_INTERP -- written by the patchelf walk, which still runs on every install, and naming the same glibc payload the specs used to name. Read with a small ELF header reader rather than `patchelf --print-interpreter`: this runs during prepare, where patchelf is not guaranteed to be resolved. Verified by moving the specs file aside and rebuilding: binding still resolves, artifact still takes the payload loader. 2. probe_sysroot accepted gcc's reported sysroot as soon as it existed and carried headers, and only consulted remap_xlings_baked_sysroot when it did not exist. So the ownership predicate added for that function -- the whole point of which is a sysroot that EXISTS and belongs to someone else -- was never reached in the case it was written for. Measured on this machine: gcc reported a sysroot under an unrelated repo and every build took its headers. Ownership is now asked first; a foreign-but-usable sysroot remains the last resort, since taking nothing would break machines with no registry subos. 3. `rel.native().rfind("..", 0)` does not compile on Windows, where native() is a wstring -- every Windows job failed to build. It was also subtly wrong where it did compile: a directory named `..cache` is not an escape. Containment is a question about path components, so path_is_under asks it of components. Also removes a duplicated flag group: the C-runtime flags were emitted both as link_toolchain_flags and again as payload_ld for gcc. Correct but wasteful, and the link line has a hard 128KiB ceiling that real workspaces already spend 43% of. gcc with specs / gcc without specs / llvm: payload interpreter, exactly one --dynamic-linker, program runs. 65 unit tests, e2e 201/86/200/65/100/28/30.
1 parent fcf2e49 commit f436c66

5 files changed

Lines changed: 169 additions & 21 deletions

File tree

src/build/flags.cppm

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -712,14 +712,18 @@ CompileFlags compute_flags(const BuildPlan& plan) {
712712
// host's /lib or, on hosts without a system toolchain, passes bare names
713713
// that lld cannot open — issue #195), -L/-rpath for -lc/-lm, and the
714714
// payload's dynamic linker.
715+
//
716+
// Only for clang-with-cfg, and only in PayloadFirst. Every other
717+
// combination already has these flags: the gcc branch above assigns
718+
// `link_toolchain_flags = lm.link_flags(...)` for any mode but None, and
719+
// the clang branch adds them for Sysroot. Emitting them here as well put
720+
// the whole C-runtime group on the line twice -- harmless to correctness,
721+
// but the link line has a hard 128KiB ceiling (MAX_ARG_STRLEN) that real
722+
// workspaces already spend 43% of.
715723
std::string payload_ld;
716-
if (lm.mode == mcpp::toolchain::CLibMode::PayloadFirst) {
717-
// Emitted for gcc too now. It used to be gated on `isClangWithCfg`,
718-
// which left gcc's run-side addressing to its install-time specs
719-
// while the compile side moved per build.
720-
if (isClangWithCfg || !lm.clangDriver)
721-
payload_ld = lm.link_flags(ninjaEsc);
722-
}
724+
if (isClangWithCfg
725+
&& lm.mode == mcpp::toolchain::CLibMode::PayloadFirst)
726+
payload_ld = lm.link_flags(ninjaEsc);
723727
// GCC: replace the payload's patched `*link:` with the pristine one, so
724728
// its accumulated rpath entries do not reach the artifact. Must come
725729
// BEFORE our own -Wl flags is not required (specs are processed by the

src/fallback/probe_sysroot.cppm

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,22 @@ import mcpp.log;
1717

1818
export namespace mcpp::fallback {
1919

20+
// Does `child` sit under `anchor`?
21+
//
22+
// Spelled with path components rather than by inspecting the relative path's
23+
// text. `native()` is a wstring on Windows, so the obvious `rfind("..", 0)`
24+
// does not even compile there -- and where it does compile it is subtly wrong,
25+
// since a directory genuinely named `..cache` starts with those two
26+
// characters without escaping anything.
27+
bool path_is_under(const std::filesystem::path& child,
28+
const std::filesystem::path& anchor) {
29+
if (anchor.empty() || child.empty()) return false;
30+
auto rel = child.lexically_relative(anchor);
31+
if (rel.empty()) return false;
32+
static const std::filesystem::path kUp{".."};
33+
return *rel.begin() != kUp;
34+
}
35+
2036
// When GCC reports a baked "subos/default" sysroot that does not belong to
2137
// THIS toolchain's home, remap it to the equivalent sysroot under the
2238
// compiler's own xpkgs tree.
@@ -42,10 +58,8 @@ remap_xlings_baked_sysroot(std::string_view reportedPath,
4258
// Owned by this toolchain's registry? Then it is the right answer.
4359
auto registry = xpkgsOpt->parent_path().parent_path();
4460
std::error_code ec;
45-
auto rel = std::filesystem::path(std::string(reportedPath))
46-
.lexically_relative(registry);
47-
const bool inside = !rel.empty()
48-
&& rel.native().rfind("..", 0) != 0;
61+
const bool inside = path_is_under(
62+
std::filesystem::path(std::string(reportedPath)), registry);
4963
if (inside && std::filesystem::exists(std::string(reportedPath), ec))
5064
return std::nullopt;
5165
}
@@ -60,6 +74,20 @@ remap_xlings_baked_sysroot(std::string_view reportedPath,
6074
return std::nullopt;
6175
}
6276

77+
// Does this sysroot belong to the same registry as the compiler that reported
78+
// it? Callers need this BEFORE deciding whether a usable sysroot is
79+
// acceptable: usability and ownership are independent, and a path can pass the
80+
// first while failing the second.
81+
bool sysroot_is_owned(std::string_view reportedPath,
82+
const std::filesystem::path& compilerBin) {
83+
if (reportedPath.empty()) return false;
84+
auto xpkgs = mcpp::xlings::paths::xpkgs_from_compiler(compilerBin);
85+
// No registry to compare against -- nothing to contradict, so accept.
86+
if (!xpkgs) return true;
87+
return path_is_under(std::filesystem::path(std::string(reportedPath)),
88+
xpkgs->parent_path().parent_path());
89+
}
90+
6391
// Is this sysroot foreign -- neither this mcpp home's registry nor a tree
6492
// belonging to the project being built?
6593
//
@@ -76,12 +104,8 @@ bool sysroot_is_foreign(const std::filesystem::path& sysroot,
76104
const std::filesystem::path& registryRoot,
77105
const std::filesystem::path& projectRoot) {
78106
if (sysroot.empty()) return false;
79-
auto under = [&](const std::filesystem::path& anchor) {
80-
if (anchor.empty()) return false;
81-
auto rel = sysroot.lexically_relative(anchor);
82-
return !rel.empty() && rel.native().rfind("..", 0) != 0;
83-
};
84-
return !under(registryRoot) && !under(projectRoot);
107+
return !path_is_under(sysroot, registryRoot)
108+
&& !path_is_under(sysroot, projectRoot);
85109
}
86110

87111
// Parse a Clang .cfg file alongside the compiler binary for --sysroot=.

src/toolchain/post_install.cppm

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,66 @@ export void ensure_post_install_fixup(const mcpp::config::GlobalConfig& cfg,
530530
}
531531

532532

533+
// PT_INTERP of an ELF, read directly.
534+
//
535+
// Not via `patchelf --print-interpreter`: this runs during prepare, where
536+
// patchelf is not guaranteed to be resolved, and a dependency on an external
537+
// tool for four fields of a header is a dependency that will be missing on
538+
// exactly the machine that needs the answer.
539+
std::string read_elf_interp(const std::filesystem::path& bin) {
540+
std::ifstream is(bin, std::ios::binary);
541+
if (!is) return {};
542+
unsigned char ident[16]{};
543+
is.read(reinterpret_cast<char*>(ident), sizeof ident);
544+
if (!is || ident[0] != 0x7f || ident[1] != 'E'
545+
|| ident[2] != 'L' || ident[3] != 'F') return {};
546+
const bool is64 = ident[4] == 2;
547+
const bool le = ident[5] == 1;
548+
if (!is64 || !le) return {}; // the only shape payloads ship
549+
550+
auto u16 = [&](std::streamoff off) -> std::uint16_t {
551+
is.seekg(off); unsigned char b[2]{};
552+
is.read(reinterpret_cast<char*>(b), 2);
553+
return static_cast<std::uint16_t>(b[0] | (b[1] << 8));
554+
};
555+
auto u64 = [&](std::streamoff off) -> std::uint64_t {
556+
is.seekg(off); unsigned char b[8]{};
557+
is.read(reinterpret_cast<char*>(b), 8);
558+
std::uint64_t v = 0;
559+
for (int i = 7; i >= 0; --i) v = (v << 8) | b[i];
560+
return v;
561+
};
562+
563+
const auto phoff = u64(0x20);
564+
const auto phentsize = u16(0x36);
565+
const auto phnum = u16(0x38);
566+
if (!is || phoff == 0 || phentsize < 0x38 || phnum == 0) return {};
567+
568+
constexpr std::uint32_t kPtInterp = 3;
569+
for (std::uint16_t i = 0; i < phnum; ++i) {
570+
const auto ph = static_cast<std::streamoff>(phoff)
571+
+ static_cast<std::streamoff>(i) * phentsize;
572+
is.seekg(ph);
573+
unsigned char t[4]{};
574+
is.read(reinterpret_cast<char*>(t), 4);
575+
if (!is) return {};
576+
const std::uint32_t type =
577+
static_cast<std::uint32_t>(t[0]) | (t[1] << 8)
578+
| (t[2] << 16) | (static_cast<std::uint32_t>(t[3]) << 24);
579+
if (type != kPtInterp) continue;
580+
const auto offset = u64(ph + 0x08);
581+
const auto filesz = u64(ph + 0x20);
582+
if (filesz == 0 || filesz > 4096) return {};
583+
std::string str(static_cast<std::size_t>(filesz), '\0');
584+
is.seekg(static_cast<std::streamoff>(offset));
585+
is.read(str.data(), static_cast<std::streamsize>(filesz));
586+
if (!is) return {};
587+
if (auto z = str.find('\0'); z != std::string::npos) str.resize(z);
588+
return str;
589+
}
590+
return {};
591+
}
592+
533593
std::string baked_runtime_binding(const std::filesystem::path& compilerBin) {
534594
if (compilerBin.empty()) return {};
535595
std::error_code ec;
@@ -568,6 +628,21 @@ std::string baked_runtime_binding(const std::filesystem::path& compilerBin) {
568628
return r;
569629
}
570630
}
631+
632+
// The compiler's OWN interpreter.
633+
//
634+
// The two sources above are files mcpp used to write, and mcpp no longer
635+
// writes them -- so on a machine where the toolchain was installed after
636+
// that change they simply are not there, and the compatibility path that
637+
// depends on them answers nothing. That is not hypothetical: it is what
638+
// turned CI red while every existing developer machine, which still has
639+
// the files from before, stayed green.
640+
//
641+
// PT_INTERP is produced by the patchelf walk, which still runs on every
642+
// install, and it names the glibc payload this toolchain was aligned to --
643+
// the same fact the specs held, from a mechanism that has not gone away.
644+
if (auto r = from_text(read_elf_interp(compilerBin)); !r.empty())
645+
return r;
571646
return {};
572647
}
573648

src/toolchain/probe.cppm

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -291,21 +291,48 @@ probe_sysroot(const std::filesystem::path& compilerBin,
291291
mcpp::platform::null_redirect));
292292
if (r) {
293293
auto s = trim_line(*r);
294-
if (!s.empty() && std::filesystem::exists(s)) {
295-
if (usable(s)) return s;
294+
295+
// A usable sysroot that belongs to somebody else is still somebody
296+
// else's. gcc records this path as a string when it is built and
297+
// reports it forever after; on a machine with several checkouts the
298+
// recorded one routinely exists AND carries headers, so accepting it
299+
// on usability alone hands the build another project's tree. Measured
300+
// right here: this repo's gcc reported a sysroot under an unrelated
301+
// one, and every build took its headers.
302+
//
303+
// The ownership test lives inside remap_xlings_baked_sysroot, which is
304+
// exactly why the order matters -- the early return below reached it
305+
// only when the path was missing, so the case the remap exists for was
306+
// the one case it never saw.
307+
const bool ownedByThisHome =
308+
!s.empty() && mcpp::fallback::sysroot_is_owned(s, compilerBin);
309+
310+
if (ownedByThisHome && usable(s)) return s;
311+
if (!s.empty() && std::filesystem::exists(s) && !usable(s))
296312
mcpp::log::debug("probe", std::format(
297313
"sysroot '{}' exists but lacks usr/include/stdlib.h — ignoring", s));
298-
}
299314

300315
// GCC bakes the build-time sysroot into the binary. For xlings-built
301316
// GCC this is a path like <buildhost>/.xlings/subos/default that
302-
// doesn't exist on the user's machine. Remap via fallback module.
317+
// doesn't exist on the user's machine -- or exists and belongs to a
318+
// different one. Remap via fallback module.
303319
if (auto remapped = mcpp::fallback::remap_xlings_baked_sysroot(s, compilerBin)) {
304320
if (usable(*remapped)) return *remapped;
305321
mcpp::log::debug("probe", std::format(
306322
"remapped sysroot '{}' lacks usr/include/stdlib.h — ignoring",
307323
remapped->string()));
308324
}
325+
326+
// Last resort: a foreign but usable sysroot beats no sysroot. This is
327+
// the pre-existing behaviour, kept for machines that have no registry
328+
// subos to remap to -- it is a worse answer, not a wrong one, and
329+
// taking nothing here would break them outright.
330+
if (!s.empty() && std::filesystem::exists(s) && usable(s)) {
331+
mcpp::log::verbose("probe", std::format(
332+
"using sysroot '{}', which is outside this toolchain's "
333+
"registry — no equivalent found under it", s));
334+
return std::filesystem::path(s);
335+
}
309336
}
310337

311338
// 2. macOS fallback: use xcrun to discover the SDK path.

tests/unit/test_sysroot_ownership.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,4 +66,22 @@ TEST(SysrootOwnership, SiblingPrefixIsNotContainment) {
6666
kRegistry, kProject));
6767
}
6868

69+
// A directory whose NAME begins with two dots is not an escape. The first
70+
// version compared the relative path's text (`rfind("..", 0)`), which called
71+
// `/home/u/.mcpp/registry/..cache` an escape from the registry -- and did not
72+
// compile at all on Windows, where `native()` is a wstring. Containment is a
73+
// question about path components, so it is asked of components.
74+
TEST(SysrootOwnership, DotDotPrefixedNameIsNotAnEscape) {
75+
EXPECT_FALSE(fb::sysroot_is_foreign(
76+
kRegistry / "..cache" / "subos" / "default", kRegistry, kProject));
77+
}
78+
79+
TEST(SysrootOwnership, PathIsUnderIsDirectlyTestable) {
80+
EXPECT_TRUE(fb::path_is_under(kRegistry / "a" / "b", kRegistry));
81+
EXPECT_TRUE(fb::path_is_under(kRegistry, kRegistry));
82+
EXPECT_FALSE(fb::path_is_under(kRegistry.parent_path(), kRegistry));
83+
EXPECT_FALSE(fb::path_is_under(kRegistry, {}));
84+
EXPECT_FALSE(fb::path_is_under({}, kRegistry));
85+
}
86+
6987
} // namespace

0 commit comments

Comments
 (0)