Skip to content

Commit fa5e914

Browse files
committed
fix: 合入前深度自审发现的四个问题(含一处内存安全、一处 workspace 回归)
## 1. 内存安全:内置 mcpp 模块的 action 缓冲区越界 `action::add()` 把上界**硬编码成 4096**,却被 `provides_[1024]` / `imports_[1024]` 调用 —— 最多越界写约 3KB。这段代码会被编进**每一个**用户的 build.mcpp,是本次改动 里最严重的一处。 修法:上界改成**参数**(`sizeof buf` 在调用点取)。一个「不挨着它所约束的数组」的 上界,正是会越界的那种形状。同时: - `command_` 原本 8192 却被 4096 的上界截半,现在按实际容量走 - 缓冲区放大到真实生成器调用的量级(protoc 带一堆 -I 的命令行很长) - 溢出不再静默截断:置 `overflow` 标记,引擎给出**点名容量限制**的诊断,而不是让 作者去找一个其实没错、只是太长的「拼写错误」 ## 2. workspace 回归:成员的 target/ 与 mcpp.lock 落到了 workspace 根 `workRoot` 在 `root` **尚未定稿**时就取了值 —— workspace 那段会把 `root` 改成选中的 成员(`root = memberDir`),于是成员的 `target/`、`mcpp.lock`、`.mcpp/`、 `compile_commands.json` 全落到 workspace 根。 CI 抓到了(macOS 与 Linux 的 `35_workspace` 报 "hello binary not found", `120_ws_root_indices` 报 "expected x.widget2 lock entry")。修法:把 `workRoot` 的 推导移到 workspace 段**之后**,并在原处留注释说明为什么不能在那里取。 教训记下来:我本地的回归面**太窄** —— 只跑了 build.mcpp 相关的用例,而这次改动动的是 「mcpp 往哪写」,workspace 才是它最敏感的形态。 ## 3. 并发:两个工程会共用同一个工具子构建的 scratch 目录 tool store 是**全局**的,所以两个工程可能同时要同一个工具。原实现共用 `<entry>/build`,于是它们并发写同一棵 ninja 树,先完成的那个还会 `remove_all` 把另一个的目录端掉。改成按**消费方**哈希隔离;被共享的是发布出来的 二进制,不是 scratch。哈希而非随机,这样重跑能复用自己的 scratch。 ## 4. JSON 有效性:除 \n 外的控制字符没转义 `\t` / `\r` 会通过 Windows 路径和日志文本进来,不转义就直接不是 JSON 了。 ## 文档 超时那条原文写成了跨平台生效 —— **Windows 上不生效**(进程启动器没有 kill-by-handle 的路径),与 `mcpp test --timeout` 是同一条限制。明说,而不是含糊过去。 ## 性能(实测,非断言) 在这台机器上**测不出可分辨的差异**:无 build.mcpp 的工程强制 prepare,15 次中位 OLD 156ms / NEW 140ms(min 62/96,max 181/194)—— 分布高度重叠,两个方向都出现过。 结构上也符合:新增路径全是 O(小),且未被使用时全部惰性(工具请求为空、 `plan.actions` 为空、hostModules 为空则一行都不多跑)。 ## 覆盖面的诚实说明 四个新 e2e 都声明 `# requires: gcc`,而 Windows runner **不提供** gcc 能力 —— 所以 action / host 工具 / 规则包这三条路在 Windows 上**未经验证**。它们在未使用时 完全惰性,不会影响既有 Windows 行为,但这不等于验证过。 验证:单测 57/57;`35_workspace` / `120_ws_root_indices` / `90_workspace_test` 三个 先前红的全绿;186–189 与 43/76/30/51/50/171/04/02 全通过。
1 parent c026ea3 commit fa5e914

5 files changed

Lines changed: 99 additions & 31 deletions

File tree

docs/07-build-mcpp.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -344,9 +344,13 @@ When nothing changed you'll see `build.mcpp up to date (cached)`; otherwise
344344
- **CWD is the project root**, so relative paths (`src/generated.cpp`) land where
345345
you expect.
346346
- A non-zero exit from `build.mcpp` aborts the build and prints its output.
347-
- **The run is bounded** (mcpp 2026.8.5.1+): a build program gets **600 s** by
348-
default, after which mcpp kills it and fails the build naming the package.
349-
Override with `MCPP_BUILD_PROGRAM_TIMEOUT=<seconds>` (`0` = no limit). The
347+
- **The run is bounded** (mcpp 2026.8.5.1+, **POSIX only**): a build program
348+
gets **600 s** by default, after which mcpp kills it and fails the build
349+
naming the package. Override with `MCPP_BUILD_PROGRAM_TIMEOUT=<seconds>`
350+
(`0` = no limit). **On Windows the bound is not enforced** — the process
351+
launcher has no kill-by-handle path yet (`mcpp.platform.process`), so a
352+
build program that hangs there still hangs the build. Same limitation as
353+
`mcpp test --timeout`; stated rather than papered over. The
350354
**compile** is deliberately *not* bounded — the same asymmetry `mcpp test`
351355
uses: a long compile is usually legitimate (a first-run `std` module build is
352356
minutes) and killing it produces a baffling failure, while a long-running

docs/zh/07-build-mcpp.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,11 @@ mcpp **不会**每次构建都重跑 `build.mcpp`。它会缓存程序产出的
308308
[05 - mcpp.toml 工程文件指南](05-mcpp-toml.md)。
309309
- **当前工作目录是工程根目录**,因此相对路径(`src/generated.cpp`)会落在你预期的位置。
310310
- `build.mcpp` 非零退出会中止构建并打印其输出。
311-
- **运行有时间上限**(mcpp 2026.8.5.1+):构建程序默认有 **600 秒**,超时后 mcpp 杀掉它
312-
并让构建失败,错误里会点名是哪个包。用 `MCPP_BUILD_PROGRAM_TIMEOUT=<秒>` 覆盖
313-
(`0` = 不限)。**编译**这一步刻意**不设**上限——与 `mcpp test` 同一条不对称纪律:
311+
- **运行有时间上限**(mcpp 2026.8.5.1+,**仅 POSIX**):构建程序默认有 **600 秒**,
312+
超时后 mcpp 杀掉它并让构建失败,错误里会点名是哪个包。用
313+
`MCPP_BUILD_PROGRAM_TIMEOUT=<秒>` 覆盖(`0` = 不限)。**Windows 上这个上限不生效**
314+
—— 进程启动器还没有 kill-by-handle 的路径(`mcpp.platform.process`),所以在那里
315+
卡死的构建程序仍会把构建挂住。与 `mcpp test --timeout` 是同一条限制;明说,而不是
316+
含糊过去。**编译**这一步刻意**不设**上限——与 `mcpp test` 同一条不对称纪律:
314317
编译跑得久通常是正当的(首次构建 `std` 模块就是分钟级),杀掉它只会产生莫名其妙的
315318
失败;而构建**程序**跑得久通常是卡住了,不设上限就会让整个构建挂死且毫无诊断。

