ReadFile does not return while reading stdout from a child process after it ends

Question

I am working on my library which needs to capture and process the standard output (and err) of a child process as it runs. The problem arises when ReadFile is used to read the output, it does not return once the process ends (gets killed or exits).

It looks like ReadFile is not able to detect that the other end of the pipe (the write handle) is closed. According to the documentation it should return FALSE and set the last error to ERROR_BROKEN_PIPE:

If an anonymous pipe is being used and the write handle has been closed, when ReadFile attempts to read using the pipe's corresponding read handle, the function returns FALSE and GetLastError returns ERROR_BROKEN_PIPE.

Here is my code, I have stripped out the irrelevant bits: (NOTE: I have updated the allium_start to follow the suggested changes, I am keeping the original for reference, please use the newer function code to find flaws)

bool allium_start(struct TorInstance *instance, char *config, allium_pipe *output_pipes) {
    // Prepare startup info with appropriate information
    SecureZeroMemory(&instance->startup_info, sizeof instance->startup_info);
    instance->startup_info.dwFlags = STARTF_USESTDHANDLES;

    SECURITY_ATTRIBUTES pipe_secu_attribs = {sizeof(SECURITY_ATTRIBUTES), NULL, true};

    HANDLE pipes[2];
    if (output_pipes == NULL) {
        CreatePipe(&pipes[0], &pipes[1], &pipe_secu_attribs, 0);
        output_pipes = pipes;
    }
    instance->startup_info.hStdOutput = output_pipes[1];
    instance->startup_info.hStdError = output_pipes[1];
    instance->stdout_pipe = output_pipes[0]; // Stored for internal reference

    // Create the process
    bool success = CreateProcessA(
        NULL,
        cmd,
        NULL,
        NULL,
        config ? true : false,
        0,
        NULL,
        NULL,
        &instance->startup_info,
        SecureZeroMemory(&instance->process, sizeof instance->process)
    );

    // Return on failure
    if (!success) return false;
}

char *allium_read_stdout_line(struct TorInstance *instance) {
    char *buffer = instance->buffer.data;

    // Process the input
    unsigned int read_len = 0;
    while (true) {
        // Read data
        unsigned long bytes_read;
        if (ReadFile(instance->stdout_pipe, buffer, 1, &bytes_read, NULL) == false || bytes_read == 0) return NULL;

        // Check if we have reached end of line
        if (buffer[0] == '\n') break;

        // Proceed to the next character
        ++buffer; ++read_len;
    }

    // Terminate the new line with null character and return
    // Special handling for Windows, terminate at CR if present
    buffer[read_len >= 2 && buffer[-1] == '\r' ? -1 : 0] = '\0';

    return instance->buffer.data;
}

The allium_start creates the pipe for output redirection (it uses the same pipe for both stdout and stderr to get merged streams) and then creates the child process. The other allium_read_stdout_line function is responsible for reading the output from the pipe and returning it when it encounters a new line.

The issue occurs at the ReadFile function call, it never returns if there is nothing to read after the process exits, from my understanding all the handles of a process are closed by Windows when it ends, so it looks like ReadFile is not able to detect the fact that the pipe (write handle) at the other end has been closed.

How do I fix this? I have been searching for a solution but I have found none so far, one potential option is to use multi-threading and put ReadFile in a separate thread so that it doesn't block the whole program, by using that method I can check if the process still exists periodically while I wait for the reading to finish... or kill/stop the thread if the process is gone.

I do prefer fixing the issue instead of opting for a workaround, but I am open to any other solutions to make it work. Thanks in advance!

---

Edit: After reading @RemyLebeau's answer and @RbMm's comments in that answer, it is pretty clear that my understand of how handle inheritance works is fundamentally flawed. So I incorporated their suggestions (SetHandleInformation to disable inheritance of read handle and closing it after creating the child process) into my allium_start function:

bool allium_start(struct TorInstance *instance, char *config, allium_pipe *output_pipes) {
    // Prepare startup info with appropriate information
    SecureZeroMemory(&instance->startup_info, sizeof instance->startup_info);
    instance->startup_info.dwFlags = STARTF_USESTDHANDLES;

    SECURITY_ATTRIBUTES pipe_secu_attribs = {sizeof(SECURITY_ATTRIBUTES), NULL, true};

    HANDLE pipes[2];
    if (output_pipes == NULL) {
        CreatePipe(&pipes[0], &pipes[1], &pipe_secu_attribs, 0);
        output_pipes = pipes;
    }
    SetHandleInformation(output_pipes[0], HANDLE_FLAG_INHERIT, 0);
    instance->startup_info.hStdOutput = output_pipes[1];
    instance->startup_info.hStdError = output_pipes[1];
    instance->stdout_pipe = output_pipes[0]; // Stored for internal reference

    // Create the process
    bool success = CreateProcessA(
        NULL,
        cmd,
        NULL,
        NULL,
        config ? true : false,
        0,
        NULL,
        NULL,
        &instance->startup_info,
        SecureZeroMemory(&instance->process, sizeof instance->process)
    );

    // Close the write end of our stdout handle
    CloseHandle(output_pipes[1]);

    // Return on failure
    if (!success) return false;
}

(The below text was originally here before edit 2)

But sadly it still doesn't work :(

Edit 2 (after accepting answer): It does work! See my last comment on the accepted answer.

Answer 1

You are not managing your pipes correctly, or more specifically, you are not controlling the inheritance of your pipe handles. DO NOT let the child process inherit the reading handle of your pipe (output_pipes[0]), otherwise the pipe will not break correctly when the child process ends.

Read MSDN for more details:

Creating a Child Process with Redirected Input and Output

The case of the redirected standard handles that won’t close even though the child process has exited

Use SetHandleInformation() or PROC_THREAD_ATTRIBUTE_LIST to prevent CreateProcess() from passing output_pipes[0] to the child process as an inheritable handle. The child process does not need access to that handle, so there is no need to pass it over the process boundary anyway. It only needs access to the writing handle of your pipe (output_pipes[1]).

Answer 2

For anonymous pipelines, the read process and the write process will have the handler of hRead and hWrite, each of process has its own handler(copy after inheritance). So after your child process exit and close the handler in it, anther hWrite still in parent process. We must pay attention to close hRead in the write process, close hWrite in the read process.

I can reproduce this ReadFile issue, and if closing write handler after setting child's hStdOutput and hStdError, the ReadFile will return 0 after the child process exit.

Here is my code sample, Parent.cpp:

#include <windows.h> 
#include <iostream>
#include <stdio.h>

HANDLE childInRead = NULL;
HANDLE W1 = NULL;
HANDLE W2 = NULL;
HANDLE R2 = NULL;

HANDLE R1 = NULL;

#define BUFSIZE 4096

void CreateChildProcess() {
    TCHAR applicationName[] = TEXT("kids.exe");
    PROCESS_INFORMATION pi;
    STARTUPINFO si;
    BOOL success = FALSE;

    ZeroMemory(&pi, sizeof(PROCESS_INFORMATION));
    ZeroMemory(&si, sizeof(STARTUPINFO));

    si.cb = sizeof(STARTUPINFO);
    si.hStdError = W1;
    si.hStdOutput = W1;
    si.hStdInput = R2;
    si.dwFlags |= STARTF_USESTDHANDLES;

    success = CreateProcess(NULL, applicationName, NULL, NULL, TRUE, CREATE_NEW_CONSOLE, NULL, NULL, &si, &pi);

    if (!success) {
        printf("Error creating child process \n");
    }
    else {
        printf("Child process successfuly created \n");
        CloseHandle(pi.hProcess);
        CloseHandle(pi.hThread);
    }
}

int main()
{
    printf("Parent process running.... \n");

    DWORD dRead, dWritten;
    CHAR chBuf[BUFSIZE] = { 0 };
    BOOL bSuccess = FALSE;

    SECURITY_ATTRIBUTES secAttr;
    secAttr.nLength = sizeof(SECURITY_ATTRIBUTES);
    secAttr.bInheritHandle = TRUE;
    secAttr.lpSecurityDescriptor = NULL;

    printf("Creating first pipe \n");

    if (!CreatePipe(&R1, &W1, &secAttr, 0)) {
        printf("\n error creating first pipe \n");
    }
    printf("Creating second pipe \n");

    if (!CreatePipe(&R2, &W2, &secAttr, 0)) {
        printf("\n error creating second pipe \n");
    }

    if (!SetHandleInformation(R1, HANDLE_FLAG_INHERIT, 0)) {
        printf("\n R1 SetHandleInformation \n");
    }
    if (!SetHandleInformation(W2, HANDLE_FLAG_INHERIT, 0)) {
        printf("\n W1 SetHandleInformation \n");
    }

    printf("\n Creating child process..... \n");

    HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
    HANDLE hStdIn = GetStdHandle(STD_INPUT_HANDLE);
    CreateChildProcess();
    CloseHandle(W1);
    CloseHandle(R2);
    for (;;) {
        printf("Inside for loop \n");

        //1. read from stdin
        printf("read from stdin:\n");
        bSuccess = ReadFile(hStdIn, chBuf, BUFSIZE, &dRead, NULL);
        if (!bSuccess) {
            printf("error reading \n");
            break;
        }


        //2. write to Pipe2
        printf("write to Pipe2...\n");
        bSuccess = WriteFile(W2, chBuf, 100, &dWritten, NULL);
        if (!bSuccess) {
            printf("error reading \n");
            break;
        }

        //3. read from Pipe1
        printf("read from Pipe1...\n");
        bSuccess = ReadFile(R1, chBuf, BUFSIZE, &dRead, NULL);
        if (!bSuccess)
        {
            printf("error reading :%d \n", GetLastError());
            break;
        }

        //4. write to stdout
        printf("write to stdout:\n");
        bSuccess = WriteFile(hStdOut, chBuf, 100, &dWritten, NULL);
        if (!bSuccess) {
            printf("error reading \n");
            break;
        }
    }
    getchar();
    return 0;
}

Kids.cpp:

#include <windows.h>
#include <stdio.h>

#define BUFSIZE 4096

int main()
{
    DWORD dRead, dWritten;
    CHAR chBuf[BUFSIZE];
    BOOL success = FALSE;
    HANDLE stdIn = GetStdHandle(STD_INPUT_HANDLE);
    HANDLE stdOut = GetStdHandle(STD_OUTPUT_HANDLE);

    printf("Child process running....");

    if (stdIn == INVALID_HANDLE_VALUE || stdOut == INVALID_HANDLE_VALUE) {
        ExitProcess(1);
    }

    //for (;;) {
        success = ReadFile(stdIn, chBuf, BUFSIZE, &dRead, NULL);
        //if (!success || dRead == 0) break;

        success = WriteFile(stdOut, chBuf, dRead, &dWritten, NULL);
        //if (!success) break;
    //}
    return 0;
}