fix(syscall-stat): improve builtin flow - #115
yuKing123-king wants to merge 1 commit into
Conversation
|
注意提交信息，应该是fix不是test |
220b53c to
c1c5e24
Compare
ok，后面统一会改成fix |
c1c5e24 to
1cf902e
Compare
yuKing123-king
changed the title
| static std::atomic<bool> exit_flag(false); // Flag to signal exit | ||
| static std::atomic<bool> exit_flag(false); | ||
|
|
||
| #ifdef BUILTIN |
There was a problem hiding this comment.
这个宏的引入是不必要的，局部函数直接用static就行，没必要考虑这种兼容性
|
|
||
| // Parse command line arguments | ||
| void parse_args(int argc, char **argv) | ||
| static int parse_args(int argc, char **argv) |
There was a problem hiding this comment.
所有局部函数已改成static
| int *log_fd, | ||
| int *stats_fd | ||
| ){ | ||
| test_name = "syscall-stat"; // 告诉 mock/test 框架当前正在运行哪个工具 |
1cf902e to
13a61c2
Compare
| // stats | ||
|
|
||
| while (0 == bpf_map_get_next_key(stats_fd, &key, &nxt_key)) | ||
| int ret = bpf_map_get_next_key(stats_fd, NULL, &nxt_key); |
There was a problem hiding this comment.
这的改动是有必要的，因为原来的代码是
while (0 == bpf_map_get_next_key(stats_fd, &key, &nxt_key))
{
info sys_stat;
bpf_map_lookup_elem(stats_fd, &nxt_key, &sys_stat);
if (nxt_key >= sizeof(sys_tbl) / sizeof(sys_tbl[0]))
{
key = nxt_key;
continue;
}
if (sys_stat.cnt == 0)
{
key = nxt_key;
continue;
}
stats.push_back({nxt_key, sys_stat});
total += sys_stat.cnt;
memset(&sys_stat, 0, sizeof(sys_stat));
bpf_map_update_elem(stats_fd, &nxt_key, &sys_stat, BPF_ANY);
key = nxt_key;
}while (0 == bpf_map_get_next_key(stats_fd, &key, &nxt_key))这里循环条件一开始就调用next_key，但是key一开始是0，next_key就是1，并且后面用来查询stats_fd这个map一直都用的next_key，也就是说后面的代码永远都不会去获取key=0的stats_fd，所以最后的结果永远都不会有系统调用号为0的read，这个问题是我用syscall-stat去追踪ls的系统调用的时候发现没有read这个系统调用，ls是查询功能，所以理论上来说肯定会调用read这个系统调用的。后来我调整遍历逻辑，确保从代码可以从第一个 key 开始完整遍历。再去使用syscall-stat去追踪ls的系统调用时，结果中就有read这个系统调用了
| { | ||
| pr_error("Error polling ring buffer: %d\n", err); | ||
| sleep(5); // Sleep before retrying | ||
| std::this_thread::sleep_for(std::chrono::microseconds(5)); // Sleep before retrying |
There was a problem hiding this comment.
因为我觉得这里的sleep(5); 是ring_buffer__poll() 出错后，等待5秒之后再重新试一下，避免错误路径一直占用cpu，但是5s我觉得太久了，会影响使用性能所以改成了5微秒，如果感觉太短也可以改回5s
| {"pid2pathhash",{sizeof(pid_t), sizeof(u32), 1024, BPF_MAP_TYPE_LRU_HASH}}, | ||
| {"logs", {0, 0, 1024 * 1024, BPF_MAP_TYPE_RINGBUF}}, | ||
| }; | ||
| static std::atomic<int> *condition; |
There was a problem hiding this comment.
condition这个使用了，这个是.cpp代码和test代码用来同步状态的
{"pid2pathhash",{sizeof(pid_t), sizeof(u32), 1024, BPF_MAP_TYPE_LRU_HASH}},
{"logs", {0, 0, 1024 * 1024, BPF_MAP_TYPE_RINGBUF}},这两个确实没使用，我现在删除
| // stats | ||
|
|
||
| while (0 == bpf_map_get_next_key(stats_fd, &key, &nxt_key)) | ||
| int ret = bpf_map_get_next_key(stats_fd, NULL, &nxt_key); |
13a61c2 to
fd937c9
Compare
yuKing123-king
changed the title
fd937c9 to
e0ac09a
Compare
1.多余静态变量已删除 |
xu-lang
left a comment
xu-lang
left a comment
There was a problem hiding this comment.
问题还是很多，以上只是部分，更严重的是修改思路框架设计过于随意，过于依赖全局变量
| free(buf); | ||
| exit(0); | ||
| break; | ||
| return 1; // Indicate that help was displayed |
There was a problem hiding this comment.
- 原来这里是exit(0)，exit和return的区别是，exit会立即终止当前线程，后续代码全部不执行，而return是终止当前函数，后面代码正常执行。
- 如何这里还是exit的话，线程终止后面代码不执行，没有相关状态变量传到syscall-stat-test.cpp中，syscall-stat-test.cpp会正常执行，但是无法区分测试失败是因为"用户请求了帮助"还是"程序崩溃"，所以每个地方都需要一个返回值参数返回给syscall-stat-test.cpp区分是什么原因引起的测试失败。
- 如果这里一定需要exit(0)这样的语义，这里可以通过定义BUILTIN这个宏来继续使用exit(0)。
| stats_fd = bpf_get_map_fd(obj->obj, "syscall_stat", goto err_out); | ||
|
|
||
| #ifdef BUILTIN | ||
| *filter_fdp = filter_fd; |
There was a problem hiding this comment.
为什么把fd传出去，外面保证fd的生命周期了吗?是否导致fd泄漏
| *condition = 1; | ||
| while(!exit_flag) | ||
| { | ||
| std::this_thread::sleep_for(std::chrono::microseconds(5)); |
| } | ||
|
|
||
| #ifdef BUILTIN | ||
| *conditionPrint = 1; |
There was a problem hiding this comment.
不要这样传参出去，一堆这种出参，极其ugly
std::atomic<int> *conditionp,
std::atomic<int> *conditionPrintP,
std::atomic<bool> **exit_flagp,
int *filter_fd,
int *stats_fd
e0ac09a to
20d00b5
Compare
| local_map_info = { | ||
| {"filter", {sizeof(u32), sizeof(struct Rule), 1, BPF_MAP_TYPE_HASH}}, | ||
| {"syscall_stat",{sizeof(u32), sizeof(struct info), 453, BPF_MAP_TYPE_ARRAY}}, | ||
| {"logs", {0, 0, 1024 * 1024, BPF_MAP_TYPE_RINGBUF}}, |
There was a problem hiding this comment.
这里的filter和syscall_stat和logs，虽然在当前代码中没有使用，这个local_map_info是传给mock.cpp去生成3个伪map供syscall-stat-test.cpp中使用
| char *end = nullptr; | ||
| errno = 0; | ||
| long val = strtol(optarg, &end, 10); | ||
| if (end == optarg || *end != '\0' || errno == ERANGE || val < 0 || (unsigned long)val > UINT32_MAX) |
There was a problem hiding this comment.
这里的修改是为了检验用户输入负数，字母等非法参数，例如-1，12abc，abc这样的参数，可以提示用户参数输错
| free(buf); | ||
| exit(0); | ||
| break; | ||
| return 1; // Indicate that help was displayed |
There was a problem hiding this comment.
- 原来这里是exit(0)，exit和return的区别是，exit会立即终止当前线程，后续代码全部不执行，而return是终止当前函数，后面代码正常执行。
- 如何这里还是exit的话，线程终止后面代码不执行，没有相关状态变量传到syscall-stat-test.cpp中，syscall-stat-test.cpp会正常执行，但是无法区分测试失败是因为"用户请求了帮助"还是"程序崩溃"，所以每个地方都需要一个返回值参数返回给syscall-stat-test.cpp区分是什么原因引起的测试失败。
- 如果这里一定需要exit(0)这样的语义，这里可以通过定义BUILTIN这个宏来继续使用exit(0)。
| Usage(argv[0]); | ||
| free(buf); | ||
| exit(-1); | ||
| return -1; |
| int slen = strlen(sys_tbl[i]); | ||
| if (!sys_tbl[i]) | ||
| continue; | ||
| int slen = strlen(sys_tbl[i]); |
There was a problem hiding this comment.
这里的修改是防止sys_tbl[i]是NULL，导致strlen(NULL)
| if (total) | ||
| { | ||
| printf("\ntotal: %d\n", total); | ||
| printf("\ntotal: %u\n", total); |
There was a problem hiding this comment.
这里的修改是因为上面定义的total是u32 total = 0;，是u32类型不是int类型
| if (0 != syscall_stat_bpf::attach(obj)) | ||
| { | ||
| exit(-1); // Attach BPF program | ||
| goto err_out; // Attach BPF program |
There was a problem hiding this comment.
这里不使用exit(-1)是因为如果上面的syscall_stat_bpf::open_and_load();成功了，但是syscall_stat_bpf::attach(obj)失败了，但是使用exit(-1)的话，会终止整个程序，导致后面的obj释放无法执行，造成泄露
| stats_fd = bpf_get_map_fd(obj->obj, "syscall_stat", goto err_out); | ||
|
|
||
| #ifdef BUILTIN | ||
| runtime_state->fds_promise.set_value(SyscallStatFds{filter_fd, stats_fd}); |
There was a problem hiding this comment.
- 这里向外传filter_fd和stat_fd的原因是，syscall-stat-test.cpp需要这两个fd去查询mock.cpp生成的rule和stat 这两个map的数据，以便向 map 中注入测试事件（bpf_map_update_elem(stats_fd, ...)）并验证过滤规则（bpf_for_each_map_elem(filter_fd, ...)）。
- 这里只是传了两个整数值（fd 的拷贝），filter_fd和stat_fd的生命周期依旧还是本程序中控制，在 err_out 标签处通过 syscall_stat_bpf::destroy(obj) 统一释放，测试线程仅持有 fd 的整数副本用于读写操作，不负责关闭，因此不会造成 fd 泄漏或双重释放。
|
/oc |
1 similar comment
|
/oc |
|
User rwenz2004 does not have write permissions |
|
/review 检视一下 |
|
/review |
|
Preparing review... |
1 similar comment
|
Preparing review... |
|
/review |
|
Preparing review... |
|
/review |
|
Persistent review updated to latest commit 967e9da |
967e9da to
026ce7f
Compare
|
/review 检查一下当前代码存在哪些安全风险的问题，代码不规范的地方 |
|
Persistent review updated to latest commit 026ce7f |
026ce7f to
cb5591a
Compare
|
/review 检查一下当前代码存在哪些安全风险的问题，代码不规范的地方 |
|
Preparing review... |
|
/review 检查一下当前代码存在哪些安全风险的问题，代码不规范的地方 |
|
Preparing review... |
cb5591a to
c87fcf2
Compare
Signed-off-by: Wang Yu <wangyu6@uniontech.com>
c87fcf2 to
9b69265
Compare
|
你好，这是对 libdkapture#115 fix(syscall-stat): improve builtin flow 的评审。 本轮为 DKapture 两仓（libdkapture / dkapture-bpf）全部以 fix 开头 open PR 的批量评审，共 17 个；汇总表如下，本 PR 加粗。逐项详评见分隔线下方。
本 PR 评审详情总体结论: 参数校验强化（:203-241）、handle_event 越界加固（:251-265）、exit 改返回值方向正确；作者已按评审把 key-0 修复拆出为 #119、删未用静态变量、去掉 BUILTIN_LOCAL 宏。已通读全部 24 条 issue 评论与 25 条行级评论，以下为增量意见。 主要问题:
次要建议:
亮点: handle_event 三重校验（:251-265）消除基线越界读；pthread_create 失败处理与 t1 清理（:534-546）、attach 失败不再 exit（:505-508）修复基线资源泄漏。 commit message: 问题说明——最终 commit 仍为 "test: improve builtin flow"（9b69265e），rwenz2004 07-27 已指出应为 fix，作者 07-31 答应改但未兑现，且与 PR 标题 fix(syscall-stat) 不一致，仓库近期提交均为 fix(scope): 格式。 已有讨论: 共识——BUILTIN_LOCAL 宏去掉改 static（rwenz2004 07-31，已改）；无用静态变量删除（xu-lang 08-04，已删）；key-0 修复拆独立 PR（xu-lang 08-04，即 #119）。未决——xu-lang 08-05 对 help 返回 1 的异议作者仅解释未回退；fd 传出泄漏疑虑作者已解释（整数副本、destroy 统一释放）；xu-lang 对 cv.wait 的“空转”质疑未获直接回复（cv.wait 实为阻塞非忙等，真正问题是无超时）；bot persistent review 指出的错误路径返回值未设置，作者部分修复，残留见上 P1。 |
4 participants
对本次 syscall-stat 工具修改的内容