src/build/directives.cppm

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,22 @@ std::optional<mcpp::manifest::BuildAction> decode_action(std::string_view payloa
517517

518518
std::string action_error(const Directives& d) {
519519
for (auto const& payload : d.at(Slot::Actions)) {
520+
// The typed API sets this when an argv did not fit its fixed buffer.
521+
// Diagnosed separately because "malformed action" would send the
522+
// author looking for a typo in something that was actually correct
523+
// and merely too long.
524+
if (payload.find("\"overflow\":true") != std::string::npos) {
525+
return std::format(
526+
"build.mcpp declared an action whose arguments did not fit.\n"
527+
" The typed `mcpp::action` builder uses fixed buffers "
528+
"(the bundled module has to stay\n"
529+
" buildable before a std module exists, so it cannot use "
530+
"std::string).\n"
531+
" Shorten the command — e.g. pass a response file, or a "
532+
"directory instead of\n"
533+
" enumerating its files.\n"
534+
" payload: {}", payload);
535+
}
520536
if (decode_action(payload)) continue;
521537
// A malformed action is a hard error, never a skip: an action that
522538
// silently does not exist produces a build missing generated sources,

src/build/hostprogram.cppm

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -64,19 +64,23 @@ struct action {
6464
const char* role = "source"; // "source" | "check" | "artifact"
6565
const char* description = "";
6666
bool blocking = false; // check only: gate compilation on it
67-
action& input(const char* p) { add(inputs_, p); return *this; }
68-
action& output(const char* p) { add(outputs_, p); return *this; }
69-
action& arg(const char* a) { add(command_, a); return *this; }
67+
action& input(const char* p) { add(inputs_, sizeof inputs_, p); return *this; }
68+
action& output(const char* p) { add(outputs_, sizeof outputs_, p); return *this; }
69+
action& arg(const char* a) { add(command_, sizeof command_, a); return *this; }
7070
// Declare what a generated MODULE INTERFACE provides/imports. Same
7171
// "declare instead of discover" trade [modules].scan_overrides makes, and
7272
// what lets a generated .cppm exist as a graph node at all.
73-
action& provides(const char* n) { add(provides_, n); return *this; }
74-
action& imports(const char* n) { add(imports_, n); return *this; }
73+
action& provides(const char* n) { add(provides_, sizeof provides_, n); return *this; }
74+
action& imports(const char* n) { add(imports_, sizeof imports_, n); return *this; }
7575
void submit() const {
7676
std::printf("mcpp:action={\"id\":"); esc(id);
7777
std::printf(",\"role\":"); esc(role);
7878
std::printf(",\"description\":"); esc(description);
7979
std::printf(",\"blocking\":%s", blocking ? "true" : "false");
80+
// A truncated argv would otherwise be INVALID rather than obviously
81+
// wrong — the engine turns this marker into a diagnostic that names
82+
// the limit, instead of a generic "malformed action".
83+
if (overflow_) std::printf(",\"overflow\":true");
8084
std::printf(",\"inputs\":[%s]", inputs_);
8185
std::printf(",\"outputs\":[%s]", outputs_);
8286
std::printf(",\"command\":[%s]", command_);
@@ -85,26 +89,42 @@ struct action {
8589
std::printf("}\n");
8690
}
8791
private:
88-
char inputs_[4096]{}, outputs_[4096]{}, command_[8192]{}, provides_[1024]{}, imports_[1024]{};
92+
// Fixed buffers because this module must stay buildable BEFORE a std BMI
93+
// exists (it is what a build.mcpp imports, and it may be compiled first) —
94+
// so no std::string. Sizes chosen for real generator invocations: a protoc
95+
// command line with many -I paths runs long.
96+
char inputs_[8192]{}, outputs_[8192]{}, command_[16384]{},
97+
provides_[2048]{}, imports_[2048]{};
98+
mutable bool overflow_ = false;
8999
static void esc(const char* s) {
90100
std::putchar('"');
91101
for (const char* p = s; *p; ++p) {
92-
if (*p == '"' || *p == '\\') std::putchar('\\');
93-
if (*p == '\n') { std::printf("\\n"); continue; }
94-
std::putchar(*p);
102+
unsigned char c = (unsigned char)*p;
103+
if (c == '"' || c == '\\') { std::putchar('\\'); std::putchar(c); continue; }
104+
// Any control character has to be escaped or the payload is not
105+
// JSON at all. \n was handled before; \t and \r reach this code
106+
// through ordinary Windows paths and log text.
107+
if (c < 0x20) { std::printf("\\u%04x", c); continue; }
108+
std::putchar(c);
95109
}
96110
std::putchar('"');
97111
}
98-
static void add(char* buf, const char* s) {
112+
// Capacity is a PARAMETER. The previous revision hardcoded 4096 while the
113+
// smallest buffer here was 1024 — a bound living somewhere other than next
114+
// to the array it bounds is exactly the shape that overflows.
115+
bool add(char* buf, unsigned long cap, const char* s) {
99116
unsigned long o = 0; while (buf[o]) ++o;
117+
if (o + 4 >= cap) { overflow_ = true; return false; }
100118
if (o) buf[o++] = ',';
101119
buf[o++] = '"';
102-
for (const char* p = s; *p && o + 3 < 4096; ++p) {
120+
for (const char* p = s; *p; ++p) {
121+
if (o + 3 >= cap) { buf[o] = 0; overflow_ = true; return false; }
103122
if (*p == '"' || *p == '\\') buf[o++] = '\\';
104123
buf[o++] = *p;
105124
}
106125
buf[o++] = '"';
107126
buf[o] = 0;
127+
return true;
108128
}
109129
};
110130
inline void rerun_if_changed(const char* path) { std::printf("mcpp:rerun-if-changed=%s\n", path); }
@@ -350,8 +370,12 @@ build_host_module(const fs::path& bdir, const fs::path& compiler,
350370
"(src/<name>.cppm or [lib] path).",
351371
logicalName, interfacePath.string()));
352372
}
353-
// A filesystem-safe stem: a module name contains dots, which are fine in a
354-
// path but make `foo.rules.o` read as an extension chain.
373+
// A filesystem-safe stem. Partition separators and any path separator that
374+
// sneaks into a logical name would otherwise create directories that do
375+
// not exist. Dots are left ALONE on purpose: `a.b.rules.o` is a legal
376+
// filename, GCC's own gcm.cache uses the dotted module name verbatim, and
377+
// rewriting them would make the object name disagree with the BMI name for
378+
// no gain.
355379
std::string stem(logicalName);
356380
for (auto& c : stem) if (c == ':' || c == '/' || c == '\\') c = '-';
357381

src/build/prepare.cppm

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -876,15 +876,11 @@ prepare_build(bool print_fingerprint,
876876
if (!root) {
877877
return std::unexpected("no mcpp.toml found in current directory or any parent");
878878
}
879-
// Where mcpp writes. Defaults to the project root, so every existing
880-
// invocation is byte-for-byte unchanged; the tool-provisioning pass points
881-
// it at the tool store instead (BuildOverrides::work_dir).
882-
const std::filesystem::path workRoot =
883-
overrides.work_dir.empty() ? *root : overrides.work_dir;
884-
{
885-
std::error_code wdEc;
886-
std::filesystem::create_directories(workRoot, wdEc);
887-
}
879+
// NOTE: `workRoot` is deliberately NOT derived here. `root` is not final
880+
// yet — the workspace block below reassigns it to the selected member
881+
// (`root = memberDir`), and anchoring the write root to the pre-switch
882+
// value puts a member's target/, mcpp.lock and .mcpp/ at the WORKSPACE
883+
// root. See the derivation right after that block.
888884

889885
auto m = mcpp::manifest::load(*root / "mcpp.toml");
890886
if (!m) return std::unexpected(m.error().format());
@@ -988,6 +984,18 @@ prepare_build(bool print_fingerprint,
988984
}
989985
}
990986

987+
// Where mcpp WRITES — derived here because `root` is only final now: the
988+
// workspace block above may have moved it to the selected member. Defaults
989+
// to the project root, so every existing invocation is byte-for-byte
990+
// unchanged; the tool-provisioning pass points it at the tool store
991+
// instead (BuildOverrides::work_dir).
992+
const std::filesystem::path workRoot =
993+
overrides.work_dir.empty() ? *root : overrides.work_dir;
994+
{
995+
std::error_code wdEc;
996+
std::filesystem::create_directories(workRoot, wdEc);
997+
}
998+
991999
// Inject synthetic targets (e.g. test binaries from `mcpp test`).
9921000
for (auto& t : extraTargets) m->targets.push_back(t);
9931001

@@ -4093,7 +4101,20 @@ prepare_build(bool print_fingerprint,
40934101
sub.project_root = depPkg.root;
40944102
// Never the package root: it is shared across projects and
40954103
// may be read-only. This is the reason work_dir exists.
4096-
sub.work_dir = entry / "build";
4104+
//
4105+
// Scratch is keyed on the CONSUMING project, not shared:
4106+
// the store is GLOBAL, so two projects can want the same
4107+
// tool at once. A single `<entry>/build` would have them
4108+
// writing one ninja tree concurrently, and whichever
4109+
// finished first would `remove_all` it out from under the
4110+
// other. The published binary is what gets shared; the
4111+
// scratch is not.
4112+
//
4113+
// Hashed rather than random so a re-run reuses its own
4114+
// scratch (ninja stays incremental if the publish step
4115+
// never got to delete it).
4116+
sub.work_dir = entry / std::format("build-{}",
4117+
mcpp::toolchain::hash_string(workRoot.string()));
40974118
sub.target_triple = ""; // HOST — the whole point
40984119
sub.profile = "release";
40994120
sub.cache_mode = overrides.cache_mode;
@@ -4181,8 +4202,8 @@ prepare_build(bool print_fingerprint,
41814202
mcpp::build::tool_store::write_entry(entry, key);
41824203
// The sub-build tree is large (protoc is several hundred
41834204
// objects) and the key covers every input, so a hit never
4184-
// needs it again.
4185-
std::filesystem::remove_all(entry / "build", cpEc);
4205+
// needs it again. Removes only THIS consumer's scratch.
4206+
std::filesystem::remove_all(sub.work_dir, cpEc);
41864207
record(binOut);
41874208
}
41884209
}

0 commit comments

Comments
 (0)