From 9b7ee920289cf18fdcbd4e1b7d89d208835e9385 Mon Sep 17 00:00:00 2001
From: Justin Kotalik <jukotali@microsoft.com>
Date: Mon, 2 Jul 2018 17:18:05 -0700
Subject: [PATCH] Check thread invalid handle and remove extra dup2 call.
 (#1008)

---
 .../RequestHandlerLib/PipeOutputManager.cpp   | 12 ++++++-----
 .../CommonLibTests/PipeOutputManagerTests.cpp | 20 +++++++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/AspNetCoreModuleV2/RequestHandlerLib/PipeOutputManager.cpp b/src/AspNetCoreModuleV2/RequestHandlerLib/PipeOutputManager.cpp
index 5620572c6f..e8b61a962b 100644
--- a/src/AspNetCoreModuleV2/RequestHandlerLib/PipeOutputManager.cpp
+++ b/src/AspNetCoreModuleV2/RequestHandlerLib/PipeOutputManager.cpp
@@ -13,7 +13,7 @@ PipeOutputManager::PipeOutputManager() :
     m_dwStdErrReadTotal(0),
     m_hErrReadPipe(INVALID_HANDLE_VALUE),
     m_hErrWritePipe(INVALID_HANDLE_VALUE),
-    m_hErrThread(INVALID_HANDLE_VALUE),
+    m_hErrThread(NULL),
     m_fDisposed(FALSE)
 {
     InitializeSRWLock(&m_srwLock);
@@ -54,7 +54,6 @@ PipeOutputManager::StopOutputRedirection()
 
     if (m_fdPreviousStdOut >= 0)
     {
-        _dup2(m_fdPreviousStdOut, _fileno(stdout));
         LOG_IF_DUPFAIL(_dup2(m_fdPreviousStdOut, _fileno(stdout)));
     }
     else
@@ -79,7 +78,6 @@ PipeOutputManager::StopOutputRedirection()
 
     // GetExitCodeThread returns 0 on failure; thread status code is invalid.
     if (m_hErrThread != NULL &&
-        m_hErrThread != INVALID_HANDLE_VALUE &&
         !LOG_LAST_ERROR_IF(GetExitCodeThread(m_hErrThread, &dwThreadStatus) == 0) &&
         dwThreadStatus == STILL_ACTIVE)
     {
@@ -96,8 +94,12 @@ PipeOutputManager::StopOutputRedirection()
         }
     }
 
-    CloseHandle(m_hErrThread);
-    m_hErrThread = NULL;
+    if (m_hErrThread != NULL)
+    {
+        CloseHandle(m_hErrThread);
+        m_hErrThread = NULL;
+    }
+
 
     if (m_hErrReadPipe != INVALID_HANDLE_VALUE)
     {
diff --git a/test/CommonLibTests/PipeOutputManagerTests.cpp b/test/CommonLibTests/PipeOutputManagerTests.cpp
index e15fa2868a..67ce9f2f04 100644
--- a/test/CommonLibTests/PipeOutputManagerTests.cpp
+++ b/test/CommonLibTests/PipeOutputManagerTests.cpp
@@ -32,6 +32,26 @@ namespace PipeOutputManagerTests
 
         pManager->NotifyStartupComplete();
 
+    }
+
+    TEST(PipeManagerOutputTest, SetInvalidHandlesForErrAndOut)
+    {
+        auto m_fdPreviousStdOut = _dup(_fileno(stdout));
+        auto m_fdPreviousStdErr = _dup(_fileno(stderr));
+
+        SetStdHandle(STD_ERROR_HANDLE, INVALID_HANDLE_VALUE);
+        SetStdHandle(STD_OUTPUT_HANDLE, INVALID_HANDLE_VALUE);
+
+        PCWSTR expected = L"test";
+
+        PipeOutputManager* pManager = new PipeOutputManager();
+        ASSERT_EQ(S_OK, pManager->Start());
+
+        pManager->NotifyStartupComplete();
+
+        _dup2(m_fdPreviousStdOut, _fileno(stdout));
+        _dup2(m_fdPreviousStdErr, _fileno(stderr));
+
         // Test will fail if we didn't redirect stdout back to a file descriptor.
         // This is because gtest relies on console output to know if a test succeeded or failed.
         // If the output still points to a file/pipe, the test (and all other tests after it) will fail.