Skip to content

fix(dkapture): honor parsed pid in read(vector<path>, cb) callback mode - #141

Open
JoeSergen wants to merge 1 commit into
DKapture:mainfrom
JoeSergen:fix/dkapture-paths-callback-pid
Open

JoeSergen wants to merge 1 commit into
DKapture:mainfrom
JoeSergen:fix/dkapture-paths-callback-pid

Conversation

@JoeSergen

Copy link
Copy Markdown

Fixes #134

ead(std::vector<const char*>&, DKCallback, void*)parsed the pid out of each/proc//path but then ignored it and calledread(dt, cb, ctx), which iterates ALL processes. Read the specific process' data via the buffer API and invoke the callback once per DataHdrrecord, so/proc/1234/statonly reports pid 1234. Guards against zero/oversizeddsz` to avoid an infinite loop on bad records.

The overload read(std::vector<const char*>&, DKCallback, void*) parsed
the pid out of each /proc/<pid>/<node> path but then ignored it and
called read(dt, cb, ctx), which iterates ALL processes. Read the
specific process' data via the buffer API and invoke the callback once
per DataHdr record, so /proc/1234/stat only reports pid 1234. Guard
against zero/oversized dsz to avoid an infinite loop on bad records.
@xu-lang

xu-lang commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

/review

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

134 - Partially compliant

Compliant requirements:

  • 解析出的 pid 现通过 read(dt, pid, buf, bsz) 正确使用
  • 按 DataHdr 记录逐条回调
  • 添加了 cb 空指针检查,返回 -EINVAL
  • 添加了 dsz 零值/越界守卫,防止无限循环
  • 行为与 buffer 版本一致

Non-compliant requirements:

Requires further human verification:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@xu-lang

xu-lang commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

P1 /proc//fd 仍然只能回调一个 FD
so/dkapture.cpp:313 (

) 改为调用 read(dt, pid, buf, size)。但 PROC_PID_FD 的每个文件描述符分别存储为一个 DataHdr,而按具体 PID 查询的 DataMap::unsafe_find() 找到匹配 hash 后立即返回,只复制最新的一条记录。因此传入 /proc/123/fd 时,只会回调该进程的一个 FD,而接口文档要求“每个 FD 回调一次”。
建议不要通过单记录 buffer API实现该回调重载。可以继续使用遍历接口,但增加一个包装回调,根据 DataHdr::pid 过滤目标 PID;或者为 DataMap 增加按 PID 和类型遍历全部匹配记录的接口。

@dkapture-ci-bot

Copy link
Copy Markdown
Contributor

你好,这是对 libdkapture#141 fix(dkapture): honor parsed pid in read(vector, cb) ca 的评审。

本轮为 DKapture 两仓(libdkapture / dkapture-bpf)全部以 fix 开头 open PR 的批量评审,共 17 个;汇总表如下,本 PR 加粗。逐项详评见分隔线下方。

PR 标题 作者 规模 结论 主要风险
#149 fix(net-traffic): initialize rules before loadin yuKing123-king +19/-9 可合并 规则安装顺序与其它工具约定不一致;本仓改动无正确性缺陷
#147 fix(net-filter): abort rule loading on invalid c yuKing123-king +9/-3 需修改 失败时 clear_rules() 波及 add_rule() 装入的规则;空白注释行导致整个加载失败
#141 fix(dkapture): honor parsed pid in read(vector<p JoeSergen +23/-2 需修改 unsafe_find 单记录语义使 /proc//fd 只回调一次;返回值语义漂移
#140 fix(dkapture): return bytes read, not remaining JoeSergen +10/-4 可合并 无阻塞风险;与文档契约对齐,仓内无调用方依赖旧语义
#114 fix: replace manual destructor calls in construc JoeSergen +28/-7 需修改 捆绑了与 open PR #112 逐字节相同的 fs_watch 修复,跨 PR 重复
#125 fix(lsof): start ringbuf consumer before iterato yuKing123-king +17/-3 需修改 线程启动后错误路径仍 goto err_out → 释放运行中 rb(UAF)+ 线程泄漏
#112 fix: fs_watch calls trace_file_init instead of m JoeSergen +1/-1 可合并 无;仅修一处复制粘贴错误
#103 fix: make power-snoop internal symbols static fo yuKing123-king +3/-2 可合并 改动无害;但 PR 描述的 multiple definition 在当前构建配置下无法复现
#119 fix(syscall-stat): stop skipping syscall key 0 d yuKing123-king +18/-6 可合并 循环终止四条路径已逐一核实;缩进与提交拆分小问题
#115 fix(syscall-stat): improve builtin flow yuKing123-king +204/-41 需修改 三处 bpf_get_map_fd 错误路径未设 ret,最终 return ret 误报成功
#117 fix(trace-signal): validate invalid command line yuKing123-king +190/-29 需修改 BUILTIN 测试入口未接入 test/Makefile,不可达;register_signal 残留
#102 fix(trace-exec): reject invalid command line arg yuKing123-king +135/-22 需修改 BUILTIN 入口同样未接入构建;-h 退出码 0→1 属未说明的行为变更
#97 fix(so): reuse pinned dkapture bpf objects corre yuKing123-king +1/-1 需修改 test mock 仍按 map- 前缀命名 pin,合并后 gtest FindMap 用例失败
#35 fix(pagefault): add max_entries and value_size f yuKing123-king +7/-3 可合并 修复真实,但已被 main 上等效修复 1e90486 取代,建议确认后关闭
#32 fix(peek-fd): correct args field name from mvlen yuKing123-king +1/-1 可合并 无功能风险;标题/描述与实际改动方向不符
#30 fix(trace-signal): avoid inflight event key coll yuKing123-king +36/-22 需修改 sys_exit_kill 的 !rule 早退路径仍泄漏 inflight 条目
#29 fix(syscall-stat): replace exec fexit with kprob yuKing123-king +93/-13 阻塞 exec 路径从 struct filename* 本身读字符串,-f 过滤将完全失效

