Execute: ensure pipes are read (#10)

johnwparent · web-flow · commit 091b1a7e455b · 2025-07-28T14:37:39.000-07:00
Prior to this PR there were two bugs:

One: we default initialized a boolean to indicate whether we were
piping child process output to a file or the wrapper's stdout.
Default-initializing this causes it to have undefined behavior, as
either true or false, which means the reader on the child process'
stdout/stderr pipes will spuriously close as they attempt to write
to a nonexistent file.

Two: The readers for the child process' stdout/stderr pipes can
potentially exit early if the child process hangs for a bit without
output.

This PR resolves both by:

- Adding an initialization value to the file check (false)
- Ensuring the child process terminates before either of the
  stdout/stderr reader threads
- Ensuring the readers do not exit before the Execute class has
  indicated the child has termianted

This PR adds a test that:

- Dumps 64k bytes of output to stdout
- expects the compiler wrapper to handle that and exit w/ success
  without hanging
diff --git a/Makefile b/Makefile
@@ -108,11 +108,28 @@ test_relocate_dll: build_and_check_test_sample
 	del tester.exe
 	link main.obj ..\tmp_lib\calc.lib /out:tester.exe
 	.\tester.exe
+	cd ../..
+
+test_pipe_overflow: build_and_check_test_sample
+	set SPACK_CC_TMP=%SPACK_CC%
+	set SPACK_CC=$(MAKEDIR)\test\lots-of-output.bat
+	cl /c /EHsc "test\src file\calc.cxx"
+	set SPACK_CC=%SPACK_CC_TMP%
+
+build_zerowrite_test: test\writezero.obj
+	link $(LFLAGS) $** Shlwapi.lib /out:writezero.exe
+
+
+test_zerowrite: build_zerowrite_test
+	set SPACK_CC_TMP=%SPACK_CC%
+	set SPACK_CC=$(MAKEDIR)\writezero.exe
+	cl /c EHsc "test\src file\calc.cxx"
+	set SPACK_CC=%SPACK_CC_TMP%
 
 test_and_cleanup: test clean-test
 
 
-test: test_wrapper test_relocate_exe test_relocate_dll
+test: test_wrapper test_relocate_exe test_relocate_dll test_pipe_overflow
 
 
 clean : clean-test clean-cl
@@ -128,4 +145,5 @@ clean-cl :
 	del cl.exe
 
 clean-test:
-	rmdir /q /s tmp
+	-@ if EXIST "tmp" rmdir /q /s "tmp"
+	
diff --git a/src/execute.cxx b/src/execute.cxx
@@ -161,7 +161,7 @@ int ExecuteCommand::PipeChildToStdErr()
     CHAR chBuf[BUFSIZE];
     BOOL bSuccess = TRUE;
     HANDLE hParentOut;
-    if (this->write_to_file) {
+    if (this->write_to_file && this->fileout != INVALID_HANDLE_VALUE) {
         hParentOut = this->fileout;
     }
     else {
@@ -171,10 +171,35 @@ int ExecuteCommand::PipeChildToStdErr()
     for (;;)
     {
         bSuccess = ReadFile( this->ChildStdErr_Rd, chBuf, BUFSIZE, &dwRead, NULL);
-        if( ! bSuccess || dwRead == 0 ) break;
-
-        bSuccess = WriteFile(hParentOut, chBuf,
-                            dwRead, &dwWritten, NULL);
+        // For an explanation behind the use of termianted here
+        // see the docstring on pipechildtostdout
+        if( ! bSuccess || (dwRead == 0 && this->terminated) ) break;
+        if(dwRead != 0) {
+            bSuccess = WriteFile(hParentOut, chBuf,
+                                dwRead, &dwWritten, NULL);
+            if (dwWritten < dwRead && bSuccess){
+                // incomplete write but not a failure
+                // since bSuccess is true
+                // So lets write until bSuccess is false or
+                // until all bytes are written
+                int currentPos = dwWritten;
+                while((dwWritten < dwRead) || dwWritten == 0) {
+                    dwRead = dwRead - dwWritten;
+                    CHAR * partialBuf = new CHAR[dwRead];
+                    for(int i = 0; i < dwRead; ++i) {
+                        partialBuf[i] = chBuf[currentPos + i];
+                    }
+                    bSuccess = WriteFile(hParentOut, partialBuf,
+                                        dwRead, &dwWritten, NULL);
+                    delete partialBuf;
+                    if (! bSuccess) break;
+                    currentPos += dwWritten;
+                }
+            }
+            if (! bSuccess ) {
+                break;
+            }
+        }
         if (! bSuccess ) break;
     }
     return !bSuccess;
@@ -192,7 +217,7 @@ int ExecuteCommand::PipeChildToStdout()
     CHAR chBuf[BUFSIZE];
     BOOL bSuccess = TRUE;
     HANDLE hParentOut;
-    if (this->write_to_file) {
+    if (this->write_to_file && this->fileout != INVALID_HANDLE_VALUE) {
         hParentOut = this->fileout;
     }
     else {
@@ -202,10 +227,41 @@ int ExecuteCommand::PipeChildToStdout()
     for (;;)
     {
         bSuccess = ReadFile( this->ChildStdOut_Rd, chBuf, BUFSIZE, &dwRead, NULL);
-        if( ! bSuccess || dwRead == 0 ) break;
-
-        bSuccess = WriteFile(hParentOut, chBuf,
-                            dwRead, &dwWritten, NULL);
+        // Typically dwRead == 0 indicates the writer end of the pipe has ceased writing
+        // however if the writer were to invoke WriteFile with a size of 0, dwRead would
+        // be 0 but the writer would not have terminated. 
+        // As such we need an explicit indication the writer process has termianted.
+        // From the MSVC docs:
+        // If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a pipe,
+        // the other end of the pipe called the WriteFile function with nNumberOfBytesToWrite 
+        // set to zero.
+        if( ! bSuccess || (dwRead == 0 && this->terminated) ) break;
+        if(dwRead != 0){
+            bSuccess = WriteFile(hParentOut, chBuf,
+                                dwRead, &dwWritten, NULL);
+            if (dwWritten < dwRead && bSuccess){
+                // incomplete write but not a failure
+                // since bSuccess is true
+                // So lets write until bSuccess is false or
+                // until all bytes are written
+                int currentPos = dwWritten;
+                while((dwWritten < dwRead) || dwWritten == 0) {
+                    dwRead = dwRead - dwWritten;
+                    CHAR * partialBuf = new CHAR[dwRead];
+                    for(int i = 0; i < dwRead; ++i) {
+                        partialBuf[i] = chBuf[currentPos + i];
+                    }
+                    bSuccess = WriteFile(hParentOut, partialBuf,
+                                        dwRead, &dwWritten, NULL);
+                    delete partialBuf;
+                    if (! bSuccess) break;
+                    currentPos += dwWritten;
+                }
+            }
+            if (! bSuccess ) {
+                break;
+            }
+        }
         if (! bSuccess ) break;
     }
     return !bSuccess;
@@ -242,6 +298,7 @@ int ExecuteCommand::ReportExitCode()
         if(exit_code != STILL_ACTIVE)
             break;
     }
+    this->terminated = true;
     CloseHandle(this->procInfo.hProcess);
     return exit_code;
 }
@@ -294,9 +351,16 @@ bool ExecuteCommand::Execute(const std::string &filename)
  */
 int ExecuteCommand::Join()
 {
+    // Join primary thread first
+    // This process sets the termianted flag
+    // without which the reader threads will not
+    // terminate, so the primary thread must be
+    // joined first so we have a guaruntee that the
+    // reader processes can exit
+    int commandError = this->exit_code_future.get();
     if(!this->child_out_future.get())
         return -999;
     if(!this->child_err_future.get())
         return -999;
-    return this->exit_code_future.get();
+    return commandError;
 }
diff --git a/src/execute.h b/src/execute.h
@@ -61,8 +61,9 @@ class ExecuteCommand {
     SECURITY_ATTRIBUTES saAttr;
     SECURITY_ATTRIBUTES saAttrErr;
     HANDLE fileout = INVALID_HANDLE_VALUE;
-    bool write_to_file;
+    bool write_to_file = false;
     bool cpw_initalization_failure = false;
+    bool terminated = false;
     std::string base_command;
     StrList command_args;
 };
diff --git a/test/lots-of-output.bat b/test/lots-of-output.bat
@@ -0,0 +1,3 @@
+@echo OFF
+
+for /l %%x in (1, 1, 1250) do echo Test boilerplate output to fill stdout line number %%x
diff --git a/test/writezero/writezero.cxx b/test/writezero/writezero.cxx
@@ -0,0 +1,14 @@
+#include <iostream>
+#include <string>
+#include <windows.h>
+#include <tchar.h>
+#include <stdio.h>
+#include <strsafe.h>
+
+#define BUFSIZE 100
+
+int main(int argc, char ** argv) {
+    HANDLE stdOut = GetStdHandle(STD_OUTPUT_HANDLE);
+    char buff[BUFSIZE];
+    WriteFile(stdOut, buff, 0, NULL, NULL);
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+@echo OFF`
	`2`	`+`
	`3`	`+for /l %%x in (1, 1, 1250) do echo Test boilerplate output to fill stdout line number %%x`