fix(env): guard env_lookup_flat against a thread with no VM

[vbmithr]

What & why

Summary

ray_env_get() segfaults when called from any thread that did not call
ray_runtime_create(). env_lookup_flat() dereferences the thread-local
__VM unconditionally, while every other function on the same path —
including its immediate neighbour doing the identical walk — already
guards it.

This is reachable from the public IPC API: closing a connection fires the
.ipc.on.close lifecycle lookup, which calls ray_env_get().

Reproduction

Reproduced against the packaged build 2.6.2.r147.g1388b334; the
unguarded dereference is still present on dev at c27fdc34. No
embedder, ~25 lines:

#include <rayforce.h>
#include <pthread.h>
#include <stdio.h>

static int64_t hook_sym;

static void* worker(void* _) {
    (void)_;
    printf("worker: ray_env_get on a VM-less thread...\n"); fflush(stdout);
    ray_t* v = ray_env_get(hook_sym);
    printf("worker: returned %p (no crash)\n", (void*)v); fflush(stdout);
    return NULL;
}

int main(void) {
    ray_runtime_t* rt = ray_runtime_create(0, NULL);
    if (!rt) { fprintf(stderr, "runtime_create failed\n"); return 2; }
    hook_sym = ray_sym_intern(".ipc.on.close", 13);
    printf("main: ray_env_get -> %p\n", (void*)ray_env_get(hook_sym));
    fflush(stdout);
    pthread_t t;
    pthread_create(&t, NULL, worker, NULL);
    pthread_join(t, NULL);
    return 0;
}

$ gcc -g vmprobe.c -o vmprobe -lrayforce -lpthread -lm && ./vmprobe
main: ray_env_get -> (nil)
worker: ray_env_get on a VM-less thread...
Segmentation fault (core dumped)     # exit 139

Cause

__VM is _Thread_local (src/core/runtime.h:127) and is assigned in
exactly one place in the tree — src/core/runtime.c:261, inside
ray_runtime_create. No public API binds a VM to any other thread, so
every other thread has __VM == NULL by construction.

src/lang/env.c:296:

static ray_t* env_lookup_flat(int64_t sym_id) {
    for (int32_t d = __VM->scope_depth - 1; d >= 0; d--) {   /* NULL deref */

Why this looks like an oversight rather than an unsupported use

Every other function on this path is already written for the VM-less
case:

Location	Guard
env.c:309 ray_env_get_local	if (!__VM) return NULL; — identical scope walk, guarded
ipc.c:264 ipc_ctx_handle	return __VM ? __VM->ipc_handle : -1;
ipc.c:268 ipc_ctx_poll	return __VM ? (ray_poll_t*)__VM->ipc_poll : NULL;
ipc.c:272 ipc_ctx_set	if (!__VM) return;
runtime.c:55 ray_last_err_msg	thread-local shadow, added for this exact case
env.c:296 env_lookup_flat	none

runtime.c:50-54 states the intent directly:

__VM->err.msg is the canonical per-thread slot now that the VM is
stable for the thread's lifetime, but FFI callers can hit errors on
threads that never bound a VM — this thread-local shadow keeps
ray_error_msg() answerable there too.

The IPC Client API block in rayforce.h also documents no threading
restriction, while the header is explicit wherever one exists
("main-thread only" for the progress/span APIs, "call from the thread
that owns poll" for listener servicing, "one writer handle per directory"
for AOF).

The teardown path is the practical trigger:

ray_ipc_close -> ray_poll_deregister -> ipc_on_close
              -> hook_call_lifecycle(IPC_HOOK_CLOSE)
              -> hook_lookup -> ray_env_get -> env_lookup_flat   /* boom */

hook_call_lifecycle's very next statement is if (!fn) return;, so with
no hook installed this is meant to be a no-op — the unguarded deref is the
only thing preventing that.

Fix

Skip the local scopes when there is no VM; globals must still resolve.
Matches ray_env_get_local's existing style.

 static ray_t* env_lookup_flat(int64_t sym_id) {
-    for (int32_t d = __VM->scope_depth - 1; d >= 0; d--) {
-        ray_scope_frame_t* f = &__VM->scope_stack[d];
-        for (int32_t i = 0; i < f->count; i++) {
-            if (f->keys[i] == sym_id) return f->vals[i];
+    /* FFI/embedding callers reach this on threads that never bound a VM
+     * (cf. ray_env_get_local, already guarded). Such a thread has no
+     * local scopes; global bindings must still resolve. */
+    if (__VM) {
+        for (int32_t d = __VM->scope_depth - 1; d >= 0; d--) {
+            ray_scope_frame_t* f = &__VM->scope_stack[d];
+            for (int32_t i = 0; i < f->count; i++) {
+                if (f->keys[i] == sym_id) return f->vals[i];
+            }
         }
     }
     for (int32_t i = 0; i < g_env.count; i++) {

This also covers the dotted walk, whose head segment resolves through the
same function (env.c:340, :371).

Known limitation, stated deliberately

This makes the no hook installed case safe, which is what the IPC
teardown path needs. If a hook is globally bound and the close happens
on a VM-less thread, hook_lookup will now find the lambda and
call_fn1 will run without a VM. That is a separate question — whether
lifecycle hooks should run at all on such a thread, or whether dispatch
should bind a VM first — and I have not addressed it here rather than
guess at the intended semantics. Happy to follow up if you have a
preference.

Testing

Applied to a clean checkout of dev at c27fdc34:

• make lib -j8 builds clean.
• vmprobe linked against it goes from exit 139 (SIGSEGV) to exit 0:
  worker: returned (nil) (no crash).
• No behavioural change on a thread that has a VM — the loop body is
  byte-identical, only wrapped in if (__VM).

I could not run make test to completion: it fails to compile
test/test_link.o on glibc 2.44 (Arch), where test/test_link.c:34
redefines _POSIX_C_SOURCE as 200809L over the 202405L that
features.h already set, and -Werror rejects it. That is a
pre-existing issue in a file this change does not touch, and it
reproduces independently of the patch — but it does mean I have not
verified the suite, and you may want to run it yourself before merging.

Checklist

• PR targets dev (not master)
• Commits follow Conventional Commits
• make builds cleanly (no new warnings) — make lib verified at c27fdc34
• make test passes — could not run locally, see Testing above: the
  suite fails to compile test/test_link.o on glibc 2.44 for a
  pre-existing reason unrelated to this change (verified on a pristine
  unpatched checkout). No test added: this is a NULL guard on a path with
  no existing thread-related coverage, and the repro above needs a second
  thread. Happy to add one if you'd like it wired into the suite.

[singaraiona]

Verified end to end — reproduced, patched, and ran the suite. This is correct and I'm merging it.

Reproduction, your vmprobe.c verbatim against a build of this branch and a build of its parent:

build	exit
559b37cf~1	139 (SIGSEGV)
559b37cf	0 — worker: returned (nil) (no crash)

Suite: make lib clean, make test → === 3887 of 3887 passed (0 skipped, 0 failed) ===. So the concern in your checklist is settled; you weren't able to run it, and it's green.

Your reading of the intent is right. ray_env_get_local at env.c:309 does the byte-identical scope walk with if (!__VM) return NULL; immediately below the unguarded copy at :296 — that asymmetry is an oversight, not a policy. And ray_env_get is public (rayforce.h:677) with a documented synchronization caveat but no threading precondition, so a VM-less caller is within the contract as written.

The known limitation you flagged is the right call, and thank you for stating it rather than guessing. With a hook globally bound, call_fn1 would now run on a thread with no VM — whether lifecycle hooks should run there at all, or whether dispatch should bind a VM first, is a real design question and not one to settle inside a NULL guard. I'll open a separate issue for it.

Two follow-ups, both on us:

1. Your make test blocker is a real bug and a one-liner. test/test_link.c:34 has #define _POSIX_C_SOURCE 200809L sitting after rayforce.h, test.h and the rest have already pulled in features.h — so it has no effect where it is, and on glibc 2.44 it's a redefinition that -Werror rejects. Deleting the line is the whole fix. Happy to take it, or it's yours if you'd rather unblock your own checkout.

2. A regression test would be welcome but isn't a merge condition. test/test_heap_parallel.c is the only existing pthread_create user in the suite, so there's a pattern to copy if you want to wire the probe in. Not blocking this.