subprocess: fix command line memory leak on Windows #36581

1. 
   stickies-v commented at 10:04 AM on October 20, 2026: contributor

   Popen::execute_process() copied the command line into a buffer from new[], but did not delete it. This leaked memory each time a process started on Windows.

   CreateProcessA needs a writable command line buffer. Since C++17, std::string::data() supplies one, so give the std::string buffer directly and remove the copy.

   This only affects Windows. It is only reachable if -signer=<cmd> is set, through authenticated RPC or the GUI. Each call leaks only the startup-defined <cmd>, and the rate is limited by process spawn time, so the impact is limited.

   edit: this was fixed upstream in https://github.com/arun11299/cpp-subprocess/pull/131, but since this codebase is C++20 it seems better to avoid the copy (and the const cast). A direct backport isn't possible anyway, since #35704 switched us to CreateProcessA.

2. 
   DrahtBot commented at 10:04 AM on October 20, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36581
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36581.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36581#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	hebasto, winterrdog, jpk68
   Stale ACK	hodlinator

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

3. maflcko added the label Windows on Oct 20, 2026
4. maflcko added the label Utils/log/libs on Oct 20, 2026
5. 
   hebasto commented at 10:35 AM on October 20, 2026: member

   Concept ACK.

   Thank you!

6. in src/util/subprocess.h:1121 in fe6608590c

   1117 | @@ -1121,7 +1118,7 @@ inline void Popen::execute_process() noexcept(false)
   1118 |  
   1119 |    // Create the child process.
   1120 |    bSuccess = CreateProcessA(NULL,
   1121 | -                            szCmdline,    // command line
   1122 | +                            command_line.data(), // command line; must be writable (LPSTR)
   

   ---

   ---

   

   hodlinator commented at 1:54 PM on October 21, 2026:

   https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa states:

   The system adds a terminating null character to the command-line string to separate the file name from the arguments. This divides the original string into two strings for internal processing.

   But that should be fine as I interpret it as the replacement of a char-value, and this is the last place command_line is used.

   ---

   

   hebasto commented at 1:16 PM on October 22, 2026:

   The docs only state that CreateProcessW can modify the string. They don’t say either way for CreateProcessA.

   So maybe no need to amend the code comment?

   ---

   

   hodlinator commented at 1:22 PM on October 22, 2026:

   Ah, it's further up:

   The Unicode version of this function, CreateProcessW, can modify the contents of this string. Therefore, this parameter cannot be a pointer to read-only memory (such as a const variable or a literal string). If this parameter is a constant string, the function may cause an access violation.

   ---

   

   stickies-v commented at 2:40 PM on October 22, 2026:

   CreateProcessA requires a mutable string (LPSTR) as part of its function signature, and it seems like the implementation then passes a copy to CreateProcessW. But I agree that perhaps the docstring change is confusing, CreateProcessA's function signature is enough, what it does under the hood is not our concern. Force-pushed.

7. hodlinator approved
8. 
   hodlinator commented at 1:56 PM on October 21, 2026: contributor

   ACK fe6608590c624042fa4a77a6c9f3a46b70bc646c

9. DrahtBot requested review from hebasto on Oct 21, 2026
10. 
    jpk68 commented at 11:55 PM on October 21, 2026: contributor

    ACK cf30d0a4123bb4237495f04e723ce9f706c2edf2

11. 
    sedited commented at 7:56 AM on October 22, 2026: contributor

    Has this patch been checked on a windows box? I find the null terminator behaviour hard to reason about.

12. 
    maflcko commented at 8:07 AM on October 22, 2026: member

    You mean outside the Windows CI, which covers rpc_singer.py?

    https://github.com/bitcoin/bitcoin/actions/runs/39405290402/job/118569619457?pr=36581#step:14:1185 and https://github.com/bitcoin/bitcoin/actions/runs/39405290402/job/118571318163?pr=36581#step:11:418

    Maybe there could be a Windows /fsanitize=address nightly build with leak sanitizer enabled as well?

13. 
    hodlinator commented at 8:48 AM on October 22, 2026: contributor

    Took it for a spin in a Windows debug build now (had not previously done so).

    Overriding through:

    command_line = "C:\\Users\\hodlinator\\bitcoin\\build\\bin\\Debug\\bitcoind.exe arg";
    

    There was no null terminator inserted/overwritten for that string: <img width="1342" height="750" alt="image" src="https://github.com/user-attachments/assets/79fece01-1924-4ead-9ca7-b63377c46622" />

14. 
    hodlinator commented at 9:05 AM on October 22, 2026: contributor

    Ran it again and set a memory breakpoint on the space-char before "arg", but it didn't fire, so it wasn't temporarily replaced with NUL and then set back to space (as far as the debugger could observe, I confirmed it would break when our code modified memory).

15. 
    stickies-v commented at 9:18 AM on October 22, 2026: contributor

    Thanks for the empirical data, @hodlinator! Also, command_line is a local variable and unused after the CreateProcessA call, and the buffer CreateProcessA receives is the same as before - just with a different owner.

16. 
    hebasto commented at 12:16 PM on October 22, 2026: member

    Reproduced on Windows with the following harness:

    #include <crtdbg.h>
    #include <util/subprocess.h>
    
    std::string SysErrorString(int) { return ""; }
    
    int main()
    {
        _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
        _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
        _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
    
        subprocess::Popen p({"cmd.exe", "/c", "exit", "0"},
                            subprocess::input{subprocess::PIPE},
                            subprocess::output{subprocess::PIPE},
                            subprocess::error{subprocess::PIPE});
        p.wait();
    }
    

    On the master branch, it prints:

    Detected memory leaks!
    Dumping objects ->
    {204} normal block at 0x0147E740, 18 bytes long.
     Data: <cmd.exe /c exit > 63 6D 64 2E 65 78 65 20 2F 63 20 65 78 69 74 20
    Object dump complete.
    
    

    With this PR, it prints nothing.

17. DrahtBot requested review from hebasto on Oct 22, 2026
18. hebasto approved
19. 
    hebasto commented at 1:18 PM on October 22, 2026: member

    ACK fe6608590c624042fa4a77a6c9f3a46b70bc646c.

    Consider #36581 (review) as a nit.

20. 
    winterrdog commented at 1:19 PM on October 22, 2026: contributor

    ACK fe6608590c624042fa4a77a6c9f3a46b70bc646c

21. subprocess: fix command line memory leak on Windows

    Popen::execute_process() copied the command line into a buffer from
    new[], but did not delete it. This leaked memory each time a process
    started on Windows.
    
    CreateProcessA needs a writable command line buffer. Since C++17,
    std::string::data() supplies one, so give the std::string buffer
    directly and remove the copy.

    cf30d0a412
22. stickies-v force-pushed on Oct 22, 2026
23. 
    stickies-v commented at 2:41 PM on October 22, 2026: contributor

    Force-pushed to remove an unnecessary docstring change, no other changes.

24. hebasto approved
25. 
    hebasto commented at 2:44 PM on October 22, 2026: member

    re-ACK cf30d0a4123bb4237495f04e723ce9f706c2edf2.

26. DrahtBot requested review from winterrdog on Oct 22, 2026
27. DrahtBot requested review from hodlinator on Oct 22, 2026
28. 
    winterrdog commented at 2:47 PM on October 22, 2026: contributor

    reACK cf30d0a4123bb4237495f04e723ce9f706c2edf2

29. fanquake merged this on Oct 22, 2026
30. fanquake closed this on Oct 22, 2026

    ---

31. stickies-v deleted the branch on Oct 22, 2026

Contributors

stickies-vDrahtBothebastohodlinatorjpk68seditedmaflckowinterrdog

Review Requested
hodlinator