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

Address underlying cause of hanging 'git push' for git://-protocol servers without sendpack.sideband=false #2376

Closed
1 task done
assarbad opened this issue Oct 27, 2019 · 11 comments

Comments

@assarbad
Copy link

assarbad commented Oct 27, 2019

  • I was not able to find an open or closed issue matching what I'm seeing

Sorry for "not following protocol" with this ticket, but it's a mix of stuff collected from at least three different tickets, notably: #907, #2278 and #2375.

I'd like to use this to track my own findings and progress regarding the underlying issue, which I think is - in my opinion - caused by attempting to use NtQueryObject to query the name of the pipes acting as standard handles. Pipes are beasts that are different from Unix pipes on Windows. Most importantly this particular operation (trying to query object information) is known to be blocking under certain circumstances. Especially when the pipes are in a blocking state (default as far as I'm aware).

Copying from my comment in #907 ...:

Anyway, regarding the actual topic at hand (blocking inside git pack-objects), I think you would be well advised to talk to some of the folks at MS who are much better equipped to provide an answer than I am. As I understand you work (or worked?) for MS to drive this project forward? Anyway, looking at detect_msys_tty I can take a guess of what's going on here. The namespaces for both mailslots and (named) pipes are prone to blocking as I have learned many years ago. The only safe method I ever found to work with these was in a separate thread as I frequently ran into issues where operations such as the NtQueryObject call in detect_msys_tty blocked. For ntobjx I didn't go down that route yet, but a predecessor tool had also support for listing mailslots and pipes. If you rely on NtQueryObject for such a use-case, I think that's questionable (but then, so is the overall enterprise of pretending Windows is unixy enough for all of this ;)). Although by far not as knowledgeable as one Mark Russinovich of Sysinternals/Microsoft fame, I think that I know a thing or two about the NT native API.

For the record the two stack traces for the "client" and "server" processes respectively in the case of blocking:

Parent git process (git push):

0x0000000000000000
ntdll.dll!NtWaitForSingleObject+0x14
KERNELBASE.dll!WaitForSingleObjectEx+0x93
git.exe!waitpid+0x57
git.exe!finish_command+0x59
git.exe!send_pack+0xda5
git.exe!git_transport_push+0x139
git.exe!transport_push+0x53a
git.exe!push_with_options+0xe1
git.exe!cmd_push+0xaea
git.exe!handle_builtin+0x248
git.exe!cmd_main+0x1ab
git.exe!main+0x78
git.exe!wmain+0x33a
git.exe!__tmainCRTStartup+0x249
git.exe!mainCRTStartup+0x1b
KERNEL32.DLL!BaseThreadInitThunk+0x14
ntdll.dll!RtlUserThreadStart+0x21

Child git process (git pack-objects --all-progress-implied --revs --stdout --thin --delta-base-offset --progress):

0x0000000000000000
ntdll.dll!NtQueryObject+0x14
git.exe!detect_msys_tty+0x82
git.exe!winansi_init+0x1d1
git.exe!wmain+0x31f
git.exe!__tmainCRTStartup+0x249
git.exe!mainCRTStartup+0x1b
KERNEL32.DLL!BaseThreadInitThunk+0x14
ntdll.dll!RtlUserThreadStart+0x21

I remember having had this problem with pipes myself in the past and I'll try to consult with a friend who also had the very same problem in conjunction with some proprietary software in times of Windows XP. Several publicly available tools, e.g. from Microsoft but also others, dodge this bullet by employing a driver of their own. Not really practical for Git, I'll admit.

Thinking of XP, what's the lowest version of Windows on which Git for Windows is supposed to run? I'm asking because the GetFileInformationByHandleEx is supposed to do exactly what the NtQueryObject function does in this particular case. The older GetHandleInformation is related, but doesn't quite cover the use-case. Either way you'll get fewer "🙄" from Windows devs than when using the NT native API function (i.e. NtQueryObject).

@assarbad
Copy link
Author

Relevant blog article. Looks like the Win32 API I recommended in #907 could be a good match. Need to verify, though:

  1. that it works at all
  2. if there are limitations on older Windows versions

