diff --git a/include/rayforce.h b/include/rayforce.h index 2ab29271..7b23814f 100644 --- a/include/rayforce.h +++ b/include/rayforce.h @@ -677,7 +677,28 @@ 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 ===== + * + * 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_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); 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..10d39b6e 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]; } @@ -118,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", @@ -189,7 +212,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 +229,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 +244,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 +263,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 +276,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 +292,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 +336,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/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 97530cbe..55d8415e 100644 --- a/test/test_table.c +++ b/test/test_table.c @@ -564,6 +564,108 @@ 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(); +} + +/* 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. */ +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)); + 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(); +} + /* 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 +717,9 @@ 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/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 }, { NULL, NULL, NULL, NULL },