Re: [PATCH v2] Cygwin: pty: Fix nat_pipe_owner_pid when gdb runs non-cygwin app

Hi Johannes,

On Thu, 9 Jul 2026 10:13:26 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> On Wed, 8 Jul 2026, Takashi Yano wrote:
> 
> > Previously, nat_pipe_owner_pid was incorrectly set to 0 when the
> > inferior of gdb was a non-cygwin app. Due to this bug, repeatedly
> > running a non-cygwin app under gdb could lead to an unexpected crash.
> > 
> > This occurred because the previous code in setup_for_non_cygwin_app()
> > set nat_pipe_owner_pid to exec_dwProcessId, which is correct when the
> > caller is the stub process of the non-cygwin app. However, when the
> > caller is gdb, the owner should be gdb itself, so nat_pipe_owner_pid
> > must be set to myself->dwProcessId.
> > 
> > With this fix, attach_console_temporarily() can be called with target
> > pid equal to the process's own pid, in which case the attach operation
> > is skipped.
> > 
> > Fixes: 1e6c51d74136 ("Cygwin: pty: Reorganize the code path of setting up and closing pcon.")
> > Signed-off-by: Takashi Yano <[email protected]>
> > Reviewed-by:
> > ---
> > v2: Skip attaching operation when attaching to myself is requested.
> 
> From what I can see, the fix is correct. Three non-blocking notes:
> 
> The new source comment (and the commit message) has exec_dwProcessId and
> dwProcessId the wrong way round, I think. exec_dwProcessId holds the
> stub's own pid, saved before the overlay; dwProcessId is what gets
> repointed to the native child. The code still does the right thing because
> `exec_dwProcessId ?: dwProcessId` reduces to "our own pid" on every path
> that reaches it, which is exactly what the owner-self check already
> assumes. But the commentary as written will mislead the next reader.

Ah, right. I revised the comment and the commit message.

> The sibling assignment in `setup_pseudoconsole()` still writes
> exec_dwProcessId directly, without the fallback. Harmless today because
> its sole caller sets the owner first, but leaving the two spots
> inconsistent invites a future regression. Worth applying the same `?:`
> there.

Indeed. I also apply the fix here.

> Also: I believe that the bug is not really gdb-specific: _any_ Cygwin
> process that spawns a non-cygwin app through the CreateProcess hook
> without _P_OVERLAY hits it. If that is so, the subject and log undersell
> the scope.

Noted to the commit message.

> In any case, I happily provide my:
> 
>   Reviewed-by: Johannes Schindelin <[email protected]>

Thanks! I'll push the patch to master and cygwin-3_6-branch.

-- 
Takashi Yano <[email protected]>