fix(serde): reject table frames with mismatched schema/column counts - #444
Conversation
The table decoder set ncols = cols->len but bounded the rebuild loop by min(cols->len, schema->len), so a crafted frame whose schema-name count and column count disagree was silently accepted as a truncated table (extra columns dropped, or extra schema names ignored) instead of being rejected. Reject up front when schema->len != cols->len with a domain error, before building the table. Adds a serde.rfl regression that splices a 2-name i64 schema with a 1-column list into one table payload and asserts de rejects it.
singaraiona
left a comment
There was a problem hiding this comment.
Reviewed. The guard itself is correct, well-placed (after both decodes, before ray_table_new), and the error path releases both schema and cols symmetrically. CI is green — merging. A few notes, the first two are small in-PR cleanups that can go in a follow-up, the rest are adjacent gaps in the same threat model worth filing:
-
Dead loop bound: with the guard in place, the
&& i < schema->lenclause in the build loop (serde.c:814on dev) is unreachable —ncols == schema->lenalways holds. Worth removing so the guard reads as the single mechanism; as it stands the min()-style bound suggests truncated frames are still tolerated here. -
Test hard-codes wire layout: the
.rflsplice bakes in the 16-byte header,sizeat byte offset 8, payload-fits-in-one-byte, and little-endianness. Ifray_ipc_header_tever changes,(de _s74f)starts failing on a bad header and!- domainstill passes — the test goes green while no longer exercising the mismatch. The C harness intest_store.c(test_serde_table_dict_de_errors) already builds frames viasizeof(ray_ipc_header_t)and would stay honest; consider moving/duplicating the case there. -
Same invariant, ragged rows: the guard enforces name/column count agreement but nothing checks the columns' lengths agree. A crafted frame with columns of length 1000 and 1 still decodes;
ray_table_nrowsreports column 0's length and row-wise ops read past the short column. The splayed loader explicitly rejects this (splay.c"torn overwrite?" check) — serde is now the odd decoder out. -
DICT branch has the sibling bug: the dict decode (
serde.c:833) builds the 2-slot block directly with nokeys->len == vals->lencheck (nor thatvalsis a LIST). A crafted dict with more keys than vals makesray_dict_remove/sym probes indexvalsby a keys-bounded idx — OOB read/release. Strictly worse than the table case this PR fixes. -
Decode errors are swallowed and leaked on the IPC path: in
ipc.cthe dispatch is gated onif (msg && !RAY_IS_ERR(msg))with no else — the newdomainerror object is neverray_error_free'd (ray_releaseis a no-op on errors) and the client gets a plain null instead of the error. Remote-repeatable leak, made trivially reachable by this PR. Same pattern in the response-frame branch (~ipc.c:918).
How it looks from the user's side
de— and object file-load / IPC decode, which share the same path — accepts a serialized table whose schema-name count and column count disagree (the kind of frame a truncated read or a partially-written blob leaves behind) and silently loads it as a truncated table instead of erroring.Reproduced with a hand-crafted frame carrying 2 schema names but 1 column:
Before — the decoder loops over
min(names, columns), so the extra name is dropped and the corrupt/truncated frame deserializes into a plausible-but-wrong 1-column table, with no error:After — the mismatch is rejected up front:
The danger is that the truncated table is indistinguishable from a real one, so it flows on into joins/queries/aggregations silently, rather than failing at the point the corruption entered.
Fix
ray_de_raw's table case setncols = cols->lenbut bounded the rebuild loop bymin(cols->len, schema->len), so a frame with unequal counts was accepted as the shorter of the two (extra columns dropped, or extra schema names ignored). Reject up front whenschema->len != cols->lenwith adomainerror, before building the table.Tests
Adds a
serde.rflregression that splices a well-formed 2-name i64 schema with a well-formed 1-column list into one table payload and assertsderejects it.Full ASan+UBSan suite green:
3713 of 3714 passed (1 skipped, 0 failed).