本 PR 评审详情

作者: JoeSergen | 规模: +23/-2 | 文件: 1
结论: 需修改
主要风险: 按具体 pid 经 DataMap::unsafe_find 读取只能取到最新一条记录,/proc//fd 等多记录类型只回调一次;且返回值语义从"字节数"变为"记录数"。

总体结论: PR 正确解决了 #134 的核心问题(旧代码解析出 pid 后调用 read(dt, cb, ctx) 遍历全部进程),改为经 buffer API 按指定 pid 读取并逐条回调,方向正确。但底层 DataMap::unsafe_find 命中 hash 即复制单条记录返回,导致多记录类型(fd 等)仍只回调一次,PR 描述中"invoke the callback once per DataHdr record"未真正达成;返回值也从字节数漂移为记录数,与文档契约及同文件其他重载不一致。

主要问题:

  • (P1) so/dkapture.cpp:311 — 改为调用 read(dt, pid, buf, bsz) 后,DataMap::unsafe_find(so/data-map.cpp:378-391)在找到第一个匹配 hash 时即 "memcpy(buf, dh, dh->dsz); return dh->dsz;" 只复制最新一条记录。而 BPF 侧 PROC_PID_FD 为每个 fd 独立生成一条 DataHdr,hash 均为 MK_KEY(pid, PROC_PID_FD)(dkapture-bpf observe/proc-info.bpf.c:870-884,dump_task_file 迭代器对每个 file 调 fill_hdr(hdr, task, dsz, PROC_PID_FD))。因此 read("/proc/123/fd", cb, ctx) 只会回调一次、仅含一个 fd,与接口"每个 FD 回调一次"的语义不符;其后 316-332 行的 while 逐条解析循环实际只会执行一轮。
  • (P2) so/dkapture.cpp:329 — total++ 统计的是回调记录条数;旧实现 total += rsz 中 rsz 是 sub_iterator 累加的 Σdh->dsz 字节数(so/data-map.cpp:171 "ret += dh->dsz"),文档契约也规定"读取成功时,返回读取到的数据的大小"(docs/dkapture-api.md:161-163)。同文件的 read(std::vector&, cb, ctx)(so/dkapture.cpp:278-291)仍返回字节数,两个 vector 回调重载语义将不一致,且与 fix(dkapture): return bytes read, not remaining buffer size, in vector read()s #140 统一"返回字节数"的方向相悖。

次要建议:

  • so/dkapture.cpp:301 — 每次调用无条件在堆上分配/释放 1MB std::vector,轮询场景开销可观;且单条记录 >1MB 时底层返回 -ENOBUFS 被 312-315 行静默 continue(旧回调路径无大小上限,不会丢数据)。建议复用缓冲或在头文件注明 1MB 上限。
  • so/dkapture.cpp:296 — 本 PR 为 path 重载新增了 !cb → -EINVAL 防护(好),但同文件 read(std::vector&, cb, ctx)(so/dkapture.cpp:278)仍无此校验,cb 为空时 set_iterator(nullptr) 变成静默空读返回 0,建议一并补齐保持一致。

亮点:

  • dsz 越界/为零守卫(so/dkapture.cpp:320-323)可防坏记录导致的死循环;dsz 为 unsigned int(dkapture-bpf/export/dkapture.h:108),与 (ssize_t) 比较无负数绕过漏洞,守卫逻辑严密。
  • cb 空指针返回 -EINVAL 与 file_watch(so/dkapture.cpp:325)既有风格一致。

commit message: 符合规范 — fix(dkapture) 作用域正确,说明了改动机(pid 被忽略)与 dsz 防护理由,单一职责。

已有讨论: bot 自动评审(09-11)认为完全合规;成员 xu-lang 于 09-14 提出 P1:/proc//fd 只能回调一个 FD,建议不改走单记录 buffer API,而是继续用遍历接口加按 DataHdr::pid 过滤的包装回调,或为 DataMap 增加按 pid+type 遍历全部匹配记录的接口。经本评审核实该问题成立且当前 head(e6df61c0)尚未修复,是合并前必须解决的阻塞点;返回值语义漂移为本评审新增的增量意见。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: dkapture::read(paths, cb, ctx) 忽略解析出的 pid,返回所有进程数据

3 participants