From c7ccc45b2aeca03366ec7e237b852164c99a9d03 Mon Sep 17 00:00:00 2001 From: Anton Date: Fri, 18 Sep 2026 00:32:59 +0200 Subject: [PATCH 1/2] fix(table): make the public table accessors total over ray_t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ray_table_nrows and ray_table_ncols segfault when handed a value that is not a table (#567). They guard NULL and errors but not the type tag, then decode the argument's payload as the two-slot {schema, cols} layout and dereference whatever comes out. The reporter framed this as a quirk of IPC-deserialized atoms, since a locally built ray_i64(42) was benign. The real trigger is just payload bytes that dereference badly — a plain I64 vector crashes deterministically: ray_t* v = ray_vec_from_raw(RAY_I64, poison, 4); ray_table_ncols(v); /* SIGSEGV at table.c:274 */ These are not holding a deliberate line. The rest of the layer already checks its own tag — ray_dict_keys rejects a non-RAY_DICT, ray_parted_nrows rejects a non-parted vector — so the table accessors are the outlier, and the same hole is in all eight of them, not just the two reported. An embedder sees one opaque ray_t* and has nothing to check against but the tag the accessor already holds. Add tbl_is_table() and route every public accessor through it: wrong tag gets the empty answer (0 / NULL / -1), mutators become no-ops, and none of them reads the payload as slots. Document the guarantee in rayforce.h, which previously said nothing either way, and point callers who need a diagnosable rejection at ray_table_validate_rectangular. No cost: the call sites are per-operation, not per-row (agg_engine.c:128's parallel-size test is the hot one), and the compare hits a field in the same cache line as the NULL check already there. Interleaved A/B over ClickBench q13/16/17/20/22/28 on the 10M splayed store, min-of-3 x 2 passes, shows no movement outside run-to-run spread. --- include/rayforce.h | 14 ++++++++++++- src/table/table.c | 28 +++++++++++++++++-------- test/test_table.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 9 deletions(-) diff --git a/include/rayforce.h b/include/rayforce.h index 2ab29271..e2c273eb 100644 --- a/include/rayforce.h +++ b/include/rayforce.h @@ -677,7 +677,19 @@ ray_err_t ray_sym_load(const char* path); ray_t* ray_env_get(int64_t sym_id); ray_err_t ray_env_set(int64_t sym_id, ray_t* val); -/* ===== Table API ===== */ +/* ===== Table API ===== + * + * The accessors below are total over ray_t: handed a value whose type is not + * RAY_TABLE — an atom, a vector, a dict, an error, NULL — each returns the + * empty answer (0, NULL, -1) and each mutator is a no-op. They never read the + * argument's payload as table storage, so an embedder holding an opaque + * ray_t* can call them without first establishing the type. + * + * ray_table_validate_rectangular() remains the way to get a *diagnosable* + * rejection: it returns a typed error naming what it got, where these + * accessors answer 0/NULL indistinguishably from a genuinely empty table. + * Use it when the difference matters. + */ ray_t* ray_table_new(int64_t ncols); ray_t* ray_table_add_col(ray_t* tbl, int64_t name_id, ray_t* col_vec); diff --git a/src/table/table.c b/src/table/table.c index 10a6c09f..73485a13 100644 --- a/src/table/table.c +++ b/src/table/table.c @@ -50,6 +50,18 @@ static inline ray_t** tbl_slots(ray_t* tbl) { return (ray_t**)ray_data(tbl); } +/* The public accessors below decode `tbl`'s payload as the two-slot + * {schema, cols} layout, so they are only meaningful for a RAY_TABLE. An + * embedder sees one opaque ray_t* and has nothing to check against, so each + * accessor is total over ray_t: a wrong tag gets the empty answer, never a + * read through a decoded payload (#567). Matches ray_dict_keys and + * ray_parted_nrows, which already check their own tags. Internal callers + * establish the type during dispatch and pay one predicted compare against a + * field already in the same cache line as `type`. */ +static inline bool tbl_is_table(ray_t* tbl) { + return tbl && !RAY_IS_ERR(tbl) && tbl->type == RAY_TABLE; +} + static inline ray_t* tbl_schema(ray_t* tbl) { return tbl_slots(tbl)[0]; } @@ -189,7 +201,7 @@ ray_t* ray_table_validate_rectangular(ray_t* tbl, const char* context) { * -------------------------------------------------------------------------- */ ray_t* ray_table_get_col(ray_t* tbl, int64_t name_id) { - if (!tbl || RAY_IS_ERR(tbl)) return NULL; + if (!tbl_is_table(tbl)) return NULL; ray_t* schema = tbl_schema(tbl); ray_t* cols = tbl_cols(tbl); if (!schema || !cols) return NULL; @@ -206,7 +218,7 @@ ray_t* ray_table_get_col(ray_t* tbl, int64_t name_id) { * -------------------------------------------------------------------------- */ ray_t* ray_table_get_col_idx(ray_t* tbl, int64_t idx) { - if (!tbl || RAY_IS_ERR(tbl)) return NULL; + if (!tbl_is_table(tbl)) return NULL; ray_t* cols = tbl_cols(tbl); if (!cols) return NULL; if (idx < 0 || idx >= cols->len) return NULL; @@ -221,7 +233,7 @@ ray_t* ray_table_get_col_idx(ray_t* tbl, int64_t idx) { * -------------------------------------------------------------------------- */ void ray_table_set_col_idx(ray_t* tbl, int64_t idx, ray_t* col_vec) { - if (!tbl || RAY_IS_ERR(tbl) || !col_vec) return; + if (!tbl_is_table(tbl) || !col_vec) return; ray_t** slots = tbl_slots(tbl); ray_t* cols = slots[1]; if (!cols || RAY_IS_ERR(cols)) return; @@ -240,7 +252,7 @@ void ray_table_set_col_idx(ray_t* tbl, int64_t idx, ray_t* col_vec) { * -------------------------------------------------------------------------- */ int64_t ray_table_col_name(ray_t* tbl, int64_t idx) { - if (!tbl || RAY_IS_ERR(tbl)) return -1; + if (!tbl_is_table(tbl)) return -1; ray_t* schema = tbl_schema(tbl); if (!schema) return -1; if (idx < 0 || idx >= schema->len) return -1; @@ -253,7 +265,7 @@ int64_t ray_table_col_name(ray_t* tbl, int64_t idx) { * -------------------------------------------------------------------------- */ void ray_table_set_col_name(ray_t* tbl, int64_t idx, int64_t name_id) { - if (!tbl || RAY_IS_ERR(tbl)) return; + if (!tbl_is_table(tbl)) return; ray_t** slots = tbl_slots(tbl); ray_t* schema = slots[0]; if (!schema || RAY_IS_ERR(schema)) return; @@ -269,13 +281,13 @@ void ray_table_set_col_name(ray_t* tbl, int64_t idx, int64_t name_id) { * -------------------------------------------------------------------------- */ int64_t ray_table_ncols(ray_t* tbl) { - if (!tbl || RAY_IS_ERR(tbl)) return 0; + if (!tbl_is_table(tbl)) return 0; ray_t* schema = tbl_schema(tbl); return schema ? schema->len : 0; } int64_t ray_table_nrows(ray_t* tbl) { - if (!tbl || RAY_IS_ERR(tbl)) return 0; + if (!tbl_is_table(tbl)) return 0; ray_t* cols = tbl_cols(tbl); if (!cols || cols->len <= 0) return 0; ray_t* first_col = ((ray_t**)ray_data(cols))[0]; @@ -313,6 +325,6 @@ int64_t ray_parted_nrows(ray_t* v) { } ray_t* ray_table_schema(ray_t* tbl) { - if (!tbl || RAY_IS_ERR(tbl)) return NULL; + if (!tbl_is_table(tbl)) return NULL; return tbl_schema(tbl); } diff --git a/test/test_table.c b/test/test_table.c index 97530cbe..209e8e14 100644 --- a/test/test_table.c +++ b/test/test_table.c @@ -564,6 +564,55 @@ static test_result_t test_table_accessors_null_and_err(void) { PASS(); } +/* Every public table accessor is total over ray_t: handed a value that is not + * a table it returns the empty answer, it does not read the payload as slot + * pointers. Issue #567 — an embedder sees one opaque ray_t*, so there is + * nothing to check against but the tag the accessor already has. The payload + * here is poisoned so that any accessor still treating it as slots dereferences + * 0x4141... and takes the process down rather than quietly returning garbage. */ +static test_result_t test_table_accessors_wrong_type(void) { + int64_t poison[] = {0x4141414141414141LL, 0x4141414141414141LL, + 0x4141414141414141LL, 0x4141414141414141LL}; + ray_t* vec = ray_vec_from_raw(RAY_I64, poison, 4); + TEST_ASSERT_NOT_NULL(vec); + + TEST_ASSERT_EQ_I(ray_table_ncols(vec), 0); + TEST_ASSERT_EQ_I(ray_table_nrows(vec), 0); + TEST_ASSERT_NULL(ray_table_schema(vec)); + TEST_ASSERT_NULL(ray_table_get_col(vec, ray_sym_intern("c", 1))); + TEST_ASSERT_NULL(ray_table_get_col_idx(vec, 0)); + TEST_ASSERT_EQ_I(ray_table_col_name(vec, 0), -1); + + /* Mutators must be no-ops rather than writes through a decoded payload. */ + ray_table_set_col_name(vec, 0, ray_sym_intern("c", 1)); + ray_table_set_col_idx(vec, 0, vec); + TEST_ASSERT_EQ_I(((int64_t*)ray_data(vec))[0], 0x4141414141414141LL); + + ray_release(vec); + PASS(); +} + +/* A dict shares the table's two-slot layout, so the accessors would happily + * read real pointers out of it and answer with a dict's shape. The tag is the + * only thing separating the two; assert it is honoured. */ +static test_result_t test_table_accessors_reject_dict(void) { + int64_t kraw[] = {1, 2, 3}; + int64_t vraw[] = {10, 20, 30}; + ray_t* keys = ray_vec_from_raw(RAY_I64, kraw, 3); + ray_t* vals = ray_vec_from_raw(RAY_I64, vraw, 3); + ray_t* d = ray_dict_new(keys, vals); + TEST_ASSERT_NOT_NULL(d); + TEST_ASSERT_EQ_I(d->type, RAY_DICT); + TEST_ASSERT_EQ_I(ray_dict_len(d), 3); + + TEST_ASSERT_EQ_I(ray_table_ncols(d), 0); + TEST_ASSERT_EQ_I(ray_table_nrows(d), 0); + TEST_ASSERT_NULL(ray_table_schema(d)); + + ray_release(d); + PASS(); +} + /* ray_parted_nrows on a plain (non-parted) vector returns vec->len directly. */ static test_result_t test_parted_nrows_plain_vec(void) { int64_t raw[] = {1, 2, 3, 4}; @@ -615,6 +664,8 @@ const test_entry_t table_entries[] = { { "table/set_col_name", test_table_set_col_name, table_setup, table_teardown }, { "table/set_col_name_shared", test_table_set_col_name_shared, table_setup, table_teardown }, { "table/accessors_null_and_err", test_table_accessors_null_and_err, table_setup, table_teardown }, + { "table/accessors_wrong_type", test_table_accessors_wrong_type, table_setup, table_teardown }, + { "table/accessors_reject_dict", test_table_accessors_reject_dict, table_setup, table_teardown }, { "table/parted_nrows_plain_vec", test_parted_nrows_plain_vec, table_setup, table_teardown }, { "table/nrows_empty_col", test_table_nrows_empty_col, table_setup, table_teardown }, { NULL, NULL, NULL, NULL }, From 142de18b69232cc1026871ae92ff199e6b596968 Mon Sep 17 00:00:00 2001 From: Anton Date: Fri, 18 Sep 2026 11:22:02 +0200 Subject: [PATCH 2/2] fix(table): guard ray_table_add_col too; state the contract per return type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit caught a real hole in the first commit. The header block I added promised totality for every declaration below it, and ray_table_add_col is in that list but was not converted — so the documentation invited exactly the call that corrupts memory: a non-table gets ray_cow'd, then new schema and cols pointers are written straight into the argument's payload. Worse than the crash #567 reported, and now advertised as safe. Guard it. It consumes its tbl ref and owns its result, so the wrong-tag answer is a typed "type" error rather than an empty one, releasing tbl on that path exactly as the bad-column path beside it already does. Rewrite the header block to say what each function actually returns on a wrong tag instead of asserting one blanket answer for a list whose members have different return contracts, and to spell out that add_col consumes tbl on the error path. Tests: table/add_col_wrong_type, written first and watched to fail in ray_vec_append on the decoded 0x4141... payload. Broadened table/accessors_reject_dict from three functions to the whole set, since a dict carries real pointers and was certifying less than the header claimed. Also corrects a stale comment the audit flagged in passing: table.h:30 said RAY_TABLE was 13, it is 98. make test: 3890 of 3890 passed. --- include/rayforce.h | 27 +++++++++++++++-------- src/table/table.c | 11 ++++++++++ src/table/table.h | 2 +- test/test_table.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 10 deletions(-) diff --git a/include/rayforce.h b/include/rayforce.h index e2c273eb..7b23814f 100644 --- a/include/rayforce.h +++ b/include/rayforce.h @@ -679,16 +679,25 @@ ray_err_t ray_env_set(int64_t sym_id, ray_t* val); /* ===== Table API ===== * - * The accessors below are total over ray_t: handed a value whose type is not - * RAY_TABLE — an atom, a vector, a dict, an error, NULL — each returns the - * empty answer (0, NULL, -1) and each mutator is a no-op. They never read the - * argument's payload as table storage, so an embedder holding an opaque - * ray_t* can call them without first establishing the type. + * Every function here that takes a `ray_t* tbl` is total over ray_t: handed a + * value whose type is not RAY_TABLE — an atom, a vector, a dict, an error, + * NULL — none of them reads the argument's payload as table storage. An + * embedder holding one opaque ray_t* can call any of them without first + * establishing the type. What a wrong tag yields differs by return contract: * - * ray_table_validate_rectangular() remains the way to get a *diagnosable* - * rejection: it returns a typed error naming what it got, where these - * accessors answer 0/NULL indistinguishably from a genuinely empty table. - * Use it when the difference matters. + * ray_table_ncols / ray_table_nrows -> 0 + * ray_table_col_name -> -1 + * ray_table_schema / ray_table_get_col* -> NULL + * ray_table_set_col_name / ray_table_set_col_idx -> no-op + * ray_table_add_col -> typed "type" error + * ray_table_validate_rectangular -> typed "type" error + * + * ray_table_add_col consumes its `tbl` ref on every path, the error path + * included, so a wrong tag releases the argument exactly as a bad column does. + * + * The two that return a typed error are the ones to reach for when the + * distinction matters: the accessors answer 0/NULL indistinguishably from a + * genuinely empty table, so they cannot tell you *why* the answer was empty. */ ray_t* ray_table_new(int64_t ncols); diff --git a/src/table/table.c b/src/table/table.c index 73485a13..10d39b6e 100644 --- a/src/table/table.c +++ b/src/table/table.c @@ -130,6 +130,17 @@ ray_t* ray_table_new(int64_t ncols) { ray_t* ray_table_add_col(ray_t* tbl, int64_t name_id, ray_t* col_vec) { if (!tbl || RAY_IS_ERR(tbl)) return tbl; + /* Unlike the accessors, this consumes `tbl` and owns its result, so the + * wrong-tag answer has to be a typed error rather than an empty one — + * same consume-and-error contract as the bad-column path below. Without + * it a non-table gets ray_cow'd and then has slot pointers written into + * its payload (#567). */ + if (tbl->type != RAY_TABLE) { + ray_t* err = ray_error("type", "table add_col: expected table, got %s", + ray_type_name(tbl->type)); + ray_release(tbl); + return err; + } if (!table_col_is_valid(col_vec)) { ray_release(tbl); return ray_error("domain", "table add_col: column must be list/vector-like, got %s", diff --git a/src/table/table.h b/src/table/table.h index 16eff4eb..dfd1c34a 100644 --- a/src/table/table.h +++ b/src/table/table.h @@ -27,7 +27,7 @@ /* * table.h -- Table operations. * - * A table has type = RAY_TABLE (13), len = current column count. + * A table has type = RAY_TABLE (98), len = current column count. * Data region: first sizeof(ray_t*) bytes = pointer to schema (I64 vector * of column name symbol IDs), then ncols * sizeof(ray_t*) = column vector * pointers. diff --git a/test/test_table.c b/test/test_table.c index 209e8e14..55d8415e 100644 --- a/test/test_table.c +++ b/test/test_table.c @@ -592,6 +592,40 @@ static test_result_t test_table_accessors_wrong_type(void) { PASS(); } +/* ray_table_add_col is the one mutator that cannot answer "empty": it consumes + * its table ref and returns an owned result, so the wrong-tag answer has to be + * a typed error, matching the bad-column path right beside it. Handed a + * non-table it previously ran ray_cow then wrote new slot pointers straight + * into the argument's payload — the same #567 hazard, but corrupting rather + * than only crashing. */ +static test_result_t test_table_add_col_wrong_type(void) { + int64_t poison[] = {0x4141414141414141LL, 0x4141414141414141LL, + 0x4141414141414141LL, 0x4141414141414141LL}; + ray_t* vec = ray_vec_from_raw(RAY_I64, poison, 4); + TEST_ASSERT_NOT_NULL(vec); + + int64_t col_raw[] = {1, 2, 3}; + ray_t* col = ray_vec_from_raw(RAY_I64, col_raw, 3); + TEST_ASSERT_NOT_NULL(col); + + /* add_col consumes one ref of its first argument, so hand it one of ours + * and keep the reference we assert through afterwards. */ + ray_retain(vec); + ray_t* res = ray_table_add_col(vec, ray_sym_intern("c", 1), col); + TEST_ASSERT_NOT_NULL(res); + TEST_ASSERT_TRUE(RAY_IS_ERR(res)); + TEST_ASSERT_STR_EQ(ray_err_code(res), "type"); + + /* And the payload it would have decoded is untouched. */ + TEST_ASSERT_EQ_I(((int64_t*)ray_data(vec))[0], 0x4141414141414141LL); + TEST_ASSERT_EQ_I(((int64_t*)ray_data(vec))[1], 0x4141414141414141LL); + + ray_error_free(res); + ray_release(col); + ray_release(vec); + PASS(); +} + /* A dict shares the table's two-slot layout, so the accessors would happily * read real pointers out of it and answer with a dict's shape. The tag is the * only thing separating the two; assert it is honoured. */ @@ -608,6 +642,25 @@ static test_result_t test_table_accessors_reject_dict(void) { TEST_ASSERT_EQ_I(ray_table_ncols(d), 0); TEST_ASSERT_EQ_I(ray_table_nrows(d), 0); TEST_ASSERT_NULL(ray_table_schema(d)); + TEST_ASSERT_NULL(ray_table_get_col(d, ray_sym_intern("c", 1))); + TEST_ASSERT_NULL(ray_table_get_col_idx(d, 0)); + TEST_ASSERT_EQ_I(ray_table_col_name(d, 0), -1); + + /* Mutators would otherwise write through the dict's real slot pointers. */ + ray_table_set_col_name(d, 0, ray_sym_intern("c", 1)); + ray_table_set_col_idx(d, 0, d); + TEST_ASSERT_EQ_I(ray_dict_len(d), 3); + TEST_ASSERT_EQ_I(((int64_t*)ray_data(ray_dict_keys(d)))[0], 1); + + int64_t col_raw[] = {1, 2, 3}; + ray_t* col = ray_vec_from_raw(RAY_I64, col_raw, 3); + ray_retain(d); + ray_t* res = ray_table_add_col(d, ray_sym_intern("c", 1), col); + TEST_ASSERT_TRUE(RAY_IS_ERR(res)); + TEST_ASSERT_STR_EQ(ray_err_code(res), "type"); + TEST_ASSERT_EQ_I(ray_dict_len(d), 3); + ray_error_free(res); + ray_release(col); ray_release(d); PASS(); @@ -665,6 +718,7 @@ const test_entry_t table_entries[] = { { "table/set_col_name_shared", test_table_set_col_name_shared, table_setup, table_teardown }, { "table/accessors_null_and_err", test_table_accessors_null_and_err, table_setup, table_teardown }, { "table/accessors_wrong_type", test_table_accessors_wrong_type, table_setup, table_teardown }, + { "table/add_col_wrong_type", test_table_add_col_wrong_type, table_setup, table_teardown }, { "table/accessors_reject_dict", test_table_accessors_reject_dict, table_setup, table_teardown }, { "table/parted_nrows_plain_vec", test_parted_nrows_plain_vec, table_setup, table_teardown }, { "table/nrows_empty_col", test_table_nrows_empty_col, table_setup, table_teardown },