@assarbad
Copy link
Author

@dscho the information on what your minimum supported Windows target is, as asked in #907, is becoming relevant now. Hope you find the time to comment. Thanks.

@assarbad
Copy link
Author

assarbad commented Oct 27, 2019

Switching out plain NtQueryObject() for GetFileInformationByHandleEx() doesn't have the desired effect on Windows 10 (1903).

diff --git a/compat/winansi.c b/compat/winansi.c
index c27b20a79d..0f1491f168 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -541,25 +541,10 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)

-#include <winternl.h>
-
-#if defined(_MSC_VER)
-
-typedef struct _OBJECT_NAME_INFORMATION
-{
-       UNICODE_STRING Name;
-       WCHAR NameBuffer[FLEX_ARRAY];
-} OBJECT_NAME_INFORMATION, *POBJECT_NAME_INFORMATION;
-
-#define ObjectNameInformation 1
-
-#else
-#include <ntstatus.h>
-#endif
-
 static void detect_msys_tty(int fd)
 {
-       ULONG result;
        BYTE buffer[1024];
-       POBJECT_NAME_INFORMATION nameinfo = (POBJECT_NAME_INFORMATION) buffer;
+       PFILE_NAME_INFO nameinfo = (PFILE_NAME_INFO) buffer;
        PWSTR name;

        /* check if fd is a pipe */
@@ -568,11 +553,11 @@ static void detect_msys_tty(int fd)
                return;

        /* get pipe name */
-       if (!NT_SUCCESS(NtQueryObject(h, ObjectNameInformation,
-                       buffer, sizeof(buffer) - 2, &result)))
+       if (!GetFileInformationByHandleEx(h, FileNameInfo,
+                       buffer, sizeof(buffer) - sizeof(WCHAR)))
                return;
-       name = nameinfo->Name.Buffer;
-       name[nameinfo->Name.Length / sizeof(*name)] = 0;
+       name = nameinfo->FileName;
+       name[nameinfo->FileNameLength / sizeof(*name)] = 0;

        /*
         * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')

Let me tinker around a bit. Unfortunately this means I have to leave the cozy Git SDK environment a bit and switch to VS and WinDbg to tickle out the necessary details.

NB: provided the targeted Windows version is properly expressed by means of WINVER and friends the above change also means that it works

@dscho
Copy link
Member

dscho commented Oct 27, 2019

the information on what your minimum supported Windows target is, as asked in #907, is becoming relevant now. Hope you find the time to comment.

Could you remind me, here, what my question was?

@assarbad
Copy link
Author

Which version of Windows is the minimum Git for Windows aims for? It's basically something that the project would define. But I guess the fact that code with GetFileInformationByHandleEx() could be built kind of implies that it matches the prerequisites.

@dscho
Copy link
Member

dscho commented Oct 27, 2019

Which version of Windows is the minimum Git for Windows aims for?

Currently, the earliest Windows version we support is Vista.

It's basically something that the project would define. But I guess the fact that code with GetFileInformationByHandleEx() could be built kind of implies that it matches the prerequisites.

According to https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex, this function is supported from Windows Vista on. So we're free to use it, without the need for DECLARE_PROC_ADDR()/INIT_PROC_ADDR() and a fall-back.

@dscho
Copy link
Member

dscho commented Jan 1, 2020

@assarbad I have to admit that I completely forgot about this ticket. And in the meantime, I also forgot about #304. Do you know, off the back of your head, whether this here ticket is related in some way (other than talking about git://) to #304? I.e. would your solution fix those problems, too?

@assarbad
Copy link
Author

Sorry @dscho I also lost sight of this. Will re-read what you wrote tonight and comment then.

@dscho
Copy link
Member

dscho commented Sep 29, 2020

Any news on this?

@yufeih
Copy link

yufeih commented Apr 23, 2021

Any news on this? Is it possible to have an environment variable to bypass tty detection?

@dscho
Copy link
Member

dscho commented Mar 17, 2022

Closing as stale.

@dscho dscho closed this as completed Mar 17, 2022
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