Re: [PATCH] winsup: console: validate duplicated handle set for master thread

Hi Chris,

On Tue, 11 Aug 2026, 飛飛飛 wrote:

> Hi Cygwin maintainers,
> 
> I originally came across this issue while working on LibreOffice development
> on Windows.
> 
> My main development environment is:
> 
>   CPU: AMD Ryzen 7 9800X3D, 8 cores / 16 hardware threads
>   OS: Windows
>   Visual Studio: Visual Studio 2022
>   Windows SDK / UCRT: 10.0.26100.0
>   Cygwin runtime: E:\LibreOfficeDev\cygwin64
>   LibreOffice source tree: E:\LibreOfficeDev\src
> 
> While developing LibreOffice, I found that parallel builds could become
> unreliable in my environment, and I often had to reduce the parallelism
> significantly to avoid a stuck build.
> 
> Out of curiosity, I started reading the Cygwin source to understand what was
> happening under parallel workloads. I used ChatGPT as an assistant while
> inspecting logs, dumps, source code and synchronization paths, but I
> reproduced,
> built and validated the changes locally myself.
> 
> During the investigation, I reproduced a console-related failure path:
> 
>   close_handle: CloseHandle(p->input_handle<0x0>) failed, Win32 error 6

That's ERROR_INVALID_HANDLE, which in and of itself is curious.

> Reviewing the console master-thread handle setup showed that
> get_duplicated_handle_set() assumed that all DuplicateHandle() calls
> succeeded, while cleanup could later attempt to close members of a partially
> initialized handle set.
> 
> The attached patch makes handle acquisition all-or-nothing.
> 
> It:
> 
>   * initializes handle_set_t before duplication;
>   * changes get_duplicated_handle_set() to return bool;
>   * checks every DuplicateHandle() result;
>   * cleans up already duplicated handles if a later duplication fails;
>   * avoids starting the console master thread with an incomplete handle set;
>   * releases duplicated console handles if thread_sync_event duplication
> fails;
>   * makes close_handle_set() tolerate partially initialized handle sets.

While the premise and goal of it is good, I'm concerned about the shape of
the patch.

It does not defend why it introduces initialization that seems not to have
been necessary before (was it implicit? was initialization forgotten and
is the initialization fixing a real, unrelated bug?). Mixing unrelated
changes in a single patch makes it unnecessarily hard to review, and
therefore unnecessarily easy for bugs to sneak in.

Also, this patch introduces a _lot_ of repetitive code. That's not only
less than pleasing on the eye, it also opens the door for subtle bugs to
creep in undetected (or for unnecessarily repetitive fixes to be needed in
the future). Can you please try to make this code less repetitive? An
inline helper, or a closure, or even a preprocessor macro, should help
dramatically.

Ciao,
Johannes

> 
> The patch does not intentionally change console synchronization semantics.
> 
> I tested this against the current official newlib-cygwin source.
> 
> Upstream base:
> 
>   81f45549e771fd4da7b88ad04bd9e6ec5c765d50
> 
> Patch commit:
> 
>   1d5fbfa8e
>   winsup: console: validate duplicated handle set for master thread
> 
> The patch was also verified in both directions:
> 
>   reverse apply against the patched HEAD: PASS
>   forward apply against the upstream parent: PASS
> 
> The resulting current-master Cygwin DLL was built and tested locally.
> 
> Runtime smoke tests:
> 
>   true.exe   PASS
>   uname.exe  PASS
> 
> I then installed the resulting DLL into the Cygwin runtime used by my actual
> LibreOffice development environment and tested real parallel LibreOffice
> builds:
> 
>   make -j4   PASS, exit status 0, elapsed time 25 seconds
>   make -j8   PASS, exit status 0
> 
> The -j8 run exercised the normal LibreOffice gbuild process tree, including
> make, sh, touch, cp and other Cygwin utilities.
> 
> I am submitting this because I believe that when an open-source failure path
> can be reproduced and hardened without changing intended behavior, it is
> worth
> trying to fix it rather than only working around it.
> 
> I am not a Cygwin maintainer, so I would be grateful for review and any
> corrections needed to better match Cygwin conventions.
> 
> Thank you for maintaining Cygwin.
> 
> Regards,
> Chris Chen
> [email protected]
>