On Windows, double check to avoid calling CloseHandle on not initiali…

[marcosbracco]

…zed handle, when CreateProcess fails.

Usually not an issue, but when on DEBUG mode, an exception is raised.

When CreateProcess fails, stdin_fd, stdout_fd and stderr_fd are left owning a not initialized fd_type. Later, clean up, they are evaluated, and being true it assumes they must be closed.

[clunietp]

I just ran across the same issue, Win64/MSVC 14. My fix was to call 'stdXXX_fd.reset' in the 'open' method of process_win.cpp if CreateProcess failed:

if(!bSuccess) {

       CloseHandle(process_info.hProcess);

       CloseHandle(process_info.hThread);

    if ( stdin_fd ) { CloseHandle( stdin_rd_p ); CloseHandle( stdin_wr_p ); stdin_fd.reset(); }  // <-added 'reset' here, and below

    if ( stdout_fd ) { CloseHandle( stdout_rd_p ); CloseHandle( stdout_wr_p ); stdout_fd.reset(); }

    if ( stderr_fd ) { CloseHandle( stderr_rd_p ); CloseHandle( stderr_wr_p ); stderr_fd.reset(); }

[eidheim]

@trent33 Thank you as well for looking into this. If I'm not mistaken, this PR, after the suggested changes, will fix your issue as well. Will merge this Tomorrow most likely.

[marcosbracco]

I just pushed update change. One comment by the way. On my Windows without MSYS, tests don't run. In example, to make open_close_test to run, I needed to change the command string, and the expected result string:

 Process process("C:\\WINDOWS\\system32\\cmd.exe /C echo Hello World "+std::to_string(c), "", 
[&stdout_error, c](const char *bytes, size_t n) {
      if(std::string(bytes, n)!="Hello World "+std::to_string(c)+"\r\n")

I can change them if you like,

Nice library by the way, very useful.

[clunietp]

Hi Marcos, does this patch fix the issue for you? I'm curious because unique_ptr:: operator bool() is logically the same as *unique_ptr != NULL. So, in essence, we're checking the same thing twice, but expecting a different result. I don't have my dev machine to triple check this, but I'm pretty sure i tried this fix earlier and it wasn't successful. Would be interested in your thoughts, thanks. -Tom

[marcosbracco]

Hi there, I neither have my dev machine right now, I will triple check on monday, but I don't think they are the same thing. operator bool will return true if there is a managed pointer, while operator * will return the value of the managed pointer. from cppreference, unique_ptr::operator bool equivalent to get() != nullptr unique_ptr::opeartor * equivalent to *get()

[eidheim]

The use of unique_ptr to store handles/file descriptors is done mostly because of the unix implementation where an int pointer is stored. I guess using a smart pointer to store a pointer to a pointer (HANDLE is equal to void*) is a bit uncommon.

@marcosbracco I actually have never tested this library outside of MSYS2 on Windows, does the examples run? Feel free to update the tests using some of the same define if statements as found in the examples, so the tests continue to work on Unix-like systems.

The unix-implementation also had similar problems when process open fails. It's fixed now though.