fix(serde): validate table/dict counts and lengths on decode - #452
Conversation
Follow-up to the schema/column-count guard, hardening the same decode paths against crafted or truncated frames (review of RayforceDB#444): - Table: drop the now-dead `&& i < schema->len` loop bound (the count guard makes ncols == schema->len), so the guard reads as the single mechanism. - Table: reject ragged columns — every column must have the same length (the row count), else ray_table_nrows reports column 0's length while row-wise ops read past the shorter columns. - Dict: reject a keys/values count mismatch. A probe finds a key index in [0, keys->len) and reads the value at that index, so more keys than values is an out-of-bounds read/release of the value block. vals may be a LIST of columns or a flat value vector, so only the length is checked and no valid dict is rejected. Tests: adds store/serde_container_count_mismatch in test_store.c, which builds frames from real serialized sub-objects via sizeof(ray_ipc_header_t) — table count mismatch, ragged columns, dict key/value mismatch — and removes the brittle wire-layout-hard-coded .rfl case it replaces.
singaraiona
left a comment
There was a problem hiding this comment.
Good direction — this closes the exact gaps flagged in the #444 review — but the central check doesn't yet establish its own invariant, and a couple of the test changes weaken coverage:
-
The ragged-column check is bypassable by atom columns (
serde.c:819): it comparescps[i]->lenwithout requiring the column to be a vector. In aray_t,lenaliases the value union, so a crafted TABLE frame whose column LIST is all I64 atoms with identical payload V passes the loop (every "length" is V),ray_table_add_colaccepts atoms (it rejects only NULL/error), andray_table_nrowsreports V rows over zero data — any row-wise op then reads ~8·V bytes out of bounds. Requirecps[i]->type > 0for every column, index 0 included. -
The on-disk decoder stays open (
col.c:578):col_read_recursivedecodes the same TABLE/DICT shapes from files with none of the three new checks — it readsnrowsand discards it ((void)nrows), adds columns with no length comparison, and hands unchecked keys/vals to the dict constructor. A corrupt or crafted column/splay file yields exactly the ragged table this PR forbids over the socket. The durable fix is to put the invariants in the shared constructors (ray_table_add_col,ray_dict_new) so both decoders — and any future one — are closed at once. -
The migrated tests assert only
RAY_IS_ERR(test_store.c:3232): any decode failure passes, including a bad splice or a header-stage rejection, so the tests can go green without reaching the new guards — the deleted.rfltest asserted thedomainclass, so this is strictly weaker. Assert the class plus a message substring. Related in the same helper:hdr->endian = 0should beRAY_SERDE_ENDIAN(sibling attest_store.c:1939does it right), andserde_splice_containerdereferencesray_vec_new's result unchecked. -
test_pool.c:640is out of scope and now tautological:mask != 0is implied by thecalls == 3assert two lines above, so the worker-id coverage the old(mask & 0x1u)check provided is gone. If the old form was flaky,(mask & ~0x3u) == 0keeps the invariant without the flake — but either way this belongs in its own PR.
Minor: the column vectors appended inline in the new tests leak their caller reference (ray_list_append retains; file convention is name-then-release), and cps/col_ptrs derive the same pointer array twice ~20 lines apart.
singaraiona
left a comment
There was a problem hiding this comment.
Re-reviewed the rework: the invariants now live in the shared constructors — table_col_is_valid in ray_table_add_col closes the atom-column bypass (len aliasing the value union), ray_dict_new enforces shape and key/value count, and ray_table_validate_rectangular covers count + column type + row-length agreement for BOTH decoders, the wire path and the on-disk col load table path. The out-of-scope test_pool.c edit is gone. This is the durable version of the fix — approving; will merge after the branch updates against dev.
Follow-up to #444, addressing the review notes on the same decode paths.
How it looks from the user's side
Before this PR,
deand object/file-load decode could accept corrupt serialized TABLE/DICT payloads and build plausible but broken in-memory objects. A user would not see an error at load time; the failure surfaced later as bad rows, an out-of-bounds read, or an ASan crash.Concrete cases:
After this change, corrupt payloads fail where they enter the system:
The same checks now apply to recursive on-disk column/table decoding, so a crafted
.col/splayed file cannot bypass the serde guard.Fix
ray_table_add_colto be list/vector-like, so atom columns are rejected through the shared constructor.ray_table_add_col's ownership contract consistent on the new invalid-column error path: the input table ref is consumed, matching the later append-failure paths and avoiding leaks in recursive on-disk decode.ray_table_validate_rectangularand call it from both serde TABLE decode and recursive on-disk TABLE decode, so ragged decoded tables are rejected by shared table logic without breaking existing runtime table use cases that intentionally construct short/broadcast columns.ray_dict_new: keys and values must be list/vector-like and their lengths must match. This closes serde, recursive on-disk decode, snapshot construction, and future callers through one constructor path.Tests
store/serde_container_count_mismatchto assert both error class and message substring.RAY_SERDE_ENDIAN, checkray_vec_new, and release inline-appended vectors.Full ASan+UBSan suite: