GeezerVison

@claude review

Claude finished @carlos-granados's task in 4m 54s —— View job

Claude PR Review

Todo:

• Gather PR diff and understand scope
• Review control socket changes
• Review new/moved library files
• Review debugger protocol changes
• Review maps / cmd_parser changes
• Review tests
• Post final review

Reviewed the 33 commits. Most are an upstream Xdebug sync; the fork-specific delta is the four fix: commits that handle control-socket activation when no debugger is attached. Overall the changes are coherent and well-targeted. Specific findings below.

Fork-specific logic — looks correct

The core fix in 05540e8a (poll the control socket when no debugging client is attached) is right: when on_demand_debugging_enabled and control_socket_active are both false we skip instrumentation (observer disabled, early return); otherwise we still enable ZEND_COMPILE_EXTENDED_STMT so the statement handler fires. xdebug_statement_call in xdebug.c:692 runs xdebug_control_socket_dispatch() before checking statement_handler_enabled, so the control socket is still polled even though the heavier debugger logic is short-circuited. The pause handler in src/base/ctrl_socket.c:263-268 then re-enables both the statement handler and the observer, and xdebug_rebuild_stack() is called when the observer wasn't active — all the right pieces.

Observations and nits

1. Comment accuracy — minor in src/base/ctrl_socket.h:26-27, xdebug.c:617-621, and the CLAUDE.md note:

   The control socket is only ever read from the statement handler

   It's also dispatched from xdebug_execute_user_code_begin() in src/base/base.c:505. The statement handler is the dominant path (it fires per-statement, not per-function), but the one-liner undersells where the call lives. Low priority.

2. Behaviour divergence from Xdebug — c29beb4 changes xdebug.control_socket default from default to no. This is a deliberate trade-off (performance over drop-in parity) and is documented in CLAUDE.md, but CLAUDE.md:7 still asserts "drop-in compatible with Xdebug INI/functions/triggers". Worth a sentence there calling out that INI defaults diverge in this one case so the two statements agree.

3. socket_is_localhost and disable_tcp_syn_transmissions_for_localhost are not static (src/debugger/com.c:143, src/debugger/com.c:175). They're only used within com.c; marking them static matches the rest of the file and avoids leaking symbols. Fix this →

4. Macro missing parentheses in src/lib/maps/maps_private.h:32:

   #define XDEBUG_PATH_MAP_EOF            1 << 30

   Current call sites only use == and assignment so this works today, but any future use in an arithmetic context (x + XDEBUG_PATH_MAP_EOF) silently misbehaves due to << precedence. Trivial fix: #define XDEBUG_PATH_MAP_EOF (1 << 30). Upstream carry-over.

5. Pre-existing bug in named_pipe_listen() on Windows (src/base/ctrl_socket.c:335-348) — not introduced here, but surfaced while reviewing:

   case ERROR_PIPE_CONNECTED:
       if (!SetEvent(...)) { ... return false; }
       /* fallthrough — missing break */
   default:
       xdebug_log_ex(...);
       return false;

   On the happy path (ERROR_PIPE_CONNECTED + successful SetEvent) control falls through to default: and the function returns false with an error log, so an actually-connected named pipe is torn down. This matches origin/main; not a regression from this PR, but it does negate the pause path on Windows under the "client already connected" case.

6. Possible mountinfo truncation in src/lib/system_utils.c:95-125 (introduced by the refactor from usefulstuff.c, so upstream behaviour is preserved): a fixed 8 KiB stack buffer with fread(buffer, 1, sizeof(buffer), ...) and no loop. On systems with many mounts (containers, snaps, long systemd units) mountinfo routinely exceeds 8 KiB; truncation can happen mid-line and xdebug_scan_mountinfo_for_private_tmp will then see a half-line without the matching /tmp token and either report nothing or report the wrong directory. Worth tracking upstream.

7. DBGp multiple_sessions handler (c8c6c38, src/debugger/handler_dbgp.c:1515-1519): the feature now reports supported="1" in feature_get but the handler was removed from feature_set. If a client calls feature_set -n multiple_sessions -v 1 it will fall through to the "unsupported feature" branch, which contradicts the supported="1" we just advertised. Upstream fix; may want a tracking note.

Tests

New tests look sensible — bug02267 cases cover both Windows and Unix for the TCP SYN retransmission fix, bug02421/02422/02424/02431/02435 give good coverage of the hardening work, and the ctrlSocketClient.php harness is a nice addition. The .phpt for bug02439 is self-contained and clear.

Summary

The fork-specific control-socket activation fix is correct and well-reasoned. The remaining items are a mix of inherited upstream oddities worth tracking and a couple of low-risk cleanups (static qualifiers, macro parens, CLAUDE.md consistency on the control_socket default). No blockers from my side.
• Branch: update-to-latest-xdebug

@claude which of the issues you raised were introduced by us and which come from upstream?

Claude finished @carlos-granados's task in 1m 32s —— View job

Classification of previous review findings

Split by source — fork-introduced in this PR vs. carried over from the upstream Xdebug sync (and one that predates both).

Fork-introduced (worth addressing on this PR if you want them cleaned up before merge): #1, #2. These both came out of the control-socket activation work and are small doc / comment tightening.

Upstream carry-overs (worth tracking back to Xdebug rather than fixing here, since diverging from upstream increases future-sync friction): #3, #4, #7, and the file move in #6.

Pre-existing, unrelated to this PR: #5 and the underlying mountinfo-truncation behaviour in #6.

• Check com.c static qualifiers — upstream a612ba2
• Check maps_private.h macro — upstream 0c56972
• Check system_utils.c mountinfo — upstream refactor cfad3f3, behaviour pre-existing
• Check multiple_sessions handler — upstream c8c6c38
• Check comment accuracy claim — fork 05540e8
• Check named_pipe_listen fallthrough — pre-existing on origin/main
• Check CLAUDE.md drop-in parity line — fork c29beb4
  • Branch: update-to-latest-xdebug

@claude do a new review

Claude finished @carlos-granados's task in 5m 37s —— View job

Claude PR Review — fresh pass

Todo:

• Pull the current diff against origin/main
• Re-check fork-specific control-socket logic
• Re-check library refactors (arg, set, stripcslashes, system_utils, cmd_parser, maps)
• Re-check DBGp handler changes
• Re-check new tests
• Post the review

Re-reviewed the PR at its current state (33 commits, latest 62c760f). The CLAUDE.md and README.md updates landed cleanly, the control-socket activation logic reads well, and the fork-specific cleanup commits are each tightly scoped. One new, significant finding this pass — a latent ownership bug that the fork's memory-leak fix in handle_command is now actively exposing.

New finding — exposed by this PR

src/base/ctrl_socket.c:216 — ps handler frees an interior zend_string pointer

Commit 442fcaa correctly adds xdebug_xml_node_dtor(retval) / xdebug_str_free(message) to handle_command() — the retval tree and message buffer were being leaked on every ctrl-socket command on origin/main. Good fix.

However, that destructor now reaches a latent bug in the ps handler:

file = xdebug_xml_node_init("fileuri");
if (fse && fse->filename) {
    xdebug_xml_add_text(file, ZSTR_VAL(fse->filename));  // free_text=1
}

xdebug_xml_add_text() defaults to free_text=1 (xml.c:171), so xdebug_xml_text_node_dtor will call xdfree(node->text) → free(ZSTR_VAL(fse->filename)) at destruction time. ZSTR_VAL(zstr) is an interior pointer into a zend_string allocation (the inline val[1] flexible-array), not a standalone malloc'd pointer — free() on it is undefined behavior and will typically crash with free(): invalid pointer.

Why it doesn't crash in the new tests: the bug02424-* tests invoke ps with start_with_request=no and no debug client, so the observer is deactivated on first call (base.c:488-493) and XG_BASE(stack) is empty — the new if (fse && fse->filename) guard skips the problematic call entirely.

When it will crash: any scenario where the stack has user-defined frames when a ps command arrives — e.g. a user has xdebug.mode=debug + a connected IDE + control_socket enabled, triggers pause via ctrl-socket (which populates the stack via xdebug_rebuild_stack()), then issues a subsequent ps. Fse is populated, ZSTR_VAL gets stored for later free, destructor crashes.

Compare the sibling engine node two lines earlier, which does it correctly:

xdebug_xml_add_text(engine, xdstrdup(XDEBUG_NAME));

Fix: wrap in xdstrdup, same as engine:

xdebug_xml_add_text(file, xdstrdup(ZSTR_VAL(fse->filename)));

Was previously masked by the retval leak. Worth adding a regression test that exercises ps with a populated stack (connected debugger + pause → ps) so this doesn't regress silently again.

Fix this →

Resolved since the previous review

Carrying over, upstream — same classification as last time

• Bump actions/upload-artifact from 4 to 7 #3 socket_is_localhost / disable_tcp_syn_transmissions_for_localhost not static in com.c:143,192 (upstream a612ba2)
• Phase 2: RINIT fast-path & deferred hooks — 630% → 10% overhead #4 XDEBUG_PATH_MAP_EOF missing parens in maps_private.h:32 (upstream 0c56972)
• Phase 3: Runtime Inspection API (JSON transport + stubs) #5 named_pipe_listen() Windows ERROR_PIPE_CONNECTED fallthrough (pre-existing on origin/main)
• Phase 2.5: Deferred activation — connect before enabling EXT_STMT #6 system_utils.c fixed 8 KiB mountinfo buffer (upstream, pre-existing — moved out of base.c by cfad3f3)
• Phase 2.7.1: Rename extension to PHP Debugger with full Xdebug compat #7 DBGp multiple_sessions advertised supported but rejected in feature_set (upstream c8c6c38); the new tests/debugger/bug02435.phpt enshrines this contradictory behavior as expected output — if upstream later fixes it, the test will need updating

Additional small observations noticed this pass

• src/lib/set.c:21 — MEM_SIZE macro missing parentheses:

  #define MEM_SIZE(s) ((s) / 8) + (((s) % 8) != 0)

  Current call sites (direct assignment and memset size argument) all parse correctly, so no bug today. Any future use in an arithmetic context (buf + MEM_SIZE(x)) would silently misbehave. Upstream carry-over.

