Skip to content

fix(net-filter): abort rule loading on invalid config - #151

Open
yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/net-filter-load-fail-rule
Open

yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/net-filter-load-fail-rule

Conversation

@yuKing123-king

Copy link
Copy Markdown
Contributor

修复 net-filter 配置加载失败时的规则回滚语义。

此前 load_rules() 在解析失败后为了避免半加载状态会调用 clear_rules(),但 clear_rules() 会清空整个 rules map。如果调用方在 load_rules() 前已经通过 add_rule()/update_rule() 添加过规则,加载配置失败时这些已有规则
也会被误删。

本次修改在 load_rules() 开始时记录 first_key,只在失败路径删除本次 load_rules() 成功添加的 key 区间,并将 key_cnt 回滚到加载前状态。这样可以保证配置加载失败时不会残留本次部分加载的规则,同时不会影响调用前已经
存在的规则。

同时放宽空行/注释行判断,支持跳过前导空格后的注释行和纯空白行,避免手写配置中常见的缩进注释或空白行导致整个配置加载失败。

验证:
先通过 add_rule() 添加一条 keep_rule,再调用 load_rules() 加载包含合法规则和 invalid-rule 的配置。load_rules() 返回 false,最终 rules map 中只保留 keep_rule,证明失败时仅回滚本次加载的规则,没有清空已有规则。

Signed-off-by: Wang Yu <wangyu6@uniontech.com>
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

内存安全:line 指针被 line++ 修改后,free(line) 释放非原始分配指针,属于未定义行为,可能导致堆损坏。同时 getline 在后续迭代中复用已偏移的指针和未更新的 len,可能引发缓冲区溢出。

⚡ Recommended focus areas for review

逻辑错误

注释行和空白行被当作语法错误处理并中止加载,而非跳过。旧代码使用 continue 跳过 #\n 开头的行,新代码在跳过前导空格后遇到 #\n\0 时直接调用 rollback_loaded_rules() 并返回 false。这与 PR 描述中"支持跳过前导空格后的注释行和纯空白行"的目标完全相反,任何包含注释或空行的配置文件都会导致加载失败。

if (*line == '#' || *line == '\n' || *line == '\0')
{
	pr_error("syntax error in config file\n");
	rollback_loaded_rules();
	free(line);
	fclose(fp);
	return false;
}
内存安全

line 指针在跳过前导空格时被直接修改(line++),但后续错误路径中调用 free(line) 释放的是偏移后的指针而非 getline 分配的原始指针,属于未定义行为。此外,若未进入错误路径,下一次 getline(&line, &len, fp) 调用时 line 仍指向缓冲区中间位置,len 仍为原始缓冲区大小,可能导致缓冲区溢出。应使用单独的指针变量来跳过空格,保持 line 不变。

while (*line == ' ' || *line == '\t')
{
	line++;

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.

1 participant