Use 'ShellExecute' instead of 'wsystem' in 'runCommand' for Windows. #15909

pull darosior wants to merge 1 commits into bitcoin:master from darosior:windows_blocknotify_blink changing 1 files +14 −3
  1. darosior commented at 2:21 PM on April 27, 2019: member

    As pointed out by #15883, the use of blocknotify or walletnotify on Windows caused a flickering command window at each call.

    This PR substitutes the use of wsystem in runCommand with createProcess, using the CREATE_NO_WINDOW flag to avoid this behavior.

  2. fanquake added the label Utils/log/libs on Apr 27, 2019
  3. darosior force-pushed on Apr 27, 2019
  4. darosior force-pushed on Apr 27, 2019
  5. promag commented at 5:12 PM on April 27, 2019: member

    @MarcoFalke can we have a gitian build?

  6. promag commented at 5:13 PM on April 27, 2019: member

    Have you considered using boost process?

  7. Sjors commented at 6:41 PM on April 27, 2019: member

    If you decide to use boost process, consider building on top of #15382 (minus the last commit). That said, this change is much smaller (though I have no idea if it's correct).

  8. MarcoFalke added the label Needs gitian build on Apr 27, 2019
  9. MarcoFalke commented at 9:10 PM on April 27, 2019: member 
    c:\projects\bitcoin\src\util\system.cpp(1140): error C2664: 'BOOL CreateProcessW(LPCWSTR,LPWSTR,LPSECURITY_ATTRIBUTES,LPSECURITY_ATTRIBUTES,BOOL,DWORD,LPVOID,LPCWSTR,LPSTARTUPINFOW,LPPROCESS_INFORMATION)': cannot convert argument 2 from 'char *' to 'LPWSTR
  10. darosior force-pushed on Apr 28, 2019
  11. darosior force-pushed on Apr 28, 2019
  12. darosior commented at 10:23 AM on April 28, 2019: member 
    c:\projects\bitcoin\src\util\system.cpp(1140): error C2664: 'BOOL CreateProcessW(LPCWSTR,LPWSTR,LPSECURITY_ATTRIBUTES,LPSECURITY_ATTRIBUTES,BOOL,DWORD,LPVOID,LPCWSTR,LPSTARTUPINFOW,LPPROCESS_INFORMATION)': cannot convert argument 2 from 'char *' to 'LPWSTR

    Fixed it by explicitly using CreateProcessA (I used CreateProcess that was interpreted as CreateProcessA during my tests and as CreateProcessW by appveyor).

    Have you considered using boost process?

    If you decide to use boost process, consider building on top of #15382 (minus the last commit). That said, this change is much smaller (though I have no idea if it's correct).

    Is it a good idea to add code that use boost since, as I've read here, we aim to remove Boost as a dependency ?

  13. darosior force-pushed on Apr 28, 2019
  14. Sjors commented at 11:54 AM on April 28, 2019: member

    The idea is to move from Boost to modern C++, where it's often the case that new C++ standards adopt things from Boost. So the hope is the boost::process can eventually easily be replaced with something in C++42 :-)

  15. darosior force-pushed on Apr 28, 2019
  16. darosior force-pushed on Apr 28, 2019
  17. laanwj commented at 6:24 PM on April 30, 2019: member

    This change was rejected before in #5834—not sure the same reasoning, or problems exist with this PR, but have a look.

  18. in src/util/system.cpp:1134 in 679f16a940 outdated 
    1146 | +    } else {
1147 | +        nErr = GetLastError();
1148 | +    }
1149 |  #endif
1150 |      if (nErr)
1151 |          LogPrintf("runCommand error: system(%s) returned %d\n", strCommand, nErr);
    luke-jr commented at 6:26 AM on May 1, 2019:

    This message is explicit about the call being system, so should probably be made ambiguous

  19. in src/util/system.cpp:1138 in 679f16a940 outdated 
    1135 |  #else
1136 | -    int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().from_bytes(strCommand).c_str());
1137 | +    STARTUPINFOA info;
1138 | +    PROCESS_INFORMATION processInfo;
1139 | +    ZeroMemory(&info, sizeof(info));
1140 | +    info.cb = sizeof(info);
    luke-jr commented at 6:28 AM on May 1, 2019:

    Why not just initialise info with {.cb = sizeof(info)} above?

  20. in src/util/system.cpp:1139 in 679f16a940 outdated 
    1136 | -    int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().from_bytes(strCommand).c_str());
