Win32-OpenSSH icon indicating copy to clipboard operation
Win32-OpenSSH copied to clipboard

Git server doesn't work with Win32-OpenSSH. Enclosed, please find three defects which I isolated and worked around..

Open razrjk opened this issue 8 years ago • 52 comments
trafficstars

I pulled version: 0.0.14.0 on May 19. I just fetched version: 0.0.15.0 (June 4) today to compare. I don't believe the below issues have been corrected.

Git using Win32-OpenSSH ssh server runs as a nopty service. There are two defects in the nopty code:

  1. shell-host.c passes through to cmd.exe, single quotes as literals. The literal single quotes become part of the file path name, resulting in an incorrect file path.
  2. The service loop in shell-host.c, which passes its stdin to the child stdin is coded with incorrect file handles. Part of the input is currently passed back to sshd.

A temporary workaround for issue 1 is to replace all single quotes with double quotes just before the call to invoke cmd /c:

@@ -1136,7 +1136,21 @@ start_withno_pty(wchar_t *command)
	if (command) {
		GOTO_CLEANUP_ON_ERR(wcscat_s(cmd, MAX_CMD_LEN, L" /c"));
		GOTO_CLEANUP_ON_ERR(wcscat_s(cmd, MAX_CMD_LEN, L" "));

	+	int i = 0;
	+	while (command[i] != L'\0')
	+	{
	+		if (command[i] == L'\'' )
	+			command[i] = L'\"';
	+		i++;
	+	}
		GOTO_CLEANUP_ON_ERR(wcscat_s(cmd, MAX_CMD_LEN, command));

A fix for issue 2 is to use the child process file handle instead of the sshd file handle:

@@ -1184,7 +1198,7 @@ start_withno_pty(wchar_t *command)
			/* for backspace, we need to send space and another backspace for visual erase */
			if (buf[i] == '\b') {
				if (in_cmd_len > 0) {
-					GOTO_CLEANUP_ON_FALSE(WriteFile(pipe_out, "\b \b", 3, &wr, NULL));
+					GOTO_CLEANUP_ON_FALSE(WriteFile(child_pipe_write, "\b \b", 3, &wr, NULL));
					in_cmd_len--;
				}
				i++;
@@ -1194,10 +1208,10 @@ start_withno_pty(wchar_t *command)
			/* For CR and LF */
			if ((buf[i] == '\r') || (buf[i] == '\n')) {
				/* TODO - do a much accurate mapping */
-				GOTO_CLEANUP_ON_FALSE(WriteFile(pipe_out, buf + i, 1, &wr, NULL));
+				GOTO_CLEANUP_ON_FALSE(WriteFile(child_pipe_write, buf + i, 1, &wr, NULL));
				if ((buf[i] == '\r') && ((i == rd - 1) || (buf[i + 1] != '\n'))) {
					buf[i] = '\n';
-					GOTO_CLEANUP_ON_FALSE(WriteFile(pipe_out, buf + i, 1, &wr, NULL));
+					GOTO_CLEANUP_ON_FALSE(WriteFile(child_pipe_write, buf + i, 1, &wr, NULL));
				}
				in_cmd[in_cmd_len] = buf[i];
				in_cmd_len++;
@@ -1207,7 +1221,7 @@ start_withno_pty(wchar_t *command)
				continue;
			}
 
-			GOTO_CLEANUP_ON_FALSE(WriteFile(pipe_out, buf + i, 1, &wr, NULL));
+			GOTO_CLEANUP_ON_FALSE(WriteFile(child_pipe_write, buf + i, 1, &wr, NULL));
			in_cmd[in_cmd_len] = buf[i];
			in_cmd_len++;
			if (in_cmd_len == MAX_CMD_LEN - 1) {

Finally, there is defect and debug issue in signal_sigchld.c. The zombied children count can become too large, which causes a seg fault. When debugging sshd.exe /ddd, debug3() is writing during signal processing which corrupts the stack. Debug3() should not write while running on an exception frame. A check for incorrect zombie count should be added:

@@ -104,14 +104,20 @@ sw_child_to_zombie(DWORD index)
	DWORD last_non_zombie, zombie_pid;
	HANDLE zombie_handle;
 
-	debug3("zombie'ing child at index %d, %d zombies of %d", index,
-		children.num_zombies, children.num_children);
+	//debug3("zombie'ing child at index %d, %d zombies of %d", index,
+		//children.num_zombies, children.num_children);
 
	if (index >= children.num_children) {
		errno = EINVAL;
		return -1;
	}
-
+	//debug3(" %x zombies,  %x children",children.num_zombies, children.num_children);
+	if((children.num_children - children.num_zombies) == 0)
+	{
+		//debug2("children == zombies, can't make more zombies");
+		errno = EINVAL;
+		return -1;
+	}
	last_non_zombie = children.num_children - children.num_zombies - 1;
	if (last_non_zombie != index) {
		/* swap */

I'm running on Windows 10 and Windows 7 systems.

Extreme care must be taken with search paths when debugging a git server using ssh protocol on Windows, especially if cygwin and mingw, etc., are also installed on the system This is true for the client and server side. Multiple executables for ssh, sshd, and git commands can cause problems if close attention is not paid to search paths.

razrjk avatar Jun 04 '17 21:06 razrjk

Thanks for the detailed report and proposed changes. Found some details here on how to setup Git server using over SSH - https://www.systutorials.com/366/set-up-git-server-through-ssh-connection/

manojampalam avatar Jun 05 '17 05:06 manojampalam

Re: shell-host.c Git push was still failing.

It appears that the whole nopty implementation assumes a pty control stream. Yet nopty may be a binary data stream. While I'm uncertain about the defaults and all the possible options of ssh (for instance, assuming pty control for a nopty connection), tty control should not be processed on a binary data stream.

After removing the pty processing from: start_withno_pty(wchar_t *command) Git push works.

I can now clone repos over ssh and push changes to them. I will continue to test.

The whole send to child inner loop simplifies to:

		while(rd){
			GOTO_CLEANUP_ON_FALSE(WriteFile(child_pipe_write, buf + i, rd, &wr, NULL));
			rd = rd - wr;
			i = i + wr;		
		}

razrjk avatar Jun 05 '17 16:06 razrjk

What repo are you working on? There were changes around this logic as part of fixing https://github.com/PowerShell/Win32-OpenSSH/issues/658 Take a look at the PR, binary streaming over stdin should work with the latest changes included in v0.0.15.0.

manojampalam avatar Jun 05 '17 20:06 manojampalam

The repo is [email protected]:PowerShell/Win32-OpenSSH.git. I cloned, built, and tried to test v.0.0.15.0.

Issue 1) from my initial post is still a problem. The file path sent by git contains literal single quote characters which become part of the file path, so the path is invalid.

The remote repo in the following case is: localhost:D:\work\git_repo.git. Note the single quotes below. They may look like a double quote, but they are single quotes around single quotes. The inside single quotes are literals in the file path.

$ git ls-remote
fatal: ''d:/work/git_repo.git'' does not appear to be a git repository
fatal: Could not read from remote repository.

I also originally tried powershell.exe and bash.exe. None of the command line interpreters liked the literal single quotes when passed with the "/c" or "-c" option. cmd.exe works okay with double quotes, so I stopped looking for another solution to the problem.

Regarding issue 2) of my original post: I do see quite a few changes, but the writes to pipe_out are still going back to the sshd process, not the child process child_pipe_write handle. Next, unless ssh options are mapped directly to cmd.exe options, I don't see how you can possibly detect any sub-mode of nopty, When the pipe is nopty, it's binary. A shell with tty capabilities would need to look at the binary stream and implement any tty escapes, not shell-host.c. After starting the process (cmd.exe), shell-host.c must simply get out of the way and let the binary data flow, in both directions.

Also, sshd.exe /ddd is still broken.

razrjk avatar Jun 06 '17 04:06 razrjk

I finally resolved another problem with large binary transfers, typical of a big git push.

shell-host.c is currently using cmd.exe. cmd.exe was dropping the end of large binary streams over slow network connection. I tried bash.exe and powershell.exe as command interpreters and they do not drop any of the stream. It took quite a long time to discover that cmd.exe was the problem.

I settled on using powershell.exe because it acts like cmd.exe for gitExtension file path resolution and resolves the common forms of path for Windows:

ssh://<host1>://<host2>/<repo-path>
<host2>:<drive>:/<repo-path>

razrjk avatar Jun 07 '17 00:06 razrjk

@razrjk as an FYI, we do code work out of https://github.com/PowerShell/openssh-portable, and only take drops back to this repo when we do releases.

joeyaiello avatar Jun 13 '17 17:06 joeyaiello

@razrjk please submit a PR on https://github.com/PowerShell/openssh-portable for the proposed changes. Also open a new issue for sshd.exe -ddd

Also add details on where you installed Git for Windows from.

manojampalam avatar Jun 14 '17 20:06 manojampalam

To work around this issue, you can run the following commands in your local repository:

git config --local remote.origin.receivepack "powershell git receive-pack"
git config --local remote.origin.uploadpack "powershell git upload-pack"

Replace origin with the name of the Windows-hosted remote.

After this you should be able to pull and push normally (works for me, anyway).

Make sure you don't select "Use Git from Git Bash only" when installing Git on the Windows server, otherwise Git won't be in the PowerShell path.

diogocp avatar Jul 12 '17 22:07 diogocp

After further testing, I still get an error (early EOF) when pushing binary files. This is with v0.0.17.0.

diogocp avatar Jul 13 '17 00:07 diogocp

That's  because it went through cmd to call powershell. I spent a day isolating the problem. My powershe'll bases ssh git server is working great!

Sent from Yahoo Mail on Android

On Wed, Jul 12, 2017 at 8:29 PM, Diogo Pereira[email protected] wrote:
After further testing, I still get an error (early EOF) when pushing binary files.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

razrjk avatar Jul 13 '17 01:07 razrjk

@razrjk, I start looking this. Could you please share more information so I can try it from end to end?

  1. Can you share the steps you setup the git client? Are you using the git client for window from this link? or a unix client?
  2. Can you share the steps you set up git server over win32_openssh?(steps to setup git server and steps to config sshd so it talk to the git server)
  3. The git commands you repro the problems ( connect from your client to the git server over sshd)

bingbing8 avatar Sep 16 '17 00:09 bingbing8

  1. Yes. Git for Windows with the mingw unix tools also installed. The ssh client doesn't matter (except to know which one is being used for debugging purposes). I've used several different combinations: OpenSSH, cygwin, Git ssh, Git putty.
  2. It has been a while since I got everything working. Attached are my raw notes, there is some random information in these notes. notes.txt
  3. With the fixes I described in this thread, my systems have been working, so I have no problems. I'm responding to let you know that the fixes worked, and similar fixes need to be added to the OpenSSH repo. We use OpenSSH sshd with Git now, all the time in our production development.

Above, I described three problems with the sshd nopty path (as of the date above - some of these problems may have been resolved. I have not pulled and rebuilt for a long time):

  1. File/path escapes did not work for Windows files/paths. The problem was corrected as described above.
  2. Data was being echoed back to sshd from cmd.exe because some file handles were incorrect.
  3. Finally, cmd.exe does not work for large streamed data. The pipe breaks under heavy traffic load. This is a difficult problem to isolate. I finally decided to use Powershell instead of cmd.exe to work around this problem (for nopty), and it is working great.

My Git repo paths take the form:

> ssh://<opensshd-host>:<port>//<git-host>/<Git_Repo>
ssh://<opensshd-host>:<port>//<git-host>/<Git_Repo>

This supports efficient transfer between sites using Git compression over ssh, and yet the sshd server can use all the different types of Windows file path syntax. The double "//" in the path looks unusual, but it works.

My systems are using Windows Active Directory and all accounts are domain accounts, validated by the Domain Controller.

Interactive ssh/sshd work as well.

razrjk avatar Sep 16 '17 17:09 razrjk

@razrjk, my remote repo is setup at d:\testgit\myrepo.git on hostmachine. How do you specify the url for the path to your repo. I always see a '/' prefix at my URL. How do you remove it? I use this command:

git clone ssh://username@domain@hostmachine/D:/testgit/myrepo.git
Cloning into 'myrepo'...
fatal: ''/D:/testgit/myrepo.git'' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Here is the ssh log: debug1: Sending command: git-upload-pack '/D:\testgit\myrepo.git' debug2: channel 0: request exec confirm 1 debug3: send packet: type 98 debug2: callback done debug2: channel 0: open confirm rwindow 0 rmax 32768 debug2: channel 0: rcvd adjust 2097152 debug3: receive packet: type 99 debug2: channel_input_status_confirm: type 99 id 0 debug2: exec request accepted on channel 0 debug2: channel 0: rcvd ext data 73 fatal: ''/D:\testgit\myrepo.git'' does not appear to be a git repository

bingbing8 avatar Sep 19 '17 23:09 bingbing8

Try git clone username@domain@hostmachine:D:/testgit/myrepo.git.

You will probably continue to get the "does not appear to be a git repository" error after that, because of the way that cmd.exe handles single quotes (notice that there are two sets of single quotes around ''/D:/testgit/myrepo.git'' in the error message). That is the first problem that @razrjk identified.

If you want to try my workaround, you can do it this way:

git init myrepo
cd myrepo
git remote add origin username@domain@hostmachine:D:/testgit/myrepo.git
git config --local remote.origin.uploadpack "powershell git upload-pack"
git fetch origin

diogocp avatar Sep 19 '17 23:09 diogocp

@diogocp Thanks. yes, git clone username@domain@hostmachine:D:/testgit/myrepo.git works. your workaround works too. I am looking at the first issue @razrjk reported.

bingbing8 avatar Sep 20 '17 04:09 bingbing8

@diogocp how do you specify the port if you are not using the default 22? git clone username@domain@hostmachine:47003:D:/testgit/myrepo.git does not work.

bingbing8 avatar Sep 20 '17 21:09 bingbing8

@bingbing8 I would set it in ~/.ssh/config. Another alternative would be to set the environment variable GIT_SSH_COMMAND="ssh -p 47003". I didn't test that, but it should work.

diogocp avatar Sep 20 '17 21:09 diogocp

@diogocp, I don't repro the issues when pushing binary files (I use an executable file). I add the path to git-upload-pack to the system environment variable %Path% on host machine. Did you try on 0.0.20.0?

bingbing8 avatar Sep 21 '17 02:09 bingbing8

Yes, I tried on 0.0.20.0. However, this does not happen with all binary files.

@razrjk seems to think it is related to the size of the data ("cmd.exe does not work for large streamed data. The pipe breaks under heavy traffic load."). I have some doubts about this theory because:

  1. I have seen it succeed when pushing tens of megabytes and fail on pushes of less than 1 MB.
  2. When it fails, it seems to fail consistently, no matter how many times you retry. It also doesn't seem to be affected by network speeds.

You should be able to consistently reproduce it this way (at least I can):

git clone https://github.com/PowerShell/Win32-OpenSSH.git
git init newrepo
cd Win32-OpenSSH
git remote add newrepo localhost:path/to/newrepo
git config --local remote.newrepo.receivepack "powershell git receive-pack"
git push newrepo

diogocp avatar Sep 21 '17 12:09 diogocp

Here is a simpler alternative. Try pushing this 28K file: https://repo.gradle.org/gradle/libs-releases-local/org/gradle/gradle-wrapper/4.1/gradle-wrapper-4.1.jar

diogocp avatar Sep 21 '17 13:09 diogocp

Diogo, By "heavy traffic load" I mean large transfer over a weak internet connection.  This is why it was difficult to isolate the problem.  cmd.exe did not work, powershell.exe does.  When you isolate the problem down to seeing that the pipe is breaking in cmd.exe for no apparent reason, then you will know. Consider that you may have another problem if it is failing all the time.

razrjk avatar Sep 21 '17 15:09 razrjk

I want to clarify a point. I'm not sure how the development has proceeded recently. To use powershell in the nopty path, you must change the sshd source code use use it. The shell is specified in shell-host.c. If you do not do this, and instead pass powershell as the shell on a command line, then cmd.exe will invoke powershell.exe and you will not have removed cmd.exe from the workflow.

razrjk avatar Sep 21 '17 16:09 razrjk

After doing some more testing, it looks like the failures are not consistent after all. The file that I meantioned above (gradle-wrapper-4.1.jar) failed every time, but after trimming it to around 4100-4200 bytes it starts failing randomly. Less than that and it always succeeds. It's really weird. The transfer seems to always complete in any case, the error is during the unpacking.

@razrjk this happens even when pushing to a remote on localhost. Also, do you have any idea why Git behaves like this while scp works perfectly?

diogocp avatar Sep 21 '17 18:09 diogocp

@diogocp, did you repro the binary push with powershell launched in shell-host or the cmd?

bingbing8 avatar Sep 22 '17 01:09 bingbing8

I didn't patch anything, I just installed it using chocolatey.

diogocp avatar Sep 22 '17 01:09 diogocp

@diogocp my question is that did you set the following config to use powershell when repro the issue? I assume yes; otherwise you will see the single quotes issue. git config --local remote.origin.receivepack "powershell git-receive-pack" git config --local remote.origin.uploadpack "powershell git-upload-pack"

bingbing8 avatar Sep 22 '17 01:09 bingbing8

@bingbing8 yes, that's what I did.

diogocp avatar Sep 22 '17 07:09 diogocp

@diogocp, after that config to use powershell, push of file gradle-wrapper-4.1.jar works for me. I will try more variation.

bingbing8 avatar Sep 22 '17 16:09 bingbing8

That's interesting. I've tried it dozens of times, creating new repositories (local and remote) each time, and it always fails. I've also tried this on two different machines, one running Windows Server 2008 R2 and the other Windows 10.

I'll see if I can reproduce it in a VM later.

diogocp avatar Sep 22 '17 16:09 diogocp

@razrjk, could you share your code changes to launch powershell in shell-host.c? I don't see the difference between pass "powershell git receive-pack" as command. The code path this line. Did you pass "C:\Windows\System32\WindowsPowerShell\v1.0" as the module name of CreateProcessW? ret = CreateProcessW(NULL, command, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi);

bingbing8 avatar Sep 25 '17 18:09 bingbing8