Tor 0.3.3.1-alpha resumes allowing RunAsDaemon=1 with relative DataDirectory

In 0.3.2.1-alpha we fixed #22731 (moved) (in commit 6fea44c6): we started refusing to start if RunAsDaemon is set and also any of a variety of config options are set to a relative value.

And then in 0.3.3.1-alpha we did commit 192be006, which split the original options->DataDirectory into a new DataDirectory_option variable.

But warn_about_relative_paths() continues to look at

  n += warn_if_option_path_is_relative("DataDirectory",options->DataDirectory);

and that function is called near the top of options_validate() -- before we call validate_data_directories() which is what populates options->DataDirectory.

changed milestone to %Tor: 0.3.4.x-final

added 033-backport-unreached 034-backport 034-backport-unreached component::core tor/tor milestone::Tor: 0.3.4.x-final priority::medium regression resolution::fixed reviewer::ahf severity::normal status::closed type::defect labels

(Found by weasel while he was trying to make a simple Tor test script)

Seems like we should either look at DataDirectory_option in warn_about_relative_paths(), or call validate_data_directories() earlier in options_validate(). My first thought is that the former is better (especially to fix the regression).

Also, it would probably be wise to think about what sort of unit test would have caught this.

Confirmed that

diff --git a/src/or/config.c b/src/or/config.c
index 2660fbd..43cc082 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -3184,7 +3184,8 @@ warn_about_relative_paths(or_options_t *options)
   n += warn_if_option_path_is_relative("GeoIPv6File",options->GeoIPv6File);
   n += warn_if_option_path_is_relative("Log",options->DebugLogFile);
   n += warn_if_option_path_is_relative("AccelDir",options->AccelDir);
-  n += warn_if_option_path_is_relative("DataDirectory",options->DataDirectory);
+  n += warn_if_option_path_is_relative("DataDirectory",
+                                       options->DataDirectory_option);
   n += warn_if_option_path_is_relative("PidFile",options->PidFile);
 
   for (config_line_t *hs_line = options->RendConfigLines; hs_line;

fixes it for me.

(Did we move any other warn_about_relative_paths() strings into a new foo_option variable?)

Ok, that above fix is bad, because it will complain when you set DataDirectory to e.g. ~/.test -- which is actually an absolute path, even if it doesn't start with /.

So I now think the "validate_data_directories() earlier" option is the best plan.

My bug28298 branch has this better fix.

But it still lacks unit test improvements. Maybe somebody wants to add those?

Replying to arma:

Ok, that above fix is bad, because it will complain when you set DataDirectory to e.g. ~/.test -- which is actually an absolute path, even if it doesn't start with /.

"~" expansion is a missing feature, not a bug. Your shell does it, many other contexts do not. See #28311 (moved).

But you've found another bug in Tor's config handling. On my machine, it results in a crash and a spurious call to free(). See #28312 (moved).

Replying to arma:

My bug28298 branch has this better fix.

But it still lacks unit test improvements. Maybe somebody wants to add those?

Maybe if we fixed #28312 (moved), your original solution would work?

Trac:
Status: new to needs_revision

(We don't have a status for "needs more code and more investigation". Maybe "needs_information"?)

Replying to teor:

"~" expansion is a missing feature, not a bug. Your shell does it, many other contexts do not. See #28311 (moved).

It is true that we don't call expand_filename() consistently on other config options. #28311 (moved) is a great ticket for tracking that (orthogonal) issue.

But you've found another bug in Tor's config handling. On my machine, it results in a crash and a spurious call to free(). See #28312 (moved).

Turns out that ticket is a duplicate of #27708 (moved).

So I think we resume being good to review/merge this fix. And we could still want somebody to add unit tests if they want.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to ahf

Opened: https://github.com/torproject/tor/pull/483

And a PR based of maint-0.3.3: https://github.com/torproject/tor/pull/484

Trac:
Keywords: regression deleted, regression 033-backport 034-backport added
Milestone: N/A to Tor: 0.3.5.x-final

Patch looks good. Both the PR for the maint-0.3.3 and master branches seems to make Travis happy.

Trac:
Status: needs_review to merge_ready

Merged to maint-0.3.5; marking for backport.

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: 0.3.4.x-final

Going to suggest "no backport" here.

These merge_ready tickets did not get backported to 0.3.3, and 0.3.3 is now unsupported.

Trac:
Keywords: regression 033-backport 034-backport deleted, 034-backport, 033-backport-unreached, regression added

Closing as no backport.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

Trac:
Keywords: N/A deleted, 034-backport-unreached added

closed

mentioned in issue #28311 (moved)

mentioned in issue #28312 (moved)

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

mentioned in issue tpo/core/tor#28311 (closed)

mentioned in issue tpo/core/tor#28312 (closed)