• src/debugger/handler_dbgp.c:2448 — DBGP_MAX_PACKET_SIZE = 64 KiB cap (upstream a9573c5, issue #2422). This is specifically for inbound DBGp commands from the IDE, not responses, so 64 KiB is comfortably more than any legitimate command. Just flagging the number for future reference in case anyone wonders. Looks right.

• src/base/ctrl_socket.c:263-268 — pause handler re-enabling instrumentation:

  if (!XG_BASE(observer_active)) {
      xdebug_rebuild_stack();
  }
  XG_BASE(statement_handler_enabled) = true;
  XG_BASE(observer_active) = true;

  Correct. But note the asymmetry with base.c:488-493, which forces observer_active = 0 on the first user-code call when there's no debug connection — regardless of whether the control socket is active. That means if a user has control_socket=time + start_with_request=no + no IDE, the observer will be shut down at the first call, and only pause re-enables it. That's probably the intended behavior (we want zero-overhead until the user explicitly asks for a pause), but it also means ps will never see populated stack frames in that configuration — which, incidentally, is what hides the finding above in the current tests.

Tests

Reviewed tests/debugger/bug02267-{win,unix}.phpt, bug02421.phpt, bug02422.phpt, bug02424-01..04.phpt, bug02431.phpt, bug02435.phpt, bug02439.phpt, dbgp/ctrlSocketClient.php. New tests are self-contained and read clearly. The ctrlSocketClient.php harness is a reasonable mirror of the existing dbgpclient.php style. No complaints.

Summary

• One real finding this pass: the ps handler's ZSTR_VAL ownership bug, newly exposed by the (correct) memory-leak fix. Would recommend addressing before merge, plus a regression test with a populated stack.
• Previous finding Bump actions/checkout from 4 to 6 #1 and Bump actions/download-artifact from 4 to 8 #2 both resolved cleanly.
• Everything else is upstream carry-over worth tracking back to Xdebug rather than fixing on this branch.
  • Branch: update-to-latest-xdebug

@claude do a new review

Claude finished @carlos-granados's task in 3m 49s —— View job

Claude PR Review — fresh pass

Todo:

• Pull current diff against origin/main
• Check if the ps handler ZSTR_VAL ownership bug from previous review is fixed
• Review any new commits since last review
• Re-check control-socket activation logic
• Re-check DBGp protocol changes
• Re-check new tests
• Post final review

Re-reviewed the PR at its current state (34 commits, HEAD b3b2319). One new commit since the previous review — b3b2319 fix: report the current file in 'ps' when the observer is not collecting frames — which directly addresses the previous pass's single finding. No new findings this pass.

Previous finding resolved

src/base/ctrl_socket.c — ps handler ZSTR_VAL ownership bug ✅ Fixed in b3b2319

The fix at src/base/ctrl_socket.c:227 applies exactly the recommended change:

if (fse && fse->filename) {
    /* xdstrdup, because the node owns its text and frees it: ZSTR_VAL is an
     * interior pointer into the zend_string, not its allocation. */
    xdebug_xml_add_text(file, xdstrdup(ZSTR_VAL(fse->filename)));
}

The commit also goes further than the minimum fix: it rebuilds the stack on demand when the observer isn't collecting frames (src/base/ctrl_socket.c:200-213), so ps can now report a fileuri even in the common "no client attached, control_socket=time" configuration — previously that path reported an empty <fileuri></fileuri>. The on-demand rebuild then empties the stack again at ctrl_socket.c:251-253, which is correct: nothing else is pushing or popping frames when the observer is inactive, so a stack left behind would go stale.

I checked the ownership round-trip:

• xdebug_rebuild_stack() (base.c:439) starts with xdebug_vector_empty(), then calls add_stack_frame_recursively(), which pushes function_stack_entry values with addref'd zend_string filenames.
• function_stack_entry_dtor (base.c:127-129) releases each fse->filename via zend_string_release.
• The post-ps xdebug_vector_empty() pops every entry, firing that dtor, so the addrefs are balanced.

Two regression tests cover both branches of the new logic:

• tests/debugger/ctrl-socket-ps-fileuri.phpt — stack empty (start_with_request=no, no on_demand_debugging_enabled), asserts the fileuri IS reported after on-demand rebuild.
• tests/debugger/ctrl-socket-ps-stack.phpt — observer collecting frames (on_demand_debugging_enabled=1), asserts ps doesn't crash after the response tree is freed by the 442fcaa leak fix.

The ctrlSocketClient.php harness is extended with a $blankFileUri option so the first test can assert against the real path. Clean, targeted change.

A small style nit, not blocking: the Unix ps semantics imply the "process program" (the entry-point file). XDEBUG_VECTOR_HEAD returns index 0, and add_stack_frame_recursively recurses on prev_execute_data before pushing the current frame, so HEAD is the oldest (bottom) frame — the entry script. That matches the intent and preserves the pre-existing behavior of the handler, but it is not obvious from the code; a one-line comment on fse = XDEBUG_VECTOR_HEAD(...) saying "entry-point frame, Unix-ps-style" would spare the next reader my paragraph of digging.

Still outstanding — upstream carry-overs (not fork-introduced, not blocking)

Same classification as the previous pass:

• Bump actions/upload-artifact from 4 to 7 #3 socket_is_localhost / disable_tcp_syn_transmissions_for_localhost not static in src/debugger/com.c:143,192 (upstream a612ba2, upstream #2267)
• Phase 2: RINIT fast-path & deferred hooks — 630% → 10% overhead #4 XDEBUG_PATH_MAP_EOF missing parens in src/lib/maps/maps_private.h:32 (upstream 0c56972, upstream #2398)
• Phase 3: Runtime Inspection API (JSON transport + stubs) #5 named_pipe_listen() Windows ERROR_PIPE_CONNECTED fallthrough in src/base/ctrl_socket.c:356-366 — pre-existing on origin/main
• Phase 2.5: Deferred activation — connect before enabling EXT_STMT #6 system_utils.c fixed 8 KiB fread with no loop in src/lib/system_utils.c:95-125 — upstream, pre-existing (moved out of usefulstuff.c by cfad3f3)
• Phase 2.7.1: Rename extension to PHP Debugger with full Xdebug compat #7 DBGp multiple_sessions advertised supported in feature_get but rejected in feature_set — upstream c8c6c38; tests/debugger/bug02435.phpt enshrines the contradictory behavior as expected output, so a future upstream fix would need the test updated
• Add security CI workflows #8 MEM_SIZE macro missing parens in src/lib/set.c:21 — upstream

Worth tracking back to Xdebug rather than fixing here (diverging increases future-sync cost). None are regressions from this PR.

Fork-specific control-socket logic — still looks correct

Re-walked the full flow:

• xdebug.c:613-648 — if no client connected AND neither on_demand_debugging_enabled nor control_socket_active is set, bail out before enabling ZEND_COMPILE_EXTENDED_STMT. The two log messages (OPCACHE-OD / OPCACHE-CS) correctly distinguish the two reasons the observer stays armed.
• src/base/ctrl_socket.c:263-290 — pause re-enables the statement handler and observer, and rebuilds the stack if the observer wasn't active. The pattern is intentionally different from ps: pause means "we're attaching, keep the stack around"; ps means "snapshot and move on."
• xdebug_statement_call in xdebug.c:692 still dispatches the control socket before the statement_handler_enabled short-circuit, so polling continues even when the heavier debugger logic is skipped.

Comment / docstring consistency

Both src/base/ctrl_socket.h:25-28 and xdebug.c:617-623 now list both the statement handler AND the observer's xdebug_execute_user_code_begin() as polling sites. CLAUDE.md:7 acknowledges the one deliberate INI-default divergence (xdebug.control_socket=no) with a cross-reference to the detailed note at CLAUDE.md:108. Both of the previous pass's doc findings read cleanly now.

Summary

• Previous finding fixed cleanly, plus a UX improvement (fileuri now reported in the no-client case) and two tests covering both branches.
• No new findings this pass.
• Everything else is upstream carry-over worth tracking back to Xdebug.

LGTM from my side.
• Branch: update-to-latest-xdebug