Tor needs to parse its command line exactly once

$ /usr/sbin/tor --ControlPortWriteToFile -f --ControlPort auto
Dec 04 16:16:22.230 [notice] Tor v0.2.3.8-alpha (git-da15c0cbd6638af3). This is experimental software. Do not rely on it for strong anonymity. (Running on Linux x86_64)
Dec 04 16:16:22.232 [warn] Unable to open configuration file "--ControlPort".
Dec 04 16:16:22.232 [err] Reading config failed--see warnings above.

changed milestone to %Tor: 0.2.5.x-final

added component::core tor/tor milestone::Tor: 0.2.5.x-final priority::high resolution::implemented status::closed tor-client type::defect labels

Trac:
Priority: normal to major

Hm. It looks like the problem here is that it's misparsing an invalid command line.

Can we come up with an example of a Valid command line that gets misparsed in this way?

That's not particularly hard, but it's a very constructed case:

--ContactInfo -f --Nickname bla

Replying to nickm:

Hm. It looks like the problem here is that it's misparsing an invalid command line.

Can we come up with an example of a Valid command line that gets misparsed in this way?

That command line is valid.

Trac:
Keywords: N/A deleted, tor-client added

Trac:
Component: Tor Client to Tor

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Trac:
Username: ctoader

4647.diff

When reading the configuration file name, I've added a check whether the file exists and if it does not and it starts with a '-' character the option is ignored. Basically I made a compromise between going through option_vars_ (O(N) algorithm) and simply discarding filenames starting with '-'.

Trac:
Username: ctoader

Thanks!

Trac:
Status: new to needs_review

I don't see how this code matches the description above. Where is the part that looks for a "-" character? (Also, that's not really a great idea. It is okay for a filename to start with a - character.)

It looks like you've removed support for logging "configuration file not present; using reasonable defaults.".

Minor issue: have a look at the diff, or how it appears in https://trac.torproject.org/projects/tor/attachment/ticket/4647/4647.diff : the indentation is mismatched in places, there are lines split for no reason, and there are new blank lines added for no reason.

Finally, as written, this doesn't actually work. If I say "tor --ControlPortWriteToFile -f --ControlPort auto", then the key-value pairs should be ControlPortWriteToFile="-f" and ControlPort="auto", even if there is a file named ControlPort.

The right thing to do here is probably something like changing the way that config_get_commandlines() works so that instead of only returning a linked list of options in result, it also returns a list of the things that it currently skips. That way both torrc-option-on-the-commandline parsing (which uses config_get_commandlines), and commandline-only parsing (currently done in find_torrc_fname and elsewhere) would use the same rules for deciding what is an option and what is an argument.

Trac:
Status: needs_review to needs_revision

Trac:
Username: ctoader

4647_02.patch

Apologies, I have appended the wrong patch, which is why the description was not consistent.. I will try your suggestion and make a different fix

Trac:
Username: ctoader

Trac:
Username: ctoader

4647_03.patch

Is this what you had in mind? Please don't waste too much time with this, just wanted to get some feedback before testing it more thoroughly. This version does exactly what you've said: "The right thing to do here is probably something like changing the way that config_get_commandlines() works so that instead of only returning a linked list of options in result, it also returns a list of the things that it currently skips. That way both torrc-option-on-the-commandline parsing (which uses config_get_commandlines), and commandline-only parsing (currently done in find_torrc_fname and elsewhere) would use the same rules for deciding what is an option and what is an argument."

Trac:
Username: ctoader

Yeah, this looks promising. Let me know when it's cleaned up and tested.

Trac:
Username: ctoader

4647-clean.patch

Cleaned and tested the changes. Please let me know if you want me to change anything.

Trac:
Username: ctoader

Trac:
Status: needs_revision to needs_review

Related to #9578 (moved)

I've combined ctoader's patches, added some fixups, and changes for #9578 (moved) and #9573 (moved) in branch "bug4647". It needs testing; writing some tests now.

Trac:
Status: needs_review to needs_revision

There's an initial set of (python) integration tests in the branch now; more work needed.

ctoader liked an earlier version of this; now it has full integration tests. See "bug4647" in my public repo. I'll let anybody who wants to look at it look at it for while, then I'll merge it.

Trac:
Status: needs_revision to needs_review

I asked ctoader to have a look, and he said:

I told you I'd email back about #4647 (moved), I went through it again with the debugger and it looks good as it is. I wrote just a few notes on how I think things would be more robust.

The only drawback is that you cannot recover from a bad command such as "asdasd" which is treated as a command which also takes an argument so "asdad -f something.cfg" would pair the first 2 as key-value which would mess up the whole thing. Could be fixed, but most likely is not worth the effort, if config_parse_commandline() also validates each command, instead of having the commands validated in options_init_from_string(). This would also have to be done to options in conf files as well (in load_torrc_from_disk() maybe).

Also config_parse_commandline() can be called just once in tor_init() and have options_init_from_torrc() use the global_cmdline_options and cmdline_only_only set in tor_init(). In case SIGHUP is caught, options_init_from_torrc() doesn't need to re-parse the commandline, but it could if you check for argv == NULL.

I hope this helps, even if I didn't find any real issues!

Note: merge #6384 (moved) after merging this.

Replying to nickm:

I asked ctoader to have a look, and he said:

I told you I'd email back about #4647 (moved), I went through it again with the debugger and it looks good as it is. I wrote just a few notes on how I think things would be more robust.

The only drawback is that you cannot recover from a bad command such as "asdasd" which is treated as a command which also takes an argument so "asdad -f something.cfg" would pair the first 2 as key-value which would mess up the whole thing. Could be fixed, but most likely is not worth the effort, if config_parse_commandline() also validates each command, instead of having the commands validated in options_init_from_string(). This would also have to be done to options in conf files as well (in load_torrc_from_disk() maybe).

Hm. I agree that error messages could be improved, but I don't think we're actually making matters worse with this patch. Previously, we'd get really confused by "tor asdad -f something.cfg", and happily read something.cfg, and then get confused by our inability to set "asdad -f", or something like that. I'll add another ticket for that when I merge this.

Also config_parse_commandline() can be called just once in tor_init() and have options_init_from_torrc() use the global_cmdline_options and cmdline_only_only set in tor_init(). In case SIGHUP is caught, options_init_from_torrc() doesn't need to re-parse the commandline, but it could if you check for argv == NULL.

I agree that this patch doesn't deliver on the actual text of the summary ("parse command line exactly once"), but it does fine in effect ("parse command line in exactly one way". I think that actually cacheing the result is probably needless.

Merged to master! Thanks for the review.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

mentioned in issue #6384 (moved)

mentioned in issue #9573 (moved)

mentioned in issue #9578 (moved)

moved to tpo/core/tor#4647 (closed)