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

Git commands inside vscode fail when using basic authentication #5115

Closed
1 task done
isaacag opened this issue Aug 29, 2024 · 7 comments · Fixed by git-for-windows/MSYS2-packages#187
Closed
1 task done
Assignees
Milestone

Comments

@isaacag
Copy link

isaacag commented Aug 29, 2024

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

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
$ git --version --build-options

git version 2.46.0.windows.1
cpu: x86_64
built from commit: 2e6a859ffc0471f60f79c1256f766042b0d5d17d
sizeof-long: 4
sizeof-size_t: 8
shell-path: D:/git-sdk-64-build-installers/usr/bin/sh
feature: fsmonitor--daemon
libcurl: 8.9.0
OpenSSL: OpenSSL 3.2.2 4 Jun 2024
zlib: 1.3.1
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19045.4651]
  • What options did you set as part of the installation? Or did you choose the
    defaults?
# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
> type "$env:USERPROFILE\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: Nano
Custom Editor Path: 
Default Branch Option:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Merge
Use Credential Manager: Enabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Disabled
Enable Pseudo Console Support: Disabled
Enable FSMonitor: Disabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

No

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

Powershell (but also tested in CMD and Git Bash)

git config --system --unset credential.helper
git pull
  • What did you expect to occur after running these commands?
  1. Open a terminal inside Visual Studio Code
  2. Run git commands above

At 2 after two vscode popups for user and password git command is executed.

  • What actually happened instead?

At 2 after two vscode popups for user and password, terminal shows:
fatal: read error: Invalid argument
fatal: expected flush after ref listing

  • If the problem was occurring with a specific repository, can you provide the
    URL to that repository to help us with testing?

Problem is happening in all repositories.

  • Notes

Downgrading to Git 2.45 fixes the problem.
Error is not reproducible on terminals outside vscode (powershell, cmd, git bash).
Error is not reproducible when windows credential manager integration is active.

@dscho
Copy link
Member

dscho commented Aug 29, 2024

I can reproduce this problem. A less invasive reproducer is:

$ git -c credential.helper= -c credential.helper= ls-remote https://github.com/<private-repository>

Unfortunately, due to the way VS Code inserts itself via GIT_ASKPASS, the private repository must exist and the current user needs to be authorized to access it, therefore it is not such a straight-forward reproducer.

@dscho
Copy link
Member

dscho commented Aug 29, 2024

I've bisected this to git-for-windows/msys2-runtime@a4d92d6.

@tyan0 this is one of your patches (backported from your upstream commit cygwin/cygwin@55431b4), do you have any idea what is going wrong here?

@tyan0
Copy link

tyan0 commented Aug 30, 2024

I could reproduce the problem. I'll look into this problem, so please wait a while.

@dscho
Copy link
Member

dscho commented Aug 30, 2024

I could reproduce the problem. I'll look into this problem, so please wait a while.

Thank you.

@tyan0
Copy link

tyan0 commented Aug 30, 2024

@dscho I have submitted a patch for this issue: https://cygwin.com/pipermail/cygwin-patches/2024q3/012747.html.
For cygwin-3_5-branch, please try cygwin-3_5-branch.patch

dscho pushed a commit to dscho/msys2-runtime that referenced this issue Sep 2, 2024
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, read pipe remains on non-blocking mode because of the
commit fc691d0246b9. Due to this behaviour, the non-cygwin app
cannot read the pipe correctly after that. With this patch, the
blocking mode of the read pipe is stored into was_blocking_read_pipe
on set_pipe_non_blocking() when the cygwin app starts and restored
on close().

Addresses: git-for-windows/git#5115
Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reviewed-by:
Signed-off-by: Takashi Yano <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Sep 2, 2024
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, read pipe remains on non-blocking mode because of the
commit fc691d0246b9. Due to this behaviour, the non-cygwin app
cannot read the pipe correctly after that. With this patch, the
blocking mode of the read pipe is stored into was_blocking_read_pipe
on set_pipe_non_blocking() when the cygwin app starts and restored
on close().

Addresses: git-for-windows/git#5115
Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reported-at: git-for-windows/git#5115
Signed-off-by: Takashi Yano <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho added this to the Next release milestone Sep 15, 2024
@dscho dscho self-assigned this Sep 15, 2024
dscho added a commit to dscho/MSYS2-packages that referenced this issue Sep 15, 2024
The Cygwin runtime (and for that reason, the MSYS2 runtime, too), change
pipes from blocking to non-blocking, which is a problem when the pipe is
actually opened by something like Git when it asks a credential helper
for information. Git, not expecting the pipe mode to be changed behind
its back, will then error out with "fatal: read error: Invalid
argument".

To address this, this patch was proposed in the Cygwin project as
https://inbox.sourceware.org/cygwin-patches/[email protected]/

Ideally, Cygwin would not even _try_ to fiddle with the pipe mode,
therefore this patch was not applied as-is. At time of writing, there is
no consensus on any replacement patch, but I have to prepare _something_
for the already-late Git for Windows v2.46.1 (which is likely the last
Git for Windows version to support Windows 7 and Windows 8, hence the
urgency). And this patch at least improves the situation.

This patch will be dropped when Git for Windows will upgrade to MSYS2
runtime v3.5, and hopefully consensus will have been reached about a
better fix by that time.

This commit corresponds to
git-for-windows/msys2-runtime#72 which fixes
git-for-windows/git#5115.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho

This comment has been minimized.

@dscho
Copy link
Member

dscho commented Sep 15, 2024

