Ticket31240 merged 1 #3

nmathewson · 2019-08-25T21:22:00Z

No description provided.

Remember that our goal in the present refactoring is to allow each subsystem to declare its own configuration structure and variables. To do this, each module will get its own config_format_t, and so we'll want a different structure that wraps several config_format_t objects. This is a "config_mgr_t".

The eventual design here will be that multiple config_format_t objects get registered with a single config_mgr_t. That config_mgr_t manages a "top-level" object, which has a pointer to the other objects. I had earlier thought of a different design, where there would be no top-level object, and config_mgr_t would deal with a container instead. But this would require a bunch of invasive refactoring that I don't think we should do just yet.

Iterating over this array was once a good idea, but now that we are going to have a separate structure for each submodule's configuration variables, we should indirect through the config_mgr_t object.

We'll want it to check all the subsidiary structures of the options object.

It's important to make sure that we don't change a config_mgr_t after we start using it to make objects, or we could get into inconsistent states. This feature is the start of a safety mechanism to prevent this problem.

We'll need to do it this way once the objects become more complex.

Every time we finalize a config manager, we now generate a new magic number for it, so that we'll get an assertion failure if we ever two to use an object with a different configuration manager than the one that generated it.

The right way to free a config object is now to wrap config_free(), always. Instead of creating an alternative free function, objects should provide an alternative clear callback to free any fields that the configuration manager doesn't manage. This lets us simplify our code a little, and lets us extend the confparse.c code to manage additional fields in config_free.

Right now, it doesn't do anything; this patch is meant to make sure that we're doing memory management correctly.

A configuration manager, in addition to a top-level format object, may now also know about a suite of sub-formats. Top-level configuration objects, in turn, may now have a suite of sub-objects.

teor2345

This is a partial review of the first few commits.

I have not reviewed "Replace config_find_option with..." or any of the commits after it.

src/app/config/config.c

teor2345 · 2019-08-26T01:02:38Z

src/app/config/confparse.c

@@ -271,14 +310,14 @@ config_assign_line(const config_format_t *fmt, void *options,
        log_warn(LD_CONFIG,
                 "Linelist option '%s' has no value. Skipping.", c->key);
      } else { /* not already cleared */
-        config_reset(fmt, options, var, use_defaults);
+        config_reset(mgr, options, var, use_defaults);
      }
    }
    return 0;
  } else if (c->command == CONFIG_LINE_CLEAR && !clear_first) {
    // XXXX This is unreachable, since a CLEAR line always has an


Why are the XXXXs here?
Does something still need to be done?

I think this is cleanup that should be done before closing #29211.

Agreed. Opened https://trac.torproject.org/projects/tor/ticket/31529#ticket

teor2345 · 2019-08-26T01:02:38Z

src/app/config/confparse.h

-    config_free_((fmt), (options));                   \
+void *config_new(const config_mgr_t *fmt);
+void config_free_(const config_mgr_t *fmt, void *options);
+#define config_free(mgr, options) do {                \


Should this macro uses STMT_BEGIN and STMT_END?

This is cleanup that should be done before closing #29211.

We have over 175 other "} while (0)" occurrences in our code, and only 146 STMT_ENDs. I've opened https://trac.torproject.org/projects/tor/ticket/31530 for this.

src/app/config/config.c

teor2345 · 2019-08-26T01:02:38Z

src/app/config/confparse.c

+static void
+managed_var_free_(managed_var_t *mv)
+{
+  if (!mv)


This if statement is unnecessary: tor_free(NULL) does nothing.

This change should be made before #29211 is closed.

I prefer to have this check in every free function, so that if we add more fields, we will not forget to add the check. The compiler will (I think) notice that it is redundant, and will not add any redundant code.

src/app/config/confparse.c

teor2345 · 2019-08-26T01:02:38Z

src/app/config/confparse.c

+ * NOTE: XXXX Eventually, there will be multiple objects supported within the
+ * toplevel object.  For example, the or_options_t will contain pointers
+ * to configuration objects for other modules.  This function gets
+ * the sub-object for a particular modules.


Typo: module

This change should be made before #29211 is closed.

Did you miss this comment?

I did! Fixed in 0eb81ec

teor2345 · 2019-08-26T01:02:38Z

src/app/config/confparse.c

        /* We got an empty linelist from the torrc or command line.
           As a special case, call this an error. Warn and ignore. */
        log_warn(LD_CONFIG,
                 "Linelist option '%s' has no value. Skipping.", c->key);
      } else { /* not already cleared */
-        config_reset(mgr, options, var, use_defaults);
+        config_reset(mgr, options, mvar, use_defaults);
      }
    }
    return 0;
  } else if (c->command == CONFIG_LINE_CLEAR && !clear_first) {
    // XXXX This is unreachable, since a CLEAR line always has an


What needs to be done here?
Or should the XXXX be removed?

I don't know what this change should be, or what it blocks.

This is also https://trac.torproject.org/projects/tor/ticket/31529#ticket

teor2345

There are a bunch of missing unit tests for new callbacks.

The design looks pretty ok, but it's hard to tell without an overview of where we are going. (I would really like a diagram or a formatted text summary.)

teor2345 · 2019-08-26T11:08:41Z

src/app/config/config.c

+      config_get_changes(get_options_mgr(), old_options, new_val);
+    for (config_line_t *line = changes; line; line = line->next) {
+      smartlist_add(elements, line->key);
+      smartlist_add(elements, line->value);
    }
    control_event_conf_changed(elements);


Wow, smartlist(k, v, k, v, …) is an unexpected API.
It's legacy code, so it's not something you need to fix.
But eventually maybe we could do smartlist(k, k, …), smartlist(v, v, …).

We can open a ticket to do this sometime in the future, if you think it is a good idea.

I agree a better API would be good; a config_line_t would actually be a reasonable type to use here.

I opened https://trac.torproject.org/projects/tor/ticket/31531

teor2345 · 2019-08-26T11:08:41Z

src/app/config/confparse.c

+ **/
+config_line_t *
+config_get_changes(const config_mgr_t *mgr,
+                   const void *options1, const void *options2)


Does this stop being void * later in the refactor?
If not, please document the possible types of options1 and options2.

This change should block #29211, but we can merge this PR without it.

16f07f1 has documentation here.

teor2345 · 2019-08-26T11:08:41Z

src/app/config/config.c

          /*
           * Count dirauth lines we have a default for; we'll use the
           * count later to decide whether to add the defaults manually
           */
          ++dirauth_lines_seen;
        }
-        if (strcmp(option_vars_[i].member.name, "FallbackDir") == 0) {
+        if (strcmp(var->member.name, "FallbackDir") == 0) {
          /*
           * Similarly count fallback lines, so that we can decided later


Typo: decide

Typos don't block anything, but it would be nice to fix them in #29211.

fb85c08 has a fix.

teor2345 · 2019-08-26T11:08:41Z

src/app/config/confparse.c

@@ -175,6 +175,31 @@ config_mgr_free_(config_mgr_t *mgr)
  tor_free(mgr);
 }

+/** Return a new smartlist_t containing a config_var_t for every variable that
+ * <b>mgr</b> knows about.  The elements of this smartlist do not need
+ * to be freed. */


But how long do they remain valid?

addressed in 4d1dfab

teor2345 · 2019-08-26T11:08:41Z

src/app/config/confparse.c

+}
+
+/** Return a new smartlist_t containing the names of all deprecated variables.
+ * The elements of this smartlist do not need to be freed. */


But how long do they remain valid?

addressed in 4d1dfab

teor2345 · 2019-08-26T11:08:41Z

src/app/config/or_options_st.h

@@ -1107,6 +1108,9 @@ struct or_options_t {
   * a possible previous dormant state.
   **/
  int DormantCanceledByStartup;
+
+  /**DOCDOC*/


Please document

I don't know if this is a blocking change yet. If we add actual data to subconfigs_, it should block this PR.

ad22fbc documents this.

Ok, seems fine.

teor2345 · 2019-08-26T11:08:41Z

src/app/config/or_state_st.h

@@ -94,6 +95,9 @@ struct or_state_t {
  /** True if we were dormant when we last wrote the file; false if we
   * weren't.  "auto" on initial startup. */
  int Dormant;
+
+  /**DOCDOC*/


Please document.

0c7528f documents this.

teor2345 · 2019-08-26T11:08:41Z

src/test/test_confparse.c

@@ -815,6 +816,7 @@ static config_format_t etest_fmt = {
  test_validate_cb,
  NULL,
  &extra,
+  -1,


There is no unit test with a subconfig: please add one.

This test should block this PR, because we want to ensure that the simplest subconfig works, and the memory management works.

@nmathewson what was the resolution here?

I think this is 55b7280, please confirm?

That is correct.

teor2345 · 2019-08-26T11:08:41Z

src/app/config/confparse.h

@@ -41,8 +41,8 @@ typedef int (*validate_fn_t)(void*,void*,void*,int,char**);

 struct config_mgr_t;

-/** Callback to free a configuration object. */
-typedef void (*free_cfg_fn_t)(const struct config_mgr_t *mgr, void*);
+/** Callback to clear all non-managed fields of a configuration object. */


Please explain what a non-managed field is.

This comment should not block this PR, but I need to understand what a non-managed field is.

2a088ca tries to explain this.

Ok. Do we need unit tests that have non-managed fields?

@nmathewson do we need unit tests that have non-managed fields?

This looks like beaf730?

That is correct.

teor2345 · 2019-08-26T11:08:41Z

src/test/test_confparse.c

@@ -824,7 +813,7 @@ static config_format_t etest_fmt = {
  test_deprecation_notes,
  test_vars,
  test_validate_cb,
-  test_free_cb,
+  NULL,


There aren't any tests that use a clear callback. Please add some.

@nmathewson do we need unit tests that use a clear callback?

This also looks like beaf730?

That's right.

This discovered a bug related to an extra & in config_mgr_list_deprecated_vars(): fix that.

Fix a bug in config_find_option_name() where it did not consider the abbreviations table.

This test case, at this point, only constructs the confmgr object. More code to come.

One test makes sure that the toplevel magic numbers are distinct. One test makes sure that we can parse a configuration object with two sub-objects.

teor2345 · 2019-08-28T05:41:45Z

src/app/config/confparse.h

+ *
+ * (Regular fields get cleared by config_reset(), but you might have fields
+ * in the object that do not correspond to configuration variables.  If those
+ * fields need to be cleared or freed, this is where to do it.)


What is the void * type here?

Done in c281fa0

teor2345 · 2019-08-28T05:41:45Z

src/test/test_confparse.c

@@ -815,6 +816,7 @@ static config_format_t etest_fmt = {
  test_validate_cb,
  NULL,
  &extra,
+  -1,


I think this is 55b7280, please confirm?

teor2345

I made some comments here, but GitHub seems to be having trouble finding some of them?

coveralls · 2019-08-29T19:29:53Z

Pull Request Test Coverage Report for Build 852

334 of 363 (92.01%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.09%) to 63.024%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/app/config/confparse.c	283	285	99.3%
src/app/config/statefile.c	11	14	78.57%
src/app/config/config.c	26	50	52.0%

Files with Coverage Reduction	New Missed Lines	%
src/app/config/config.c	1	72.86%

Totals
Change from base Build 819:	0.09%
Covered Lines:	47564
Relevant Lines:	75470

💛 - Coveralls

When accessing the last_resolved_address cache we always need to convert the AF family value to an index value else we are out of bound and thus overflowing if we write to it. This fix is on code that has not been released. GeKo reported the following libasan crash using Tor Browser alpha with tor 0.4.5.0-alpha-dev (3c884bc): ==4240==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55888490e388 at pc 0x5588842cc216 bp 0x7ffc8c421b00 sp 0x7ffc8c421af8 READ of size 2 at 0x55888490e388 thread T0 #0 0x5588842cc215 in tor_addr_compare_masked (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x5a6215) #1 0x558884203210 in is_local_to_resolve_addr (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x4dd210) #2 0x558883f7e252 in channel_tls_connect (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x258252) #3 0x558883f87ff7 in channel_connect_for_circuit (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x261ff7) #4 0x558883f8bc90 in circuit_handle_first_hop (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x265c90) #5 0x558883f8c891 in circuit_establish_circuit (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x266891) #6 0x558883fc3bbc in circuit_launch_by_extend_info (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x29dbbc) torproject#7 0x558883fc5900 (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x29f900) torproject#8 0x558883fc6988 in connection_ap_handshake_attach_circuit (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x2a0988) torproject#9 0x558883fd0d3f in connection_ap_attach_pending (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x2aad3f) torproject#10 0x7f4d50110885 (TorBrowser/Tor/libevent-2.1.so.7+0x22885) torproject#11 0x7f4d501110de in event_base_loop (TorBrowser/Tor/libevent-2.1.so.7+0x230de) torproject#12 0x558883f69b3c in do_main_loop (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x243b3c) torproject#13 0x558883f3f70c in tor_run_main (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x21970c) torproject#14 0x558883f3c2f7 in tor_main (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x2162f7) #15 0x558883f3531b in main (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x20f31b) #16 0x7f4d4f76acc9 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26cc9) torproject#17 0x558883f3ba00 (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x215a00) 0x55888490e388 is located 24 bytes to the left of global variable 'state_mgr' defined in 'src/app/config/statefile.c:184:22' (0x55888490e3a0) of size 8 0x55888490e388 is located 32 bytes to the right of global variable 'global_state' defined in 'src/app/config/statefile.c:204:20' (0x55888490e360) of size 8 SUMMARY: AddressSanitizer: global-buffer-overflow (/home/thomas/Arbeit/Tor/tor-browser-build/tor-browser_en-US/Browser/TorBrowser/Tor/tor+0x5a6215) in tor_addr_compare_masked Shadow bytes around the buggy address: 0x0ab190919c20: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x0ab190919c30: 04 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 03 f9 f9 f9 0x0ab190919c40: f9 f9 f9 f9 03 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x0ab190919c50: 00 00 00 04 f9 f9 f9 f9 00 00 00 00 00 00 00 04 0x0ab190919c60: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 =>0x0ab190919c70: f9[f9]f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x0ab190919c80: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 0x0ab190919c90: 04 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 0x0ab190919ca0: 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x0ab190919cb0: 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 0x0ab190919cc0: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==4240==ABORTING Signed-off-by: David Goulet <dgoulet@torproject.org>

nmathewson added 14 commits Jul 24, 2019

Replace config_find_option with a variant that exposes less

7e91d4f

Lower responsibility for listing changed options into confparse.c

89a3051

Fix every place in config.c that knew about option_vars_.

627ab9d

Iterating over this array was once a good idea, but now that we are going to have a separate structure for each submodule's configuration variables, we should indirect through the config_mgr_t object.

Change CONFIG_CHECK() macro to not need a config_format_t

7abd43a

We'll want it to check all the subsidiary structures of the options object.

Add a "freeze" function for config_mgr_t objects.

dde091e

It's important to make sure that we don't change a config_mgr_t after we start using it to make objects, or we could get into inconsistent states. This feature is the start of a safety mechanism to prevent this problem.

Sort variables in config_mgr_t.all_vars alphabetically

a1b2817

Use config_new() to construct configuration objects.

57e87cc

We'll need to do it this way once the objects become more complex.

Use special magic to enforce manager/object connection.

94bfbf7

Every time we finalize a config manager, we now generate a new magic number for it, so that we'll get an assertion failure if we ever two to use an object with a different configuration manager than the one that generated it.

Make a config_suite_t type to hold multiple config sub-objects

ccde62a

Right now, it doesn't do anything; this patch is meant to make sure that we're doing memory management correctly.

Partial support for multiplicity in configuration objects

76e3cc9

A configuration manager, in addition to a top-level format object, may now also know about a suite of sub-formats. Top-level configuration objects, in turn, may now have a suite of sub-objects.

Merge branch 'ticket31240' into ticket31240_merged_1

248dbbb

teor2345 suggested changes Aug 26, 2019

View changes

nmathewson added 14 commits Aug 26, 2019

finish a comment explaining all_abbrevs

9fe9f20

Document types for config_get_changes()

16f07f1

Fix typo in comment about FallbackDir handling

fb85c08

document return lifespans for config_mgr_list_vars()

4d1dfab

Document or_options_t.subconfigs_

ad22fbc

Document or_state_t.substates_

0c7528f

Better explain config_clear_fn_t

2a088ca

Add tests for variable-listing functions.

3ff9d4e

This discovered a bug related to an extra & in config_mgr_list_deprecated_vars(): fix that.

Tests for config_find_option_name()

4d06728

Fix a bug in config_find_option_name() where it did not consider the abbreviations table.

Start on test cases for the multi-object feature of confmgr.

1df56e7

This test case, at this point, only constructs the confmgr object. More code to come.

Confmgr tests: test deprecations and abbreviations.

8b9c3e6

Simple tests for nested configuration formats

55b7280

One test makes sure that the toplevel magic numbers are distinct. One test makes sure that we can parse a configuration object with two sub-objects.

Add a test for dumping a compound configuration

e9a6fa5

Fix typo in a comment on config_mgr_get_obj_mutable()

0eb81ec

Add a test for clear_fn support in handling non-managed fields

beaf730

teor2345 reviewed Aug 28, 2019

View changes

teor2345 suggested changes Aug 28, 2019

View changes

nmathewson / tor Public

Ticket31240 merged 1 #3

Are you sure you want to change the base?

Ticket31240 merged 1 #3

Conversation

nmathewson commented Aug 25, 2019

teor2345 left a comment

teor2345 left a comment

teor2345 left a comment

coveralls commented Aug 29, 2019 • edited

Pull Request Test Coverage Report for Build 852

💛 - Coveralls

coveralls commented Aug 29, 2019 •

edited