Skip to content

fix(dkapture): return bytes read, not remaining buffer size, in vector read()s - #140

Open
JoeSergen wants to merge 1 commit into
DKapture:mainfrom
JoeSergen:fix/dkapture-vector-read-return
Open

JoeSergen wants to merge 1 commit into
DKapture:mainfrom
JoeSergen:fix/dkapture-vector-read-return

Conversation

@JoeSergen

Copy link
Copy Markdown

Fixes #137

The two buffer-based overloads read(std::vector<DataType>&, pid, buf, bsz) and read(std::vector<const char*>&, buf, bsz) returned the leftover bsz after the loop instead of the number of bytes written, contradicting the documented contract. Accumulate the total bytes written and return that, matching read(DataType, std::vector<pid_t>&, ...).

…r read()s

The two buffer-based overloads read(std::vector<DataType>&, pid, buf,
bsz) and read(std::vector<const char*>&, buf, bsz) returned the
leftover bsz after the loop instead of the number of bytes written,
contradicting the documented contract ('success returns the number of
bytes read'). Accumulate total bytes written and return that, matching
the read(DataType, std::vector<pid_t>&, ...) overload.
@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 ✅

137 - PR Code Verified

Compliant requirements:

  • 两个 vector 重载均新增 total 累加变量并返回实际写入字节数,不再返回剩余 bsz
  • 循环条件改为 ret <= 0 时 continue,并新增 assert((size_t)ret <= bsz),与工单期望实现一致
  • 返回值语义与 read(DataType, std::vector<pid_t>&, ...) 对齐

Requires further human verification:

  • 按工单复现步骤实际运行,验证返回值等于实际写入字节数(本 PR 未附带测试)
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@dkapture-ci-bot

Copy link
Copy Markdown
Contributor

你好,这是对 libdkapture#140 fix(dkapture): return bytes read, not remaining buffer size, 的评审。

本轮为 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 | 规模: +10/-4 | 文件: 1
结论: 可合并
主要风险: 无阻塞风险;返回值语义修正理论上影响依赖旧"剩余 bsz"语义的外部调用方,但与文档契约一致且仓库内无此类调用点。

总体结论: 两个 buffer 版 vector 重载原先返回循环后剩余的 bsz(未读满时近似等于总缓冲大小),与文档"读取成功时,返回读取到的数据的大小"(docs/dkapture-api.md:161-163)直接矛盾,是真实 bug。改为累加 total 返回实际写入字节数,与 read(DataType, std::vector<pid_t>&, ...) 的既有实现(so/dkapture.cpp:149-163,"return dsz - bsz")对齐,方向正确。经 grep 全仓库,demo 与 test 仅使用回调模式、单 dt/单 path 重载,无任何内部调用方依赖旧的"剩余量"语义;函数签名(含虚表布局)未变,无 ABI 结构性破坏。

主要问题: 无
次要建议:

  • so/dkapture.cpp:138,157 — 新增 assert((size_t)ret <= bsz) 在 NDEBUG 构建下消失;该不变量由底层保证(unsafe_find 在 bsz < dh->dsz 时返回 -ENOBUFS,so/data-map.cpp:380-389,不会越界推进 buf),建议加一行注释注明依据,避免后人误以为存在未防护路径。
  • test/dkapture-test.cpp — 被修改的两个重载目前完全没有测试覆盖(现有用例仅覆盖单 dt 重载、单 path 重载与回调模式),建议补一条"多个 dt 读入共享 buffer,返回值等于各记录 dsz 之和"的用例,否则此 bug 不会被发现过。

亮点:

  • 循环过滤条件从 ret<0 改为 ret<=0 并 continue,行为等价但更稳健;返回值语义与重载4及文档三方对齐,修复了文档与实现长期背离的问题。

commit message: 符合规范 — fix(dkapture) 作用域正确,引用了文档契约原文,说明了与重载4对齐的理由,单一职责。

已有讨论: bot 自动评审(09-11)判定工单 #137 合规,提醒按工单步骤实际运行验证返回值(PR 未附测试);无行级评论。

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() 两个 vector 重载返回剩余缓冲区大小而非已读字节数

3 participants