ray_table_nrows/ncols segfault on a non-table (public API, no documented precondition)

[vbmithr]

Summary

ray_table_nrows() and ray_table_ncols() segfault when handed a value
that is not a table. They are part of the public embedding API in
rayforce.h, which takes an opaque ray_t* and documents no
precondition, so from an embedder's side these read as total functions.

Related but separate from #565 (which is a __VM NULL guard on a
different path).

Reproduction

Against dev at c27fdc34. The value is an ordinary scalar reply to
(+ 1 2) over IPC — type=-5:

ray_runtime_create(0, NULL);
ray_poll_t* p = ray_poll_create(); ray_runtime_set_poll(p);
int64_t h = ray_ipc_connect("127.0.0.1", port, NULL, NULL, 2000);
ray_t* r = ray_ipc_send(h, ray_str("(+ 1 2)", 7));
printf("type=%d\n", r->type);     /* -5 */
ray_table_nrows(r);               /* SIGSEGV */

Probing each accessor on that same reply, with a SIGSEGV handler:

scalar IPC reply, type=-5:
  table_nrows        -> SIGSEGV
  table_ncols        -> SIGSEGV
  dict_len           -> 0

dict_len returning 0 is not a guard — ray_dict_len is written the
same way (ray_dict_keys(d) then keys->len); this input just happens
to land on a falsy pointer. Which is the awkward part: the same call is
benign on one value and fatal on another. A locally built
ray_i64(42) passed to ray_table_nrows returns 0 here; only the
IPC-deserialized atom crashes. That makes the misuse very easy to ship —
it is how we found it, in a smoke test that had been passing a scalar
reply to ray_table_nrows for a while under a try ... with that of
course cannot catch a segfault.

Why this isn't a complaint about the design

I realise the accessors are written against block layout, not the type
tag, and that this is deliberate: your own call sites check first
(if (x->type == RAY_TABLE) in src/ops/agg.c:546, the ternary in
src/core/qlog.c:38), because the evaluator has already established the
type during builtin dispatch. Keeping ray_table_nrows branch-free
matters on paths like agg_engine.c's parallel-size test. And
ray_table_validate_rectangular() is there for callers who want an
explicit check.

That works well for the evaluator. It works less well for an embedder,
which has no evaluator in front of the API — ray_t* is one type, so
there is nothing to check against except the tag you already store.

Worth noting ray_table_ncols already returns 0 defensively:

int64_t ray_table_ncols(ray_t* tbl) {
    if (!tbl || RAY_IS_ERR(tbl)) return 0;

It rejects NULL and errors but not a wrong tag, which is the case that
actually crashes.

Possible resolutions, in your preference order not mine

1. Make them total. Extend the existing defensive return: add
   tbl->type != RAY_TABLE to the guard ray_table_ncols already has,
   and give ray_table_nrows the same. One predictable, well-branch-
   predicted comparison against a field already in cache. Same for
   ray_dict_len.
2. Document the precondition. If the branch is unacceptable on hot
   paths, saying so in rayforce.h would be enough — I grepped the
   whole header for caller must / precondition / undefined /
   assumes and there is currently nothing, so an embedder has no way
   to learn this short of a crash.
3. Checked variants. ray_table_nrows_checked() or similar, leaving
   the hot path untouched.

Any of these solves it for us; (2) alone would have been enough. We have
guarded it on our side already, so there is nothing urgent here — filing
it because the next embedder will hit the same thing, and because a
public API segfaulting on an in-range value of its own parameter type
seemed worth reporting rather than working around silently.

Happy to send a PR for whichever option you prefer.

[singaraiona]

Confirmed, and going with your option 1 — make them total. I'm taking this one; PR shortly.

Two things the investigation turned up that are worth putting on the record, because one of them changes the argument in your favour.

1. ray_dict_len is not an accident — it's already option 1.

You wrote that dict_len returning 0 "is not a guard ... this input just happens to land on a falsy pointer." It isn't luck. src/table/dict.c:111:

ray_t* ray_dict_keys(ray_t* d) {
    if (!d || RAY_IS_ERR(d) || d->type != RAY_DICT) return NULL;

The tag check is right there, and ray_dict_vals has it too. So the dict half of this layer already does exactly what you're asking for. That reframes the whole issue: the table accessors aren't holding a deliberate line that the dict accessors happen to cross — they're the outlier in their own layer. Option 1 makes them consistent rather than making them slower.

2. It reproduces on a far more ordinary value than a deserialized atom.

You framed this as a quirk of IPC-deserialized values, since a local ray_i64(42) was benign. The real trigger is just payload bytes that dereference badly. A plain I64 vector does it deterministically:

ray_t* v = ray_vec_new(RAY_I64, 4);
int64_t* d = (int64_t*)ray_data(v);
for (int i = 0; i < 4; i++) d[i] = 0x4141414141414141LL;
v->len = 4;
ray_table_ncols(v);   /* SIGSEGV */

calling ray_table_ncols...
Segmentation fault (core dumped)     # exit 139

tbl_slots() is (ray_t**)ray_data(tbl), so those int64s get read back as the schema/cols pointers and dereferenced for ->len. ray_i64(42) survives only because 42 is an unmapped-but-non-crashing read in that build's layout. That makes this materially worse than "don't pass us IPC replies" — any vector reaching either accessor is a live crash.

Worth adding: a SIGSEGV handler with siglongjmp reported these as returning 0 on my first pass, exactly the same way your try ... with couldn't catch it. Your instinct to distrust that was correct; I only got a truthful answer by calling them unguarded.

What I'll ship: the tbl->type != RAY_TABLE check folded into the guard ray_table_ncols already has, and the same guard added to ray_table_nrows. RAY_TABLE (98) is the only table tag in the tree, and all 324 internal call sites establish the type before calling, so nothing internal depends on the unchecked behaviour. No change needed for ray_dict_len per the above. I'll also do (2) and document the guarantee in rayforce.h, since you're right that the header currently says nothing either way — a total function is only useful to an embedder who knows it's total.

Thanks for the report. The "benign on one value, fatal on another" framing is what made this worth fixing rather than documenting.