1137 | +    STARTUPINFOA info;
1138 | +    PROCESS_INFORMATION processInfo;
1139 | +    ZeroMemory(&info, sizeof(info));
1140 | +    info.cb = sizeof(info);
1141 | +    ZeroMemory(&processInfo, sizeof(processInfo));
    luke-jr commented at 6:30 AM on May 1, 2019:

    Is there a need to zero this?

    darosior commented at 6:14 PM on May 1, 2019:

    I had never used the windows API before this, so I sticked to the documentation example : https://docs.microsoft.com/en-us/windows/desktop/ProcThread/creating-processes

  21. in src/util/system.cpp:1140 in 679f16a940 outdated 
    1137 | +    STARTUPINFOA info;
1138 | +    PROCESS_INFORMATION processInfo;
1139 | +    ZeroMemory(&info, sizeof(info));
1140 | +    info.cb = sizeof(info);
1141 | +    ZeroMemory(&processInfo, sizeof(processInfo));
1142 | +    if (CreateProcessA(NULL, const_cast<char *>(strCommand.c_str()), NULL, NULL, FALSE, 0x08000000, NULL, NULL, &info, &processInfo)) {
    luke-jr commented at 6:32 AM on May 1, 2019:

    CreateProcessA seems to execute the program directly, rather than passing through the shell as expected for -*notify. This may break existing usage, especially because .bat files will fail.

    darosior commented at 7:12 PM on May 1, 2019:

    Indeed, a workaround is to put cmd.exe at the command beginning as in #5834, but I don't know if this is a good way to do it..

  22. in src/util/system.cpp:1143 in 679f16a940 outdated 
    1140 | +    info.cb = sizeof(info);
1141 | +    ZeroMemory(&processInfo, sizeof(processInfo));
1142 | +    if (CreateProcessA(NULL, const_cast<char *>(strCommand.c_str()), NULL, NULL, FALSE, 0x08000000, NULL, NULL, &info, &processInfo)) {
1143 | +        WaitForSingleObject(processInfo.hProcess, INFINITE);
1144 | +        CloseHandle(processInfo.hProcess);
1145 | +        CloseHandle(processInfo.hThread);
    luke-jr commented at 6:34 AM on May 1, 2019:

    Where is nErr set to the process's return value?

    darosior commented at 7:13 PM on May 1, 2019:

    nErr is set only if the command did not succeed.

    luke-jr commented at 12:37 AM on May 2, 2019:

    nErr should always be set to the exit code of the process.

    darosior commented at 10:46 AM on May 2, 2019:

    I could have set nErr to GetLastError() outside of the else block but I've read from the doc.aspx) that a windows process would not always return 0 on success : "some functions set the last-error code to 0 on success and others do not." This means that if (nErr) could pass and log as an error on windows even if the called process executed successfully. I can change it (along with the log message as asked above) but I'll first give a look at using ShellExecute as Sipa proposed in #5834.

  23. luke-jr changes_requested
  24. luke-jr commented at 6:35 AM on May 1, 2019: member

    I'm concerned that this may cause output from executed commands to be lost when the user is running a non-daemon bitcoind. Can anyone test?

  25. luke-jr commented at 6:36 AM on May 1, 2019: member

    re boost::process, note that early versions had bugs which IIRC would likely break this use case.

  26. darosior force-pushed on May 2, 2019
  27. darosior commented at 12:32 PM on May 2, 2019: member

    @luke-jr thank you for the review. I finally opted for the ShellExecute function to handle batch files, as proposed by Sipa in #5834 (thank you @laanwj for pointing me to this PR), and to reduce diff. I also changed the Log message and did not set nErr to the process exit code because ShellExecute can return 0 on error.

  28. darosior renamed this:
    Use 'CreateProcess' instead of 'wsystem' in 'runCommand' for Windows.
    Use 'ShellExecute' instead of 'wsystem' in 'runCommand' for Windows.
    on May 2, 2019
  29. in src/util/system.cpp:1137 in 6f2d4de939 outdated 
    1135 | -#endif
1136 |      if (nErr)
1137 |          LogPrintf("runCommand error: system(%s) returned %d\n", strCommand, nErr);
1138 | +#else
1139 | +    // Second argument "verb" (=action to be performed) set to NULL results in action "open"
1140 | +    if (!ShellExecute(NULL, NULL, strCommand.c_str(), NULL, NULL, SW_HIDE))
    luke-jr commented at 12:47 PM on May 2, 2019:

    Does this actually work at all? From the docs I would expect it doesn't.

    darosior commented at 12:53 PM on May 2, 2019:

    Yes it does, I tested it before comitting.

    darosior commented at 12:56 PM on May 2, 2019:

    Actually you are right about the condition, I messed up the return values of ShellExecuteEx (boolean) and ShellExecute (HINSTANCE). I am going to fix it right now.

  30. NicolasDorier commented at 1:09 PM on May 2, 2019: contributor

    You might need to call CreateProcessW or CreateProcess instead ping @ken2812221 . I am unsure.

  31. darosior force-pushed on May 2, 2019
  32. DrahtBot commented at 3:41 PM on May 2, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  33. Use 'ShellExecuteEx' instead of 'wsystem' in 'runCommand' for Windows
    'blocknotify' or 'walletnotify' on Windows caused a cmd window to popup. The use of 'ShellExecute' with the 'SW_HIDE' flag instead of 'wsystem' avoid this behaviour
    3ba43e4209
  34. darosior force-pushed on May 2, 2019
  35. DrahtBot commented at 4:41 AM on May 4, 2019: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit 24dfcf3a56f90b101bc208f48ccdb7813fa08b83 (master):

    Gitian builds for commit b9fdef7200a577d876567c53aa7b04519f98a9da (master and this pull):

  36. DrahtBot removed the label Needs gitian build on May 4, 2019
  37. fanquake added the label Windows on Jul 30, 2019
  38. fanquake commented at 5:52 AM on July 30, 2019: member

    @ken2812221 Is not currently available. So maybe we can defer to @sipsorcery for a Concept ACK / NACK.

  39. sipsorcery commented at 6:07 AM on July 30, 2019: member

    @darosior if the command can ever contain unicode characters then SHELLEXECUTEINFOW should be used instead of SHELLEXECUTEINFOA. @ken2812221's work added some big improvements to the bitcoin core software unicode handling on Windows. The datadir and other paths should be treated as potentially containing unicode characters.

  40. darosior commented at 1:59 PM on August 3, 2019: member

    Thank you for the help with the Windows version which I struggled to test. I'll revisit this PR soon.

  41. in src/util/system.cpp:1145 in 3ba43e4209 
    1143 | +    ShExecInfo.lpVerb = NULL; // == action "open"
1144 | +    ShExecInfo.lpFile = strCommand.c_str();
1145 | +    ShExecInfo.lpParameters = NULL;
1146 | +    ShExecInfo.lpDirectory = NULL;
1147 | +    ShExecInfo.nShow = SW_HIDE;
1148 | +    ShExecInfo.hInstApp = NULL;
    luke-jr commented at 1:26 AM on September 24, 2019: 
        SHELLEXECUTEINFOA ShExecInfo = {
        .cbSize = sizeof(SHELLEXECUTEINFOA),
        .lpFile = strCommand.c_str(),
        .nShow = SW_HIDE,
    };
  42. in src/util/system.cpp:1144 in 3ba43e4209 
    1142 | +    ShExecInfo.hwnd = NULL;
1143 | +    ShExecInfo.lpVerb = NULL; // == action "open"
1144 | +    ShExecInfo.lpFile = strCommand.c_str();
1145 | +    ShExecInfo.lpParameters = NULL;
1146 | +    ShExecInfo.lpDirectory = NULL;
1147 | +    ShExecInfo.nShow = SW_HIDE;
    luke-jr commented at 1:26 AM on September 24, 2019:

    It shouldn't be hidden if there is already a console available (eg, if bitcoind is being used in a command shell)

  43. in src/util/system.cpp:1146 in 3ba43e4209 
    1144 | +    ShExecInfo.lpFile = strCommand.c_str();
1145 | +    ShExecInfo.lpParameters = NULL;
1146 | +    ShExecInfo.lpDirectory = NULL;
1147 | +    ShExecInfo.nShow = SW_HIDE;
1148 | +    ShExecInfo.hInstApp = NULL;
1149 | +    if (!ShellExecuteExA(&ShExecInfo))
    luke-jr commented at 1:26 AM on September 24, 2019:

    Needs braces

  44. luke-jr changes_requested
  45. darosior commented at 12:04 PM on September 24, 2019: member

    @luke-jr thank you for the time you spent reviewing this again. However I can't test this PR since I can't access a Windows machine and the appveyor testing is really not viable.

    I'm sorry for people experiencing #15883 but I shouldn't have done a Windows PR in the first place (maybe someone can grab and correct this if it's not too wrong) ... Closing.

  46. darosior closed this on Sep 24, 2019
  47. MarcoFalke locked this on Dec 16, 2021
Contributors
darosiorpromagSjorsMarcoFalkelaanwjluke-jrNicolasDorierDrahtBotfanquakesipsorcery
Labels
WindowsUtils/log/libs
Linked (view graph)
#15883 Bug related to blocknotify/walletnotify with Bitcoin Core on Windows.
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me