Skip to content

Commit f6f6d04

Browse files
committed
fix(env): private-glibc strip was defeated by the composed override
Found while attributing an e2e 156 failure on this branch (NOT caused by the staging change — proven by diffing the generated build.ninja for that exact project: it differs only in an unused rule's text). `process.cppm::merged_environ` strips mcpp's private-glibc payload entries from an INHERITED LD_LIBRARY_PATH, and plan.cppm's comment states that guarantee. But merged_environ takes explicit `extra` overrides verbatim, and `env::prepend_path_list` — which composes exactly such an override for ninja and for run targets — appended the inherited value RAW. So the strip was bypassed precisely in the case it exists for: a nested `mcpp run` → `mcpp test` chain, where a payload tool patched against a different glibc then segfaults inside the dynamic linker before main (bare `__vdso_time`, then SIGSEGV). The predicate moves to mcpp.platform.env, next to path-list composition, since both halves of the guarantee need it; prepend_path_list now sanitizes only the inherited TAIL, so a payload dir the caller passed explicitly — the entry the sandbox binary actually needs — always survives. PATH is untouched. Why it looked like a regression: the two CI runs restored DIFFERENT sandbox caches (…-01baa227… vs …-0e74cc64…), so the payload version set differed and the latent bug only showed on one side. Before this fix, 156 was green only when the poisoned payload version happened to match what the tools expected.
1 parent 9951157 commit f6f6d04

4 files changed

Lines changed: 165 additions & 18 deletions

File tree

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@
2323

2424
一条记录在案的实现约束:**跳过时对 mtime 的任何触碰(包括对齐到 src 的 mtime)都会让 restat 失效并重新引发级联** —— ninja 的 restat 只把「mtime 未被命令改变」的输出视为从未需要构建。所以跳过路径不动任何时间戳。
2525

26+
- **私有 glibc 的 strip 不再被「组合出的显式覆盖」绕过。** `process.cppm::merged_environ` 会把继承来的 `LD_LIBRARY_PATH` 里的私有 glibc payload 条目剥掉,但**显式 `extra` 覆盖是原样采用的**;而 `env::prepend_path_list` 组合该覆盖时,把继承值(含毒)整段追加了进去 —— 于是 strip 恰好在它唯一有用的场景下失效:嵌套的 `mcpp run``mcpp test` 链里,子工具拿到一个**版本不匹配**的 libc.so.6,在动态链接器里 main 之前 SIGSEGV(签名:一行裸 `__vdso_time`)。
27+
28+
修法是把判据下沉到 `mcpp.platform.env`(路径列表组合的所在地),并让 `prepend_path_list` 只清洗**继承来的尾部**:调用方显式传入的 payload 目录必须保留 —— 那正是沙箱二进制需要的那一条。PATH 不受影响。
29+
30+
这个缺陷与 #311 无关,是排查 e2e 156 在本 PR 上失败时找出来的:两次 CI 恢复了**不同的 sandbox 缓存**(`…-01baa227…` vs `…-0e74cc64…`),payload 版本集不同,于是同一个潜伏缺陷只在一侧显形。本改动前该测试的绿灯取决于「毒化的 payload 版本恰好与工具期望的一致」。
31+
32+
2633
### 变更
2734

2835
- **BMI 缓存根统一为 `$MCPP_HOME/bmi`** `toolchain/stdmod.cppm``default_cache_root()` 是 home 解析逻辑的一份私有拷贝,自 v0.0.1 起一字未改:**没有 Windows 的 `USERPROFILE` 分支,也没有 self-contained 安装探测**。后果是 Windows PowerShell(不设 `HOME`)下 std BMI 缓存落进**当前工作目录**`.mcpp-bmi/`,而 dep BMI 缓存在 `%USERPROFILE%\.mcpp\bmi` —— 一个缓存两个根,其中一个还随 cwd 漂移(从子目录跑就重编一次 std);release tarball 形态的安装在 Linux 上同样分家。

src/platform/env.cppm

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,24 @@ private:
4141
std::string path_list_separator();
4242
std::string runtime_library_path_key();
4343
std::string host_tool_runtime_library_path_key();
44+
45+
// Drop mcpp's private-glibc payload entries from a loader search path.
46+
//
47+
// An outer `mcpp run`/`mcpp test` points LD_LIBRARY_PATH at the payload so ITS
48+
// child (a sandbox-linked binary) can load. Anything further down the process
49+
// tree that is a HOST binary — /bin/sh, or a payload tool patched against a
50+
// DIFFERENT payload version — then resolves a mismatched libc.so.6 against the
51+
// host ld.so and dies inside the dynamic linker before main (signature: a bare
52+
// `__vdso_time` line, then SIGSEGV). libc and ld.so are version-locked through
53+
// GLIBC_PRIVATE, so this never reproduces when the two happen to match.
54+
//
55+
// Lives here, next to path-list composition, because BOTH consumers need the
56+
// same predicate: process.cppm sanitizes the INHERITED variable for every child,
57+
// and prepend_path_list sanitizes the inherited TAIL it carries into a composed
58+
// override (dirs the caller passed explicitly are always kept — that is the
59+
// entry the sandbox binary actually needs).
60+
std::string strip_private_glibc(std::string_view paths);
61+
4462
std::string prepend_path_list(std::string_view key,
4563
std::span<const std::filesystem::path> dirs);
4664

@@ -134,6 +152,25 @@ std::string host_tool_runtime_library_path_key() {
134152
#endif
135153
}
136154

155+
std::string strip_private_glibc(std::string_view paths) {
156+
auto sep = path_list_separator();
157+
std::string cleaned;
158+
std::size_t start = 0;
159+
while (start <= paths.size()) {
160+
auto end = paths.find(sep, start);
161+
if (end == std::string_view::npos) end = paths.size();
162+
auto item = paths.substr(start, end - start);
163+
if (!item.empty() && item.find("/xim-x-glibc/") == std::string_view::npos
164+
&& item.find("\\xim-x-glibc\\") == std::string_view::npos) {
165+
if (!cleaned.empty()) cleaned += sep;
166+
cleaned += item;
167+
}
168+
if (end == paths.size()) break;
169+
start = end + sep.size();
170+
}
171+
return cleaned;
172+
}
173+
137174
std::string prepend_path_list(std::string_view key,
138175
std::span<const std::filesystem::path> dirs) {
139176
if (key.empty() || dirs.empty()) return "";
@@ -149,8 +186,19 @@ std::string prepend_path_list(std::string_view key,
149186

150187
std::string k(key);
151188
if (auto* existing = std::getenv(k.c_str()); existing && *existing) {
152-
value += sep;
153-
value += existing;
189+
// Loader paths only: the inherited value may carry an OUTER mcpp's
190+
// private-glibc entry, and the composed string becomes an EXPLICIT
191+
// override — which process.cppm's merged_environ takes verbatim,
192+
// skipping the sanitation it applies to inherited variables. Without
193+
// this the strip is defeated exactly when it matters (a nested
194+
// `mcpp run` → `mcpp test` chain), and the child tool segfaults in the
195+
// dynamic linker whenever the payload versions differ.
196+
std::string tail = (k == "LD_LIBRARY_PATH" || k == "DYLD_LIBRARY_PATH")
197+
? strip_private_glibc(existing) : std::string(existing);
198+
if (!tail.empty()) {
199+
value += sep;
200+
value += tail;
201+
}
154202
}
155203
return value;
156204
}

src/platform/process.cppm

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -188,22 +188,12 @@ char** host_environ() {
188188
// `__vdso_time` line). Strip exactly the private-glibc payload entries from
189189
// inherited loader paths: user-supplied entries survive, and an `extra`
190190
// override (the correct per-child value) always wins over the inherited var.
191-
std::string strip_private_glibc(std::string_view paths) {
192-
std::string cleaned;
193-
std::size_t start = 0;
194-
while (start <= paths.size()) {
195-
auto end = paths.find(':', start);
196-
if (end == std::string_view::npos) end = paths.size();
197-
auto item = paths.substr(start, end - start);
198-
if (!item.empty() && item.find("/xim-x-glibc/") == std::string_view::npos) {
199-
if (!cleaned.empty()) cleaned += ':';
200-
cleaned += item;
201-
}
202-
if (end == paths.size()) break;
203-
start = end + 1;
204-
}
205-
return cleaned;
206-
}
191+
//
192+
// The predicate itself lives in mcpp.platform.env, because the OTHER half of
193+
// this guarantee is there: a composed override (dirs + inherited tail) arrives
194+
// here as `extra` and therefore bypasses the sanitation below, so
195+
// prepend_path_list has to sanitize the tail it carries.
196+
using mcpp::platform::env::strip_private_glibc;
207197

208198
// Build a child environment block = the current environ with `extra` overrides
209199
// applied. Returned vector owns the strings; the caller derives a NUL-terminated

tests/unit/test_platform_env.cpp

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
#include <gtest/gtest.h>
2+
#include <cstdlib>
3+
4+
import std;
5+
import mcpp.platform.env;
6+
7+
namespace {
8+
9+
class ScopedVar {
10+
public:
11+
ScopedVar(std::string name, const char* value) : name_(std::move(name)) {
12+
if (const char* old = std::getenv(name_.c_str()); old) { had_ = true; old_ = old; }
13+
apply(value);
14+
}
15+
~ScopedVar() { apply(had_ ? old_.c_str() : nullptr); }
16+
ScopedVar(const ScopedVar&) = delete;
17+
ScopedVar& operator=(const ScopedVar&) = delete;
18+
private:
19+
void apply(const char* v) {
20+
#if defined(_WIN32)
21+
::_putenv_s(name_.c_str(), v ? v : "");
22+
#else
23+
if (v) ::setenv(name_.c_str(), v, 1); else ::unsetenv(name_.c_str());
24+
#endif
25+
}
26+
std::string name_;
27+
bool had_ = false;
28+
std::string old_;
29+
};
30+
31+
std::string sep() { return mcpp::platform::env::path_list_separator(); }
32+
33+
std::string join(std::initializer_list<std::string_view> parts) {
34+
std::string out;
35+
for (auto p : parts) {
36+
if (!out.empty()) out += sep();
37+
out += p;
38+
}
39+
return out;
40+
}
41+
42+
} // namespace
43+
44+
TEST(PlatformEnv, StripPrivateGlibcDropsOnlyPayloadEntries) {
45+
auto in = join({"/usr/lib", "/home/u/.mcpp/registry/data/xpkgs/xim-x-glibc/2.39/lib64",
46+
"/opt/mine"});
47+
EXPECT_EQ(mcpp::platform::env::strip_private_glibc(in),
48+
join({"/usr/lib", "/opt/mine"}));
49+
}
50+
51+
TEST(PlatformEnv, StripPrivateGlibcCanEmptyTheList) {
52+
EXPECT_EQ(mcpp::platform::env::strip_private_glibc(
53+
"/x/xpkgs/xim-x-glibc/2.42/lib64"), "");
54+
EXPECT_EQ(mcpp::platform::env::strip_private_glibc(""), "");
55+
}
56+
57+
// The regression this guards (mcpp#311 investigation): the composed value
58+
// becomes an EXPLICIT override, and process.cppm's merged_environ takes explicit
59+
// overrides verbatim — skipping the sanitation it applies to inherited
60+
// variables. So if the inherited tail carried an outer `mcpp run`'s private
61+
// glibc, it reached the child anyway and a payload tool patched against a
62+
// DIFFERENT glibc segfaulted in the dynamic linker. Sanitizing here is what
63+
// makes process.cppm's guarantee actually hold one hop down.
64+
TEST(PlatformEnv, PrependPathListSanitizesTheInheritedLoaderTail) {
65+
auto inherited = join({"/first",
66+
"/home/u/.mcpp/registry/data/xpkgs/xim-x-glibc/2.39/lib64",
67+
"/last"});
68+
ScopedVar ld("LD_LIBRARY_PATH", inherited.c_str());
69+
70+
std::vector<std::filesystem::path> dirs{"/payload/lib"};
71+
EXPECT_EQ(mcpp::platform::env::prepend_path_list("LD_LIBRARY_PATH", dirs),
72+
join({"/payload/lib", "/first", "/last"}));
73+
}
74+
75+
// A payload dir the CALLER passed is the entry the sandbox binary needs — it
76+
// must survive even though it matches the same pattern.
77+
TEST(PlatformEnv, PrependPathListKeepsExplicitPayloadDirs) {
78+
ScopedVar ld("LD_LIBRARY_PATH", "/home/u/.mcpp/registry/data/xpkgs/xim-x-glibc/2.39/lib64");
79+
80+
std::vector<std::filesystem::path> dirs{
81+
"/home/u/.mcpp/registry/data/xpkgs/xim-x-glibc/2.42/lib64"};
82+
// Explicit dir kept, inherited (older, mismatched) entry dropped.
83+
EXPECT_EQ(mcpp::platform::env::prepend_path_list("LD_LIBRARY_PATH", dirs),
84+
"/home/u/.mcpp/registry/data/xpkgs/xim-x-glibc/2.42/lib64");
85+
}
86+
87+
// PATH is not a loader search path: leave its inherited value alone.
88+
TEST(PlatformEnv, PrependPathListLeavesPathUntouched) {
89+
auto inherited = join({"/bin", "/x/xpkgs/xim-x-glibc/2.39/lib64"});
90+
ScopedVar path("PATH", inherited.c_str());
91+
92+
std::vector<std::filesystem::path> dirs{"/tools/bin"};
93+
EXPECT_EQ(mcpp::platform::env::prepend_path_list("PATH", dirs),
94+
join({"/tools/bin", "/bin", "/x/xpkgs/xim-x-glibc/2.39/lib64"}));
95+
}
96+
97+
TEST(PlatformEnv, PrependPathListWithNoInheritedValueIsJustTheDirs) {
98+
ScopedVar ld("LD_LIBRARY_PATH", nullptr);
99+
std::vector<std::filesystem::path> dirs{"/a", "/b"};
100+
EXPECT_EQ(mcpp::platform::env::prepend_path_list("LD_LIBRARY_PATH", dirs),
101+
join({"/a", "/b"}));
102+
}

0 commit comments

Comments
 (0)