/add relnote bug When using an askpass helper (e.g. implicitly when running inside VS Code's internal terminal), Git v2.46.0 would error out with "read error: Invalid argument"; This bug has been fixed.

The workflow run was started

github-actions bot pushed a commit to git-for-windows/build-extra that referenced this issue Sep 15, 2024
When using an `askpass` helper (e.g. implicitly when running inside VS
Code's internal terminal), Git v2.46.0 [would error out with "read
error: Invalid
argument"](git-for-windows/git#5115); This bug
has been fixed.

Signed-off-by: gitforwindowshelper[bot] <[email protected]>
github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Sep 17, 2024
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, the read pipe remains in the non-blocking mode because
of the commit fc691d0. Due to this behaviour, the non-cygwin
app cannot read the pipe correctly after that. Similarly, if a
non-cygwin app is executed from a cygwin app and the non-cygwin
app exits, the read pipe remains in the blocking mode. With this
patch, the blocking mode of the read pipe is stored into a variable
was_blocking_read_pipe on set_pipe_non_blocking() when the cygwin
app starts and restored on close(). In addition, the pipe mode is
set to non-blocking mode in raw_read() if the mode is blocking
mode by referring the variable is_blocking_read_pipe as well.
is_blocking_read_pipe is a member of fhandler_pipe class and is set
by set_pipe_non_blocking(), so if other process sets the pipe mode
to blocking mode, the current process cannot know the pipe is
blocking mode. Therefore, is_blocking_read_pipe is also set on the
signal __SIGNONCYGCHLD, which is sent to the process group when
non-cygwin app is started.

Addresses: git-for-windows/git#5115
Fixes: fc691d0 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reviewed-by: Corinna Vinschen <[email protected]>, Ken Brown <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Sep 18, 2024
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, the read pipe remains in the non-blocking mode because
of the commit fc691d0246b9. Due to this behaviour, the non-cygwin
app cannot read the pipe correctly after that. Similarly, if a
non-cygwin app is executed from a cygwin app and the non-cygwin
app exits, the read pipe remains in the blocking mode. With this
patch, the blocking mode of the read pipe is stored into a variable
was_blocking_read_pipe on set_pipe_non_blocking() when the cygwin
app starts and restored on close(). In addition, the pipe mode is
set to non-blocking mode in raw_read() if the mode is blocking
mode by referring the variable is_blocking_read_pipe as well.
is_blocking_read_pipe is a member of fhandler_pipe class and is set
by set_pipe_non_blocking(), so if other process sets the pipe mode
to blocking mode, the current process cannot know the pipe is
blocking mode. Therefore, is_blocking_read_pipe is also set on the
signal __SIGNONCYGCHLD, which is sent to the process group when
non-cygwin app is started.

Backported-from: c7fe29f5cb (Cygwin: pipe: Restore blocking mode of read pipe on close()/raw_read(), 2024-09-06)
Addresses: git-for-windows/git#5115
Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reviewed-by: Corinna Vinschen <[email protected]>, Ken Brown <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Sep 25, 2024
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, the read pipe remains in the non-blocking mode because
of the commit fc691d0246b9. Due to this behaviour, the non-cygwin
app cannot read the pipe correctly after that. Similarly, if a
non-cygwin app is executed from a cygwin app and the non-cygwin
app exits, the read pipe remains in the blocking mode. With this
patch, the blocking mode of the read pipe is stored into a variable
was_blocking_read_pipe on set_pipe_non_blocking() when the cygwin
app starts and restored on close(). In addition, the pipe mode is
set to non-blocking mode in raw_read() if the mode is blocking
mode by referring the variable is_blocking_read_pipe as well.
is_blocking_read_pipe is a member of fhandler_pipe class and is set
by set_pipe_non_blocking(), so if other process sets the pipe mode
to blocking mode, the current process cannot know the pipe is
blocking mode. Therefore, is_blocking_read_pipe is also set on the
signal __SIGNONCYGCHLD, which is sent to the process group when
non-cygwin app is started.

Backported-from: c7fe29f5cb (Cygwin: pipe: Restore blocking mode of read pipe on close()/raw_read(), 2024-09-06)
Addresses: git-for-windows/git#5115
Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reviewed-by: Corinna Vinschen <[email protected]>, Ken Brown <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Oct 31, 2024
Previously, cygwin read pipe used non-blocking mode although non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwin app and the cygwin
app exits, read pipe remains on non-blocking mode because of the
commit fc691d0. Due to this behaviour, the non-cygwin app
cannot read the pipe correctly after that. Similarly, if a non-
cygwin app is executed from a cygwin app and the non-cygwin app
exits, the read pipe mode remains on blocking mode although cygwin
read pipe should be non-blocking mode.

These bugs were provoked by pipe mode toggling between cygwin and
non-cygwin apps. To make management of pipe mode simpler, this
patch has re-designed the pipe implementation. In this new
implementation, both read and write pipe basically use only blocking
mode and the behaviour corresponding to the pipe mode is simulated
in raw_read() and raw_write(). Only when NtQueryInformationFile
(FilePipeLocalInformation) fails for some reasons, the raw_read()/
raw_write() cannot simulate non-blocking access. Therefore, the pipe
mode is temporarily changed to non-blocking mode.

Moreover, because the fact that NtSetInformationFile() in
set_pipe_non_blocking(true) fails with STATUS_PIPE_BUSY if the pipe
is not empty has been found, query handle is not necessary anymore.
This allows the implementation much simpler than before.

Addresses: git-for-windows/git#5115
Fixes: fc691d0 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
Reported-by: isaacag, Johannes Schindelin <[email protected]>
Reviewed-by: Corinna Vinschen <[email protected]>, Ken Brown <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
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

Successfully merging a pull request may close this issue.

3 participants