Opened 2 weeks ago

Last modified 2 weeks ago

#28298 merge_ready defect

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

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: regression 033-backport 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

In 0.3.2.1-alpha we fixed #22731 (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.

Child Tickets

Change History (14)

comment:1 Changed 2 weeks ago by arma

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

comment:2 Changed 2 weeks ago by arma

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.

comment:3 Changed 2 weeks ago by arma

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?)

comment:4 Changed 2 weeks ago by 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 /.

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

comment:5 Changed 2 weeks ago by arma

My bug28298 branch has this better fix.

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

comment:6 in reply to:  5 ; Changed 2 weeks ago by teor

Status: newneeds_revision

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.

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.

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, your original solution would work?

comment:7 Changed 2 weeks ago by teor

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

comment:8 in reply to:  6 Changed 2 weeks ago by arma

Status: needs_revisionneeds_review

Replying to teor:

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

It is true that we don't call expand_filename() consistently on other config options. #28311 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.

Turns out that ticket is a duplicate of #27708.

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.

comment:9 Changed 2 weeks ago by dgoulet

Reviewer: ahf

comment:11 Changed 2 weeks ago by ahf

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

comment:12 Changed 2 weeks ago by nickm

Keywords: 033-backport 034-backport added
Milestone: Tor: 0.3.5.x-final

comment:13 Changed 2 weeks ago by ahf

Status: needs_reviewmerge_ready

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

comment:14 Changed 2 weeks ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final

Merged to maint-0.3.5; marking for backport.

Note: See TracTickets for help on using tickets.