Wave 8: tech-debt audits, core unit tests, CLI pipe input (W11.1-W11.7)
- W11.1 context_plugin audit (architect-huang): 3 findings on ABI exception safety, strdup null checks, dead g_max_tokens variable. Rating: B. - W11.2 config audit (engineer-chen): identified 74-line TOML parser duplication between config_plugin and config_store, dual-store data isolation, dangling c_str() risk. Rating: C. - W11.3 event_bus + service_registry unit tests (qa-liu): 12 cases total, ctest coverage 2 -> 4 targets, 100% pass. - W11.4 CLI stdin pipe mode (engineer-zhao): isatty detection, single-shot inference path with exit codes 0/1/2/3. - W11.6 scripts/refresh_status.py (engineer-li): 431-line generator that scans 16 profile.md + 5 group.md to regenerate STATUS.md. - W11.7 destructive testing (qa-xu): 10 input scenarios PASS, found bin copy mismatch (BUG-1) plus 3 minor UX bugs for follow-up. Verified: cmake build 0 error, ctest 4/4 pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
108
agents/audits/W11.2-config-audit.md
Normal file
108
agents/audits/W11.2-config-audit.md
Normal file
@@ -0,0 +1,108 @@
|
||||
# W11.2 Config Plugin / ConfigStore 职责与跨 DLL 堆审计
|
||||
|
||||
**审计人**: 陈风 (engineer-chen)
|
||||
**日期**: 2026-05-27
|
||||
**审计范围**: `plugins/config/src/config_plugin.cpp` (146行) + `dstalk-core/src/config_store.cpp` (83行)
|
||||
|
||||
---
|
||||
|
||||
## 1. 职责划分
|
||||
|
||||
| 文件 | 职责 | 所在层 |
|
||||
|------|------|--------|
|
||||
| `config_store.cpp` (dstalk::ConfigStore) | Host 自身的键值配置存储。`dstalk_init` 加载初始配置,通过 `host->config_get/set` 供所有插件调用 | core |
|
||||
| `config_plugin.cpp` (匿名 ConfigStore) | 注册 "config" 服务 (vtable `dstalk_config_service_t`),提供 `load_file` 运行时调用接口,供其他插件通过 `query_service` 发现 | plugin |
|
||||
|
||||
**结论**: 职责有交集但不完全重复——plugin 提供了 core 未暴露的 `load_file` 运行时接口。然而,两个 ConfigStore 的 TOML 解析逻辑(74行)**逐字符完全相同**,且两个 config 存储在运行时相互独立(host 的 config 与 plugin 的 config 是不同的 map),存在以下问题:
|
||||
|
||||
- 同一 key 在两个 store 中可能具有不同值
|
||||
- `host->config_get("plugin_dir")` 读的是 core store,而 `query_service("config")->get("plugin_dir")` 读的是 plugin store
|
||||
- 用户不知道该用哪个接口
|
||||
|
||||
**建议边界**: core 的 ConfigStore 作为唯一真相源;config plugin 不再持有自己的 store,改为将 `load_file` / `get` / `set` 委托给 `g_host->config_get` / `g_host->config_set`。`load_file` 则需要 core 侧新增 `config_load_file` 到 host API 或 config plugin 自行实现(仅保留解析逻辑,写入 host store)。
|
||||
|
||||
---
|
||||
|
||||
## 2. 跨 DLL 堆合规
|
||||
|
||||
| 检查项 | config_store.cpp | config_plugin.cpp |
|
||||
|--------|-----------------|-------------------|
|
||||
| 直接 `malloc`/`free` | 无 | 无 |
|
||||
| 直接 `strdup` | 无 | 无 |
|
||||
| `new`/`delete` 跨边界 | 无(仅内部 STL 容器) | 无 |
|
||||
| `get()` 返回 `const char*` 的所有权 | 指向内部 `std::string`,调用方不得释放 | 同左 |
|
||||
| `set()` 的值类型 | `const char*` 输入,拷贝到 `std::string` | 同左 |
|
||||
|
||||
**结论**: **无跨 DLL 堆违规**。两个文件均完全使用 STL 容器(`std::unordered_map<std::string, std::string>`)管理内存,所有分配/释放均在各自 DLL 的 CRT 堆内完成。返回的 `const char*` 指向内部 string buffer,调用方只读不释放,符合 ABI 契约 2.1。
|
||||
|
||||
但需注意:`config_plugin.cpp:77` 返回的 `c_str()` 指向 plugin DLL 内部的 `std::string`。调用方若持有该指针跨越 plugin unload,将导致 use-after-free。这在当前设计中是低风险(config plugin 通常不会被卸载),但建议在 ABI 文档中明确说明。
|
||||
|
||||
---
|
||||
|
||||
## 3. 线程安全
|
||||
|
||||
| 文件 | 锁机制 | 评估 |
|
||||
|------|--------|------|
|
||||
| config_store.cpp | `mutable std::mutex`,get/set 均持锁 | get/set 单独调用安全 |
|
||||
| config_plugin.cpp | 同上(匿名类内 `std::mutex`) | 同左 |
|
||||
|
||||
**已知问题**:
|
||||
|
||||
- **Dangling pointer (config_store.cpp:72, config_plugin.cpp:77)**: `get()` 在锁内获取 `it->second.c_str()`,锁释放后返回。并发 `set()` 同一 key 会触发 `std::string` 重分配,使外部持有的指针悬垂。ABI 文档 6.4 已记录此风险但代码未做防护(如返回 `std::string` 副本或用 `host->strdup` 分配)。
|
||||
|
||||
- **load_file 非原子 (config_store.cpp:57-60, config_plugin.cpp:63-66)**: `load_file` 逐行解析,每写入一个 key-value 就释放锁。并发 `get()` 可观察到新旧配置混合的状态。设计上这是合理的(避免持锁做磁盘 I/O),但调用方需知晓。
|
||||
|
||||
- **ServiceRegistry 与 ConfigStore 之间无锁协调**: plugin 注册 "config" 服务时(`config_plugin.cpp:126`),ServiceRegistry 的写锁和 ConfigStore 的 mutex 是独立的。这在当前无问题(注册只存 vtable 指针),但如果未来 config service 的注册/卸载与 store 生命周期联动,需注意锁序。
|
||||
|
||||
---
|
||||
|
||||
## 4. 服务暴露
|
||||
|
||||
`config_plugin.cpp:124-127`:
|
||||
```cpp
|
||||
static int on_init(const dstalk_host_api_t* host) {
|
||||
g_host = host;
|
||||
return host->register_service("config", 1, &g_service);
|
||||
}
|
||||
```
|
||||
|
||||
- **正确**: 在 `on_init` 期间注册,符合 ABI 契约 4.1(仅此时可注册)
|
||||
- **正确**: vtable 为 static 全局变量,生命周期覆盖插件整个加载期(ABI 4.4)
|
||||
- **正确**: service name "config" 与 `dstalk_services.h:64` 定义一致
|
||||
- **注意**: 未检查 `register_service` 返回值。如果另一个插件也注册 "config",返回 -2 会被静默忽略
|
||||
- **去重**: ServiceRegistry 已实现重复注册检测(`service_registry.cpp:12-14`),但插件自身不感知失败
|
||||
|
||||
---
|
||||
|
||||
## 5. 三个最严重发现
|
||||
|
||||
### 发现 1: 完整 TOML 解析器代码重复 (Critical)
|
||||
- **config_plugin.cpp:16-90** 与 **config_store.cpp:10-83** 的解析逻辑完全相同
|
||||
- **影响**: 任何 bug 修复需双份维护;已发现 load_file 不处理 section 名前后空格、不支持 inline table 等问题,需在两个文件中分别修复
|
||||
- **修复方向**: config plugin 消除自己的 ConfigStore,改为委托 host store(通过 `g_host->config_get/set`),仅保留 `load_file` 且将解析结果写入 host store
|
||||
|
||||
### 发现 2: 双配置存储导致数据孤岛 (High)
|
||||
- **位置**: `host.cpp:176` 创建 `dstalk::ConfigStore`(host store);`config_plugin.cpp:98` 创建匿名 `ConfigStore`(plugin store)
|
||||
- **影响**: `host->config_get("key")` 和 `query_service("config")->get("key")` 返回不同数据,用户困惑
|
||||
- **修复方向**: 合并为唯一 store(建议保留 host 侧 `dstalk::ConfigStore`,config plugin 只做服务注册包装)
|
||||
|
||||
### 发现 3: get() 返回悬垂指针 (High)
|
||||
- **config_store.cpp:72** / **config_plugin.cpp:77**: `return it->second.c_str()` 在锁释放后,并发 set 同一 key 触发 realloc
|
||||
- **影响**: 调用方持有的 `const char*` 可能指向已释放内存,导致 crash 或静默数据损坏
|
||||
- **修复方向**: 选项 A — 返回值拷贝(调用方用 `host->strdup` 在锁内复制);选项 B — 文档明确规定调用方必须立即复制(当前 ABI spec 6.4 方向);选项 C — 用 `std::string` 作为返回值类型(需改 vtable 签名,涉及 API 版本 bump)
|
||||
|
||||
---
|
||||
|
||||
## 6. 整体评级
|
||||
|
||||
**评级: C**
|
||||
|
||||
**理由**: 无跨 DLL 堆违规(本次审计核心关注点通过),线程安全基础正确但 get() 存在悬垂指针缺陷。最严重的问题是 74 行 TOML 解析器完全重复 + 双存储架构混乱,属于设计层面而非实现 bug,但维护代价和用户困惑程度显著。建议优先解决发现 1 和 2(合并 store),发现 3 在合并时一并修复。
|
||||
|
||||
---
|
||||
|
||||
## 7. 附加建议
|
||||
|
||||
1. **config plugin 的 `on_init` 检查返回值** (config_plugin.cpp:126): `register_service` 返回 -2 时应有日志警告
|
||||
2. **load_file 前清空已有数据**: 当前 `load_file` 不先 `data_.clear()`,新文件与旧数据混合。如果这是预期行为(merge 语义),应在注释中说明
|
||||
3. **config_store.hpp 缺少 `load_file` 文档**: 未说明返回值约定(0=成功, -1=文件不存在/解析失败)
|
||||
Reference in New Issue
Block a user