Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly pass stdio handles to child process #3

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

cqjjjzr
Copy link

@cqjjjzr cqjjjzr commented Mar 30, 2022

See ScoopInstaller/Scoop#4849 and restic/restic#3681

When a process (A) spawns a process (B) with DETACHED_PROCESS and communicate with it via stdio redirections, process B isn't attached to a console, but its STD_XXX_HANDLEs are still valid (set by process A). However, with shim in the middle, such communication is broken, because for unknown reason the stdio handles are not properly passed down.

This PR fixes such problem by manually passing down handles via STARTUPINFOW. However during my test, although restic/rclone succeeded to work, a blank console window still popped up. It may be suppressed via CREATE_NO_WINDOW creation flag, but I don't know if it would break other things..

Code I used to test:

Code for process A (golang, expecting the child/shim executable name = testtarget):

package main

import "fmt"
import "os"
import "os/exec"
import "bufio"
import "syscall"
import "golang.org/x/sys/windows"

func main() {
	cmd := exec.Command("testtarget")

	r, stdin, err := os.Pipe()
	if err != nil {
		fmt.Println("Failed to open read pipe", err)
	}

	stdout, w, err := os.Pipe()
	if err != nil {
		// close first pipe and ignore subsequent errors
		_ = r.Close()
		_ = stdin.Close()
		fmt.Println("Failed to open write pipe", err)
	}
	cmd.Stdin = r
	cmd.Stdout = w

	cmd.SysProcAttr = &syscall.SysProcAttr{}
	cmd.SysProcAttr.CreationFlags = windows.DETACHED_PROCESS
	err = cmd.Start()

	if err != nil {
		fmt.Println("Failed to start proc. ", err)
	}

	stdoutBuf := bufio.NewReader(stdout)
	stdinBuf := bufio.NewWriter(stdin)
	for {
		line, err := stdoutBuf.ReadString('\n')
		if err != nil {
			break
		}

		fmt.Println("from subproc: ", line)
		stdinBuf.WriteString("response\n")
		stdinBuf.Flush()
	}

	cmd.Wait()
	fmt.Println("exitcode: ", cmd.ProcessState.ExitCode());
	r.Close()
	w.Close()
	stdin.Close()
	stdout.Close()
}

Code for process B (C++):

#include <Windows.h>
#include <sstream>
#include <iomanip>

int main() {
    auto hin = GetStdHandle(STD_INPUT_HANDLE);
    auto hout = GetStdHandle(STD_OUTPUT_HANDLE);

    DWORD tlen = 0;
    DWORD ilen = 100;
    char inbuf[100];

    auto sstream = std::ostringstream();
    sstream<<std::hex<<(uintptr_t)(hin)<<std::endl;
    auto str = sstream.str();

    if (!WriteFile(hout, str.c_str(), str.size(), &tlen, nullptr))
        return -1;


    if (!ReadFile(hin, inbuf, ilen, &tlen, nullptr))
        return -1;

    sstream = std::ostringstream();
    sstream<<std::hex<<(uintptr_t)(hout)<<std::endl;
    str = sstream.str();

    if (!WriteFile(hout, str.c_str(), str.size(), &tlen, nullptr))
        return -1;


    if (!ReadFile(hin, inbuf, ilen, &tlen, nullptr))
        return -1;

    while(true);

    return 0;
}

shim.cpp Outdated
@@ -124,6 +124,15 @@ std::tuple<std::unique_handle, std::unique_handle> MakeProcess(const std::wstrin
std::unique_handle threadHandle;
std::unique_handle processHandle;

si.cb = sizeof(STARTUPINFOW);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we can just forward all of the start info of shim process to its child?
Using something like https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getstartupinfow.
WDYT?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, I'll give it a try later.

Copy link
Author

@cqjjjzr cqjjjzr Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, but maybe we need more tests to see if it will break other things (GUI apps, etc..)

all si.xxxx assignments will become a single GetStartupInfoW(&si);.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick test shows that GUI apps and console apps with complicated behavior (REPLs like powershell (+PSReadLine), python) works well. However, it seems that the check for GUI apps, therefore the console window didn't exit when the GUI is shown, but it's not related to the GetStartupInfoW change though

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, please update your PR once you feel confident about it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the new approach is pushed

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants