Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

read after select blocks for pipe to external program when running MSYS program in UCRT64 #202

Closed
purplesyringa opened this issue Feb 19, 2024 · 9 comments

Comments

@purplesyringa
Copy link

purplesyringa commented Feb 19, 2024

I'm rather inexperienced with MSYS2, so I read up on the website that UCRT64 is the recommended default environment and started using UCRT64 only. I needed Python, so I did pacman -S python3, which pulled the MSYS version of the package, which I then used in UCRT64 environment. I'm not sure if running MSYS packages in UCRT64 is even supported, so please do tell me if I was lucky it didn't break earlier.

This worked just fine until the following bug surfaced.

Python has a neat subprocess.Popen.communicate facility for running a subprocess with (possibly long) input and retrieving output without deadlocking on stdio. How it is implemented depends on the OS, but on POSIX it creates anonymous pipes for both stdin and stdout, poll(2)s the two fds, and writes to stdin or reads from stdout depending on which one becomes available. For some reason, my Python code experienced weird hangs in this function which can be reproduced as follows:

  1. Start UCRT64 environment.
  2. Install the python3 package. Make sure that which python3 resolves to /usr/bin/python3.
  3. Compile the following test script via GCC/MSVC/whatever into repr.exe:
#include <windows.h>
#include <stdio.h>
char buffer[4096];
int main() {
    DWORD n_read;
    while (ReadFile(GetStdHandle(STD_INPUT_HANDLE), buffer, sizeof(buffer), &n_read, NULL)) {
        fprintf(stderr, "Read %d\n", n_read);
    }
    return 0;
}
  1. Run python3 -c 'import subprocess; subprocess.run("./repr", input=b"\x00" * 65537, stdout=subprocess.PIPE)'.

Expected behavior: the program reads 16 4096-byte chunks and one 1-byte chunk and then the Python interpreter returns.

Actual behavior: the program reads 16 4096-byte chunks and then hangs.

I don't think this is Python's bug, because Python simply does poll+read. The bug cannot be reproduced when using MSYS or MINGW64 environment, or when using native Python in UCRT64. Not in cygwin either. It is precisely the interaction of POSIX Python with native UCRT64 environment that triggers the problem. I'm sort of at loss here, because it doesn't seem like the underlying libc should affect anything, but here we are.

