Repository navigation
fix(datatype): 迁移/复制载荷改长度前缀框架，坏载荷一律 fail-closed - #89
Conversation
EntelligenceAI PR Summary将 MIGRATE 与复制键流使用的载荷格式改为“类型标签 + 长度前缀记录”，确保包含换行或任意原始字节的元素能够完整往返。编解码逻辑统一收拢到 flowchart TD
classDef newBehavior fill:#dcfce7,stroke:#16a34a,color:#14532d;
Sources["MIGRATE / replication key streams"] --> Serialize["CacheObject serialize"]
Serialize --> Payload["Length-prefixed payload"]
Payload --> Restore["RESTORE command"]
Restore --> Deserialize["CacheObject deserialize"]
Deserialize --> Validate["Strict validation: complete records and exact end"]
Validate --> Store["Persist only valid object"]
Tests["Round-trip and malformed-payload tests"] --> Serialize
Tests --> Deserialize
class Serialize,Payload,Restore,Deserialize,Validate,Store,Tests newBehavior;
Review Scorecard
Issues found:
Safe to merge with a low blast radius; code quality rated Needs Work (3/5): the review found nothing to flag. 2 comments from earlier reviews are still open. Need to merge before these are addressed? Anyone with write access can comment Evaluated against
|
WalkthroughThis PR centralizes RESTORE payload parsing through CacheObject deserialization and introduces a length-prefixed serialization format that preserves newline-containing values and ZSET score precision. Deserialization now validates payloads atomically and fails closed on malformed, truncated, unsupported, or trailing data, with expanded unit and contract coverage. ChangesFiles
Reworked CacheObject serialization to use length-prefixed records for strings, lists, hashes, sets, and sorted sets, preserving embedded newlines and full-precision finite ZSET scores. Added the public Files
Replaced duplicated stream-based RESTORE parsing with Files
Expanded serialization round-trip coverage for strings, lists, hashes, sets, and sorted sets, including newline-containing members and values and high-precision scores. Added fail-closed tests for truncated, legacy-formatted, empty, unknown, malformed, and trailing-data payloads, verifying that partial object state is never retained. Files
Added and registered RESTORE contract tests covering successful STRING round trips and rejection of garbage or truncated payloads without creating partial keys. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| err = "bad LIST element count"; | ||
| return false; | ||
| } | ||
| std::vector<std::string> elems; |
There was a problem hiding this comment.
A small attacker-controlled payload such as LIST\n20\n18446744073709551615 makes reserve(count) request an enormous capacity before any element is validated, throwing or attempting a huge allocation instead of fail-closing normally.
Prompt to fix with AI
Copy this prompt into your AI coding assistant to fix this issue.
In src/datatype/object.cpp:591-595, prevent an untrusted LIST count from causing oversized allocation. Validate that the count is feasible from the remaining payload bytes (or apply an explicit safe bound) before calling reserve, and return false with an error for impossible counts.
| CacheObject bad; | ||
| std::string bad_err; | ||
| EXPECT_TRUE(!bad.deserialize(payload + "extra", bad_err)); |
There was a problem hiding this comment.
This only checks that trailing bytes return false. deserialize sets the string before checking trailing bytes, so the test passes even though the rejected object still contains the value; assert bad remains empty/unchanged after failure.
Prompt to fix with AI
Copy this prompt into your AI coding assistant to fix this issue.
In test/datatype_test/object_test.cpp around lines 528-530, strengthen the trailing-byte failure test to assert that `bad` has no deserialized value and remains otherwise unchanged after `deserialize` returns false. This must catch mutation before trailing-byte validation.
c76b321 to
197270e
Compare
载荷原来每条记录以 "\n" 结尾，于是内容里带换行的元素会被提前收条：
LPUSH k "a\nb" → serialize() 给 "LIST\n1\na\nb\n"
解码器 → 按声明的数量 1 读到 "a"，"b" 丢掉，然后照样回 +OK
MIGRATE 和复制键流都走这份载荷，所以那是静默的数据缺损，不是格式问题。
另一半是解析侧遇到截断就 break，交出已读到的部分仍然报成功。
改法：
- 载荷 = 一行类型标签 + 若干条 "<字节数>\n<原始字节>" 记录。读侧要求每条记录的
字节数都够，且读完必须正好落在末尾 —— 多一个字节少一个字节都是失败。
- 编解码搬进 CacheObject::serialize()/deserialize()。原来解码逻辑抄在
RestoreCommand 里，两份框架描述互相对不上；搬过来之后 RESTORE 只做参数校验和落库。
- 顺手去掉 "空类型标签当 STRING" 那个分支：垃圾载荷现在报错，不再建出一个空串键。
- ZSET 分数读侧换成 from_chars 严格解析并拒 NaN/Inf，与 #86 的 ZADD 入参判断一致；
写侧仍是 %.17g，往返按位一致。
不留旧格式的兼容垫片：这两份载荷只存在于内存里的迁移请求与复制 backlog，没有落盘，
所以让"另一种框架的字节流"被明确拒绝比猜它是什么更安全。
判伪：
- object_test.cpp 新增 4 组（V3Tests 已挂 gate 标签）。含换行/回车/空串/尾随换行的
元素、带换行的 hash 字段与 set 成员、0.123456789 的分数，都要求逐字节原样读回；
旧框架那串字节必须被拒且不许留下半个键；截一字节、空载荷、无标签、未知类型、
分数不是数字，五种都判失败。
- contract_test.cpp 新增 3 组：正常载荷 → +OK 且 GET 回原值；垃圾载荷 → 报错且
EXISTS 为 0（改动前是建键 + 回 +OK）；声明 3 字节只给 2 字节 → 报错且不留部分键。
本机做实：把 object.cpp 与 restore_cmd.cpp 按 CMakeLists 那组
-Wall -Wextra -Werror -Wconversion -Wshadow -Wdouble-promotion -Wsign-conversion 编过；
另用一个只链 object.cpp 的驱动跑完 20 项断言（含上面全部往返与 fail-closed）全过，
驱动跑完即删。契约用例要 fork 真服务器，由 CI 学。
197270e to
2d93355
Compare
* ci: 把文档与代码的一致性变成 required check 的一部分 check_consistency.py 原有六组判据都不看文档，所以 #94 那轮人工清扫之后没有任何 东西防止它再脱钩：加一个命令不更 api.md、重命名 ctest target 不更 README、把总线 端口当客户端口写进示例、INFO 版本号与文档各说各话——全都静默通过。 新增第 7 组（只比可数的量，不比措辞）： - api.md §2 索引的命令名集合 + 头部声明的总数 == CommandFactory 注册表 - README § 单独测试 的 ctest 名单与"共 N 条" == test/CMakeLists.txt 的 CC_TESTS - 指向 .md 的相对链接必须能解析（只查 .md 目标，避开正文里形似链接的代码片段） - 文档里 redis-cli -p <总线端口> 判红；"默认端口"必须等于 conf 的 port - api.md 示例的 concurrentcache_version 必须等于 string_cmd.h 里那个字面量 - 文档出现的 ghcr.io / docker.io 地址，必须有 workflow 真的往那儿推 注册表仍然取探针的运行时答案，不在这里改用正则。 判据自己也可能坏（写坏的正则 = 永远绿），所以配一个注入验证脚本 scripts/ci/check_docs_gate_injection.py：逐条把 docs/*.md 改错、要求只有对应那 一条报 ::error::、再按原字节还原，最后确认还原后重新变绿。它作为 consistency job 的一步跑，不开容错。 实测：基线 exit 0、无 error；9 条注入各只弄红一条；还原后 exit 0。 --docs-only 是本地没有 POSIX 构建时的入口（CI 不走它，1) 那条仍会把正则与探针 对齐），本地跑过的结果与 CI 一致。 * fix(cluster): 总线参数帧改成长度前缀（v2），含 0xC0 的复制写不再被静默切断 参数区以前是"各参数用裸 0xC0 字节拼接、无转义"。而复制/迁移的参数本来就允许任意 字节：send_replication_args() 把整条 RESP 数组命令塞进单个 kRepData 参数，value 里只要有一个 0xC0，这条参数就在总线层被切成两条，副本端于是执行一条错位命令、或者 因为参数不够而什么都不做。没有报错、没有重试、日志里也看不出来 —— 表现是主从静默 发散，而这是数据正确性问题不是可用性问题。 改法：参数帧换成 "<条数>\n" + 每条 "<字节数>\n<原始字节>"，与 #89 给 RESTORE 载荷 用的框架同构。版本走 header.version（v2），接收端按版本分支： - v2 按新框架解，任何一圈不完整 / 条数不符 / 末尾有余字节 → 判畸形并断链 - v1 仍按 0xC0 切，只作为升级窗口内老节点的控制面兼容路径（gossip 参数是 ASCII） - 其它版本号 → 断链。按任何一套规则去试都会解出错位参数并被下游当真命令执行 send_msg() 强制把 version 写成 v2：参数区是它自己按 v2 写的，版本必须与实际框架 一致，不能信调用方留下的值。帧长与真实写入字节数共用 bus_args_frame_bytes()， 两侧不会再各算一套（header.length 与实帧脱钩是 P0-1 那一类错位的一半成因）。 兼容性上的取舍要说清楚：任何带内转义都必须同时转义转义符本身，而老节点看不懂新 转义，所以"新老节点都能传二进制参数"在协议上做不到。混版本期间数据面（复制 / CLUSTER MIGRATE）不受支持，需要整集群一起升级；这是显式断链而不是静默错位。反过 来，控制面在升级窗口内照常，这条是 v1 分支保留的意义。 测试（cluster_link_framing_test.cpp，已接进 V3Tests / ctest -L gate）： - v2 往返：混入 0xC0、'\n'、'\r' 和一个空参数，逐字节相等 - 帧长交叉核对：用 std::to_string 的位数独立算一遍 300 条参数的帧长；截 1 字节、 条数写少都必须解失败（而不是交出一个截断参数） - 未知版本断链且一条都不投递 - v1 遗留帧仍能解出 ASCII 参数（兼容路径没被写坏） - 发送侧端到端：send_msg 写进 socketpair 的原始字节，从对端读回来按 header 声明的 版本能解出原参数——这条盯的是"编码侧与 header.length/version 脱钩" 本地证据：把 bus_args_* 四个函数抽成独立 TU，用仓库那套警告 （-Wall -Wextra -Werror -Wconversion -Wshadow -Wdouble-promotion -Wsign-conversion） 编译通过，并跑了一个往返自测：v2 原样回来、v1 对 {"SET","key",0xC0+"abc"} 确实切 成 4 条且 value 首字节被当分隔符吃掉（老 bug 的形状被复现，说明判据不是空的）。 整包构建与 V3Tests 由 CI 跑（本机是 MinGW，sys/epoll.h 编不了）。 文档同步：cluster.md §7 写明 v2/v1/未知版本三条分支、send_msg 强制标版本、以及 混版本的限制；§5.2 和不变量表里"含 0xC0 的写会被切断（登记在案）"改成已修复； api.md §14 那条"仍会在总线层被切断"一起改掉；replication_mgr 的两处注释原来声称 RESP 数组已经解决了 0xC0，那是错的（RESP 只解决命令行内部的分隔），一并纠正。 * fix(cluster): 帧版本常量要定义在使用它之前 ClusterMsg 的构造函数里写 header.version = kBusFramingVersion，而这两个常量当时 声明在 struct ClusterMsg 之后 —— 头文件里的类内成员函数体是在整个类定义完之后才 编译的，所以这不是看起来能过的问题，是彻底不过：CI 的 build-release / build-assert / gate-tests / asan-smoke 四条全在第一个用到 cluster_link.h 的翻译单元上就断了。 把常量块移到 struct ClusterMsg 之前。函数声明留在原位（在结构之后，调用点都在 .cpp 里）。 顺带说明本地为什么没抓到：MinGW 没有 sys/socket.h 与 sys/epoll.h，整个 TU 编不了； 上一轮把 bus_args_* 抽成独立 TU 只能证类型与转换，证不了同一头文件里的声明顺序。 这条只能靠 CI 的 g++ 编译，所以下一步是推上去看编译门。 * test(cluster): read_exact 一次只读到还缺多少，别把正文一起吸走 第一次 CI 跑（gate-tests / asan-smoke 同时红）报的就是这条： EXPECT_EQ(head_bytes.size(), sizeof(ClusterMsgHeader)) (actual: 2143, expected: 2120) lambda 里 ::read(fd, buf, sizeof(buf)) 允许一次读回 4096 字节，socketpair 上那一帧 （header 2120 + 参数区 23）于是被整体吸进 head_bytes。第二个 read_exact(23) 只能读到 空、断言全歪。生产代码没问题——send_msg 写出去的帧、以及按 header 声明长度解回来的 参数都是对的；错在测试自己的读取循环。 改成每次最多只读 want - got.size() 字节，剩余字节留给下一次 read_exact。
载荷原来每条记录以
\n结尾，内容里带换行的元素会被提前收条：MIGRATE 和复制键流都走这份载荷，所以这是静默的数据缺损，不是格式好不好看的问题。解析侧还有另一半：遇到截断就
break，交出已读到的部分仍然报成功；type.empty()被当成 STRING，所以垃圾载荷会建出一个空串键。改法
<字节数>\n<原始字节>记录。读侧要求每条记录的字节数都够，且读完必须正好落在末尾——多一字节少一字节都失败。CacheObject::serialize()/deserialize()。原来解码逻辑抄在RestoreCommand里，两份框架描述互相对不上（本次要改框架就得同时改两处），搬过来之后 RESTORE 只做参数校验和落库。from_chars严格解析并拒 NaN/Inf，与 fix(command): ZADD 不再收 nan/inf，也不再静默吞掉尾巴塞字符 #86 的 ZADD 入参判断一致；写侧仍是%.17g，往返按位一致。判伪
object_test.cpp新增 4 组（V3Tests 带 gate 标签）：含换行/回车/空串/尾随换行的元素、带换行的 hash 字段与 set 成员、0.123456789的分数，都要求逐字节原样读回；旧框架那串字节必须被拒且不许留下半个键；截一字节 / 空载荷 / 无标签 / 未知类型 / 分数不是数字 五种都判失败。contract_test.cpp新增 3 组：正常载荷 →+OK且 GET 回原值；垃圾载荷 → 报错且EXISTS为 0（改动前是建键 + 回 +OK）；声明 3 字节只给 2 字节 → 报错且不留部分键。本机做实
object.cpp、restore_cmd.cpp、object.h按 CMakeLists 那组-Wall -Wextra -Werror -Wconversion -Wshadow -Wdouble-promotion -Wsign-conversion编译干净；另用一个只链object.cpp的驱动跑完 20 项断言（上面所有往返与 fail-closed 场景）全过——驱动是临时的，跑完已删。契约用例要 fork 真服务器，只能由 CI 学。还没做的那一半
CLUSTER MIGRATE目前仍然不删源键。原因不是忘了，是删不了：send_command_to_node()返回的是"发出去了没有"，总线上没有请求/回复关联通道（无 correlation id、无 promise/future），所以发送方永远不知道目标节点是接受了还是回了BUSYKEY。在没有回复通道的前提下补删除，等于在目标可能拒绝的情况下删掉源键——那是数据丢失，比现在的重复键更糟。正确的修法是给总线加一次带超时的请求/回复往返（Redis 的 MIGRATE 本来就有timeout参数做这件事），我按这个方向单独开一条 PR。