Opened 7 years ago

Closed 6 years ago

#4647 closed defect (implemented)

Tor needs to parse its command line exactly once

Reported by: rransom Owned by:
Priority: High Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

$ /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.

Child Tickets

Attachments (4)

4647.diff (1.8 KB) - added by ctoader 6 years ago.
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 '-'.
4647_02.patch (1.3 KB) - added by ctoader 6 years ago.
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
4647_03.patch (7.3 KB) - added by ctoader 6 years ago.
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 <b>result</b>, 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."
4647-clean.patch (10.7 KB) - added by ctoader 6 years ago.
Cleaned and tested the changes. Please let me know if you want me to change anything.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by rransom

Priority: normalmajor

comment:2 Changed 7 years ago by 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?

comment:3 Changed 7 years ago by Sebastian

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

--ContactInfo -f --Nickname bla

comment:4 in reply to:  2 Changed 7 years ago by rransom

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.

comment:5 Changed 6 years ago by nickm

Keywords: tor-client added

comment:6 Changed 6 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Changed 6 years ago by ctoader

Attachment: 4647.diff added

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 '-'.

comment:8 Changed 6 years ago by nickm

Status: newneeds_review

Thanks!

comment:9 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

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 <b>result</b>, 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.

Changed 6 years ago by ctoader

Attachment: 4647_02.patch added

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

Changed 6 years ago by ctoader

Attachment: 4647_03.patch added

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 <b>result</b>, 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."

comment:10 Changed 6 years ago by nickm

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

Changed 6 years ago by ctoader

Attachment: 4647-clean.patch added

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

comment:11 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

comment:12 Changed 6 years ago by nickm

Related to #9578

comment:13 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

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

comment:14 Changed 6 years ago by nickm

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

comment:15 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

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.

comment:16 Changed 6 years ago by nickm

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

I told you I'd email back about #4647, 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!

comment:17 Changed 6 years ago by nickm

Note: merge #6384 after merging this.

comment:18 in reply to:  16 Changed 6 years ago by nickm

Replying to nickm:

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

I told you I'd email back about #4647, 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.

comment:19 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master! Thanks for the review.

Note: See TracTickets for help on using tickets.