(I'm not sure if this is the right place to report this bug, because it is the result of interaction of several subsystems, so please tell me if I should go elsewhere.)

@dscho
Copy link
Collaborator

dscho commented Feb 26, 2024

I can reproduce this. Also in Cygwin, so I reported it on IRC.

@dscho
Copy link
Collaborator

dscho commented Feb 26, 2024

I'm not sure if running MSYS packages in UCRT64 is even supported

Forgot to say: @purplesyringa this is completely within what is supported. Thank you for reporting this bug and providing such an easy reproducer.

@tyan0
Copy link
Contributor

tyan0 commented Mar 1, 2024

@dscho, I tried to reproduce this with cygwin 3.5.1, however, could not. Which cygwin version did you try?

@tyan0
Copy link
Contributor

tyan0 commented Mar 1, 2024

I also cannot reproduce with MSYS_NT-10.0-19045 HP-Z230 3.4.10--api-345.x86_64 2024-02-15 22:56 UTC x86_64 Msys and MSYS_NT-10.0-19045 HP-Z230 3.4.10.x86_64 2024-02-10 08:39 UTC x86_64 Msys.

@tyan0
Copy link
Contributor

tyan0 commented Mar 1, 2024

With this test case, repr.exe is a native win32 binary (not cygwin/msys2 one), and python3 is a cygwin/msys2 binary. Right?

@tyan0
Copy link
Contributor

tyan0 commented Mar 1, 2024

Ah, I could reproduce when repr.exe is built with mingw UCRT compiler. If this is compiled with /mingw64/bin/gcc the issue does not occur.

@dscho
Copy link
Collaborator

dscho commented Mar 1, 2024

Ah, I could reproduce when repr.exe is built with mingw UCRT compiler. If this is compiled with /mingw64/bin/gcc the issue does not occur.

That's strange because I did compile with /mingw64/bin/gcc and it hangs reliably over here.

In any case, thank you for looking into this, @tyan0!

@tyan0
Copy link
Contributor

tyan0 commented Mar 3, 2024

I found the cause. Currently, select() call for write-side of a pipe possibly hangs for non-cygwin reader. However, it is difficult to explain why. Please refer to https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/select.cc#L614 for detail.

I will submit a patch shortly for reviewing. Thanks.

dscho pushed a commit to dscho/Cygwin-msys2-fork that referenced this issue Mar 3, 2024
Non-cygwin app may call ReadFile() which makes NtQueryObject() for
ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
Therefore, stop to try to get query_hdl for non-cygwin apps.

Addresses: msys2#202

Backported-from: https://inbox.sourceware.org/cygwin-patches/20240303050915.2024-1-takashi.yano@nifty.ne.jp/
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho pushed a commit to dscho/Cygwin-msys2-fork that referenced this issue Mar 3, 2024
Non-cygwin app may call ReadFile() which makes NtQueryObject() for
ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
Therefore, stop to try to get query_hdl for non-cygwin apps.

Addresses: msys2#202

Backported-from: https://inbox.sourceware.org/cygwin-patches/20240303050915.2024-1-takashi.yano@nifty.ne.jp/
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Mar 3, 2024
Non-cygwin app may call ReadFile() which makes NtQueryObject() for
ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
Therefore, stop to try to get query_hdl for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Backported-from: https://inbox.sourceware.org/cygwin-patches/20240303050915.2024-1-takashi.yano@nifty.ne.jp/
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Mar 5, 2024
Non-cygwin app may call ReadFile() for empty pipe, which makes
NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
get_query_hdl_per_process. Therefore, do not to try to get query_hdl
for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Mar 5, 2024
Non-cygwin app may call ReadFile() for empty pipe, which makes
NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
get_query_hdl_per_process. Therefore, do not to try to get query_hdl
for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Mar 21, 2024
Non-cygwin app may call ReadFile() for empty pipe, which makes
NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
get_query_hdl_per_process. Therefore, do not to try to get query_hdl
for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Backported-from: f6be372 (Cygwin: pipe: Give up to use query_hdl for non-cygwin apps., 2024-03-03)
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho pushed a commit to dscho/Cygwin-msys2-fork that referenced this issue Apr 7, 2024
Non-cygwin app may call ReadFile() which makes NtQueryObject() for
ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
Therefore, stop to try to get query_hdl for non-cygwin apps.

Addresses: msys2#202

Backported-from: https://inbox.sourceware.org/cygwin-patches/20240303050915.2024-1-takashi.yano@nifty.ne.jp/
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Jul 9, 2024
Non-cygwin app may call ReadFile() for empty pipe, which makes
NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
get_query_hdl_per_process. Therefore, do not to try to get query_hdl
for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Backported-from: f6be372 (Cygwin: pipe: Give up to use query_hdl for non-cygwin apps., 2024-03-03)
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Jul 9, 2024
Non-cygwin app may call ReadFile() for empty pipe, which makes
NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
get_query_hdl_per_process. Therefore, do not to try to get query_hdl
for non-cygwin apps.

Addresses: msys2/msys2-runtime#202

Backported-from: f6be372 (Cygwin: pipe: Give up to use query_hdl for non-cygwin apps., 2024-03-03)
Fixes: b531d6b ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Collaborator

dscho commented Sep 3, 2024

@purplesyringa since cygwin/cygwin@f6be372 has been part of v3.5.2, the current msys2-runtime version (which is v3.5.4) should address this. Can you confirm?

@dscho dscho closed this as completed Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants