Skip to content

Fix spawning Tor subprocess when datadir contains spaces - #352

Open
MaximeMRF wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
MaximeMRF:fix/tor-datadir-spaces
Open

Fix spawning Tor subprocess when datadir contains spaces#352
MaximeMRF wants to merge 1 commit into
bitcoinknots:29.x-knotsfrom
MaximeMRF:fix/tor-datadir-spaces

Conversation

@MaximeMRF

Copy link
Copy Markdown

Bugfix, it resolve the issue #351

Motivation & Problem

Spawning the Tor subprocess fails when the data directory path contains spaces:

  1. subprocess::Popen was initialized with a concatenated command string, causing internal util::split() to split paths containing spaces into invalid separate arguments.
  2. The autogenerated Tor configuration file wrote DataDirectory and ControlPortWriteToFile paths without quotes, causing Tor's config parser to fail on paths with spaces.

Solution

  • Pass arguments as std::vector<std::string> to subprocess::Popen so paths with spaces are passed monolithically to the OS process without string splitting.
  • Enclose DataDirectory and ControlPortWriteToFile paths in double quotes in generated_config.
  • Add subprocess_vector_args_with_spaces unit test in torcontrol_tests.cpp.

@kwsantiago kwsantiago left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Popen change is the right fix for #351. The other two parts should come out. Details inline.

Comment thread src/torcontrol.cpp Outdated
Comment thread src/torcontrol.cpp Outdated
Comment thread src/test/torcontrol_tests.cpp Outdated
@MaximeMRF
MaximeMRF force-pushed the fix/tor-datadir-spaces branch from 7fdd9ad to df4fc30 Compare August 4, 2026 20:37

@kwsantiago kwsantiago left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tACK df4fc30

Verified on Linux with tor 0.4.8.10: datadir containing spaces launches Tor and advertises an onion, -torexecute="env tor" still works so the multi-word case is preserved, plain datadir unaffected, torcontrol_tests green. Generated torrc is back to unquoted values, so the Windows escape-sequence problem is gone.

Nit, not blocking: commit message is the branch name. Something like torcontrol: Pass Tor arguments as a vector to handle paths with spaces would read better in the log.

@chrisguida

Copy link
Copy Markdown

ACK df4fc30

A few nits:

  • The PR description is out of date and should be updated to remove the torrc quoting and the subprocess_vector_args_with_spaces test
  • The commit message @kwsantiago suggested is better than the current one

Suggestions for follow-up:

  • A regression test would actually be easier than @kwsantiago suggested, torcontrol_tests.cpp already forward-declares SplitTorReplyLine/ParseTorReplyMapping to test internals without a header, so hoisting the argv assembly into a small free function would make it directly testable. It would need an #ifdef ENABLE_TOR_SUBPROCESS guard and a bitcoin-build-config.h include in the test, since that's where subprocess.h comes in.
  • The tor executable path still can't contain spaces (-torexecute="C:\Program Files\Tor\tor.exe" still breaks, and ENABLE_TOR_SUBPROCESS is on by default on Windows).

luke-jr pushed a commit that referenced this pull request Aug 5, 2026
Github-Pull: #352
Rebased-From: df4fc30
@luke-jr luke-jr added this to the 29.4 milestone Aug 5, 2026
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.

4 participants