Skip to content

PR #722 follow-up: P2 polish items from /gate fix-loop (9 items) #723

Description

@jsavin

Rationale

PR #722 (4 fix-loop rounds, all converged) landed the lazy breakpoint-driven debug attach for callScript-spawned threads. Across the rounds, /gate reviewers raised the following P2 items that were deferred rather than blocking the merge. Tracking them as a single follow-up issue since they're all small polish/hardening items in the same subsystem (headless_spawn.c / debug_handler.c / tests/debug_protocol_test.sh).

P2 items to address

Code quality / hardening

  • TLS overflow counter asymmetry (bar-raiser round 1; concurrency round 1 B3). tls_script_stack[64] in debug_handler.c silently drops the old value on overflow with no counter, asymmetric with the well-designed tydebugstate->script_stack_overflow. Pop will mis-restore after 64 nested frames. Add static __thread short tls_script_overflow; and balance push/pop the same way the debug-state stack does.
  • TLS update on resolution failure (security round 1). When langexternalgetfullpath fails, new_script[0] stays NUL and push unconditionally overwrites tls_current_script with empty, losing the caller's path. An attacker who can trigger resolution failure during a nested call could suppress a security-relevant breakpoint set by the operator. Fix: only update TLS when resolution succeeds.
  • Bail-after-grace UAF on wedged threads (concurrency round 2; documented residual). When debug_wait_lazy_threads_drained bails after 500ms grace because a thread is wedged in a non-yielding C call, transport is freed; later thread wake-up's call to debug_send_completed(state->transport, ...) is a use-after-free on a freed function-pointer vtable. Process is exiting so impact is bounded (typically segfault at exit), but worth documenting more precisely OR sending a force-flkill and synchronous join earlier.
  • start_suspended is dead-but-future-proofing (bar-raiser round 1 P2-2). handle_debug_run always passes true, headless_thread_callscript/evaluate pass NULL. Document the future use case in the header so reviewers don't ask why the option exists.
  • Pascal-string const-drop in headless_register_thread wrapper (bar-raiser round 1 P2-3). frontier-cli/headless_spawn.c does copystring(run_spec->bsverb, bsverb_copy); to drop const. Consider changing headless_register_thread to take a const bigstring — simpler than copying.
  • Drop-notification during teardown (bar-raiser round 2 P2-A). Current order is teardown_protocol_output() -> debug_wait_lazy_threads_drained() -> ... but any lazy thread completing during the drain has its debug_send_completed silently no-op'd because stdio_write_line returns early when g_protocol_out is NULL. Benign in practice (client is gone) but the in-code comment overstates the safety. Either swap teardown order to drain -> clear -> free(transport) -> teardown_protocol_output(), or document the dropped-notification behavior.

Test rigor

  • Test 21b hot-path-cost is a smoke test, not a regression guard (bar-raiser round 1 P2-5). Current 5-second budget on a 100-iteration loop could mask a 40x slowdown in the breakpoint callback. Bump to 10K-100K iterations; tighten budget.
  • Test 21 doesn't assert lazy thread unregistration (bar-raiser round 1 P2-6). After continue + completed, nothing asserts debug/listThreads shows the lazy thread gone. The leak-detection claim in the PR description deserves a behavioral check.
  • Test 22 covers only happy path (security round 2). The dangerous UAF window — session closes WHILE thread is suspended — is partially covered by Test 22b but Test 22 itself only tests "thread completes before session closes." Could be strengthened with a multi-thread variant.

Coverage gaps

  • OOM-path test coverage (security round 3). The NULL-deref fix in commit 9955824b4 cannot be regression-tested without LD_PRELOAD malloc-failure injection. If/when the project adopts such test infrastructure, add OOM-path coverage of headless_spawn_script_thread.

Context

Exit Criteria

Each bullet above can be addressed in its own PR (most are <30-line changes). This is a tracking bucket — close individual items as PRs land.

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority/p2Medium priority - nice to havescope/medium1-2 weeks, requires design workworkstream/verb-portingKernel verb implementation and bindings

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions