From 4edff78b9dde0c8c2f445c54dbe30db84b3e4d5d Mon Sep 17 00:00:00 2001 From: Manoj Ampalam Date: Mon, 6 Nov 2017 21:38:14 -0800 Subject: [PATCH] Fixed issue in POSIX layer that could truncate write streams (#235) PowerShell/Win32-OpenSSH#908 --- contrib/win32/win32compat/fileio.c | 18 ++++++++++++++++- contrib/win32/win32compat/signal.c | 2 +- contrib/win32/win32compat/socketio.c | 4 ++-- contrib/win32/win32compat/termio.c | 6 +++--- regress/pesterTests/SSH.Tests.ps1 | 30 ++++++++++++++++++++++++++++ 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/contrib/win32/win32compat/fileio.c b/contrib/win32/win32compat/fileio.c index 72cdcb2de..3b3a99c41 100644 --- a/contrib/win32/win32compat/fileio.c +++ b/contrib/win32/win32compat/fileio.c @@ -628,7 +628,7 @@ WriteCompletionRoutine(_In_ DWORD dwErrorCode, pio->write_details.error = dwErrorCode; /* TODO - assert that remaining == dwNumberOfBytesTransfered */ if ((dwErrorCode == 0) && (pio->write_details.remaining != dwNumberOfBytesTransfered)) { - debug3("WriteCB - ERROR: broken assumption, io:%p, wrote:%d, remaining:%d", pio, + error("WriteCB - ERROR: broken assumption, io:%p, wrote:%d, remaining:%d", pio, dwNumberOfBytesTransfered, pio->write_details.remaining); DebugBreak(); } @@ -902,6 +902,22 @@ fileio_close(struct w32_io* pio) return 0; } + /* + * we report to POSIX app that an async write has completed as soon its + * copied to internal buffer. The app may subsequently try to close the + * fd thinking everything is written. IF the Windows handle is closed + * now, the pipe/file io write operation may terminate prematurely. + * To compensate for the discrepency + * wait here until async write has completed. + * If you see any process waiting here indefinitely - its because no one + * is draining from other end of the pipe/file. This is an unfortunate + * consequence that should otherwise have very little impact on practical + * scenarios. + */ + while (pio->write_details.pending) + if (0 != wait_for_any_event(NULL, 0, INFINITE)) + return -1; + CancelIo(WINHANDLE(pio)); /* let queued APCs (if any) drain */ SleepEx(0, TRUE); diff --git a/contrib/win32/win32compat/signal.c b/contrib/win32/win32compat/signal.c index 17fd9b30d..dbccc8321 100644 --- a/contrib/win32/win32compat/signal.c +++ b/contrib/win32/win32compat/signal.c @@ -205,7 +205,7 @@ sw_process_pending_signals() sigdelset(&pending_tmp, exp[i]); if (pending_tmp) { /* unexpected signals queued up */ - debug3("process_signals() - ERROR unexpected signals in queue: %d", pending_tmp); + error("process_signals() - ERROR unexpected signals in queue: %d", pending_tmp); errno = ENOTSUP; DebugBreak(); return -1; diff --git a/contrib/win32/win32compat/socketio.c b/contrib/win32/win32compat/socketio.c index 290f92c2c..d0078dcd8 100644 --- a/contrib/win32/win32compat/socketio.c +++ b/contrib/win32/win32compat/socketio.c @@ -499,7 +499,7 @@ CALLBACK WSASendCompletionRoutine(IN DWORD dwError, pio->write_details.error = dwError; /* TODO - assert that remaining == cbTransferred */ if ((dwError == 0) && (pio->write_details.remaining != cbTransferred)) { - debug3("WSASendCB - ERROR: broken assumption, io:%p, sent:%d, remaining:%d", pio, + error("WSASendCB - ERROR: broken assumption, io:%p, sent:%d, remaining:%d", pio, cbTransferred, pio->write_details.remaining); DebugBreak(); } @@ -633,7 +633,7 @@ socketio_close(struct w32_io* pio) SleepEx(0, TRUE); if ((pio->internal.state == SOCK_READY) && (pio->read_details.pending || pio->write_details.pending)) { - debug4("close - IO is still pending on closed socket. read:%d, write:%d, io:%p", + error("close - IO is still pending on closed socket. read:%d, write:%d, io:%p", pio->read_details.pending, pio->write_details.pending, pio); DebugBreak(); } diff --git a/contrib/win32/win32compat/termio.c b/contrib/win32/win32compat/termio.c index 02fc74383..372d5b0aa 100644 --- a/contrib/win32/win32compat/termio.c +++ b/contrib/win32/win32compat/termio.c @@ -58,7 +58,7 @@ ReadAPCProc(_In_ ULONG_PTR dwParam) { struct w32_io* pio = (struct w32_io*)dwParam; debug5("TermRead CB - io:%p, bytes: %d, pending: %d, error: %d", pio, read_status.transferred, - pio->read_details.pending, read_status.error); + pio->read_details.pending, pio->sync_read_status.error); pio->read_details.error = pio->sync_read_status.error; pio->read_details.remaining = pio->sync_read_status.transferred; pio->read_details.completed = 0; @@ -170,7 +170,7 @@ WriteAPCProc(_In_ ULONG_PTR dwParam) { struct w32_io* pio = (struct w32_io*)dwParam; debug5("TermWrite CB - io:%p, bytes: %d, pending: %d, error: %d", pio, write_status.transferred, - pio->write_details.pending, write_status.error); + pio->write_details.pending, pio->sync_write_status.error); pio->write_details.error = pio->sync_write_status.error; pio->write_details.remaining -= pio->sync_write_status.transferred; /* TODO- assert that reamining is 0 by now */ @@ -212,7 +212,7 @@ WriteThread(_In_ LPVOID lpParameter) if (0 == QueueUserAPC(WriteAPCProc, main_thread, (ULONG_PTR)pio)) { - debug3("WriteThread thread - ERROR QueueUserAPC failed %d, io:%p", GetLastError(), pio); + error("WriteThread thread - ERROR QueueUserAPC failed %d, io:%p", GetLastError(), pio); pio->write_details.pending = FALSE; pio->write_details.error = GetLastError(); DebugBreak(); diff --git a/regress/pesterTests/SSH.Tests.ps1 b/regress/pesterTests/SSH.Tests.ps1 index 4dc2f3085..66bc92c93 100644 --- a/regress/pesterTests/SSH.Tests.ps1 +++ b/regress/pesterTests/SSH.Tests.ps1 @@ -22,6 +22,11 @@ Describe "E2E scenarios for ssh client" -Tags "CI" { { $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue } + $acl = Get-Acl $testDir + $rights = [System.Security.AccessControl.FileSystemRights]"Read, Write" + $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($ssouser, $rights, "ContainerInherit,Objectinherit", "None", "Allow") + $acl.SetAccessRule($accessRule) + Set-Acl -Path $testDir -AclObject $acl $platform = Get-Platform $skip = ($platform -eq [PlatformType]::Windows) -and ($PSVersionTable.PSVersion.Major -le 2) @@ -151,6 +156,31 @@ Describe "E2E scenarios for ssh client" -Tags "CI" { $o = $h | ssh test_target PowerShell -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -EncodedCommand $EncodedText 2> $null $o | Should Be "8" } + + It "$tC.$tI - stream file in and out" { + # prep a file of size > 10KB (https://github.com/PowerShell/Win32-OpenSSH/issues/908 was caught with such file size) + $str = "" + (1..100) | foreach {$str += "1234567890"} + #strem file from local to remote + $testsrc = Join-Path $testDir "$tC.$tI.testsrc" + $testdst1 = Join-Path $testDir "$tC.$tI.testdst1" + $null | Set-Content $testsrc + $null | Set-Content $testdst1 + (1..105) | foreach {Add-Content -Encoding Ascii -Path $testsrc -Value $str} + # execute this script that dumps input stream in target file, on the remote end + $str = "begin {} process { Add-Content -Encoding Ascii -path $testdst1 -Value ([string]`$input)} end { }" + $EncodedText =[Convert]::ToBase64String([System.Text.Encoding]::Unicode.GetBytes($str)) + # ignore error stream using 2> $null + get-content $testsrc | ssh test_target PowerShell -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -EncodedCommand $EncodedText 2> $null + (dir $testdst1).Length | Should Be (dir $testsrc).Length + + # stream file from remote to local + $testdst2 = Join-Path $testDir "$tC.$tI.testdst2" + $null | Set-Content $testdst2 + (ssh test_target powershell get-content $testdst1 -Encoding Ascii) | Set-Content $testdst2 -Encoding ASCII + (dir $testdst2).Length | Should Be (dir $testsrc).Length + + } } Context "$tC - configure default shell Scenarios" {