Get all flag defaults from port_cfg_new()

The Tor port flags code is needlessly complex. To change a default, you need to change:

• port_cfg_new()
• port_parse_config()
• connection_listener_new()

We should call port_cfg_new() for the defaults in all these cases.

changed milestone to %Tor: 0.4.4.x-final

added actualpoints::0.3 component::core tor/tor easy extra-review intro milestone::Tor: 0.4.4.x-final outreachy-ipv6 owner::MrSquanchee points::1 priority::medium resolution::implemented reviewer::ahf severity::normal status::closed technical-debt tor-client type::defect labels

Trac:
Owner: N/A to neel
Status: new to assigned
Cc: N/A to neel

Trac:
Owner: neel to N/A
Cc: neel to N/A

Trac:
Status: assigned to new

Hii teor,

I would like to work on this issue as my first issue with tor.

Could you please assign it to me :)

Thanks,

Suraj (MrSquanchee)

Sure! Please let us know if you have any questions, or run into any trouble. :)

Trac:
Owner: N/A to MrSquanchee
Status: new to assigned

Trac:
Keywords: N/A deleted, outreachy-ipv6 added

Hii I have added all the default flags in port_cfg_new(). But I can't see as to why I need to update port_parse_config() and connection_listener_new() as they seem to work very well and are encapsulated from port_cfg_new() (Please correct me if I go wrong). The default flags and their values are as follows -->

/* entry_cfg flags */
  cfg->entry_cfg.ipv4_traffic = 1;
  cfg->entry_cfg.ipv6_traffic = 1;
  cfg->entry_cfg.prefer_ipv6 = 1;
  cfg->entry_cfg.dns_request = 1;
  cfg->entry_cfg.onion_traffic = 1;
  cfg->entry_cfg.prefer_ipv6_virtaddr = 1;
  cfg->entry_cfg.socks_prefer_no_auth = 0;
  cfg->entry_cfg.socks_iso_keep_alive = 0;
  cfg->entry_cfg.cache_ipv4_answers = 0;
  cfg->entry_cfg.cache_ipv6_answers = 0;
  cfg->entry_cfg.use_cached_ipv4_answers = 0;
  cfg->entry_cfg.use_cached_ipv6_answers = 0;
  cfg->entry_cfg.extended_socks5_codes = 0;

  /* server_cfg flags */
  cfg->server_cfg.no_advertise = 0;
  cfg->server_cfg.no_listen = 0;
  cfg->server_cfg.all_addrs = 0;
  cfg->server_cfg.bind_ipv4_only = 0;
  cfg->server_cfg.bind_ipv6_only = 0;

  /* other flags */
  cfg->is_group_writable = 0;
  cfg->is_world_writable = 0;
  cfg->relax_dirmode_check = 0;`

These flags are reflected by the initial flag values set in port_parse_config() here [https://gitweb.torproject.org/tor.git/tree/src/app/config/config.c#n6137]

If you guys agree, I will open a PR in Github.com. Waiting for your suggestions.

Thank you,

Suraj (MrSquanchee)

So the problem with port_parse_config() is that the all of its defaults would have to be kept in sync with port_cfg_new(). If we make a change to one place, we have to make it to the other, or else we don't get the expected results. This has caused hard-to-diagnose bugs in the past.

A good solution here might be to create the port_cfg_t object near the start of the loop in port_cfg_t, and then assign to its flags directly, rather than first assigning them to intermediate variables and only copying them at the end of the loop. Teor might have some thoughts here as the original opener of this bug.

As for port_cfg_new(): it isn't actually necessary to set any flags to 0 there, since the object is allocated with tor_malloc_zero(), which initializes all the memory in it to 0.

Replying to nickm:

A good solution here might be to create the port_cfg_t object near the start of the loop in port_cfg_t, and then assign to its flags directly, rather than first assigning them to intermediate variables and only copying them at the end of the loop. Teor might have some thoughts here as the original opener of this bug.

Yes, I think we should create a port_config_t object using port_config_new() at the start of:

• port_parse_config()
• connection_listener_new() And then make any changes we need to make in that object.

Please let us know if you find any differences between the default flags in port_config_new() and port_parse_config().

I changed the code of port_parse_config() successfully accommodating a port_config_t object. But still I can't see a necessity of a default port_config_t in connection_listener_new() because it's just memcopying in ln1500 https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/connection.c#n1500 and setting the traffic values conditionally here at https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/connection.c#n1517 . So it doesn't seem nice to me to give them defaults without checking the condition at line 1517. Suggest me if I should send a PR on github.com.

Replying to MrSquanchee:

I changed the code of port_parse_config() successfully accommodating a port_config_t object.

Suggest me if I should send a PR on github.com.

Great! Please send PRs whenever you write code.

We can't give feedback unless we can see the code :-)

I also noticed one place where you will need to be careful:

• prefer_ipv6_virtaddr is called prefer_ipv6_automap in port_parse_config()

But still I can't see a necessity of a default port_config_t in connection_listener_new() because it's just memcopying in ln1500 https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/connection.c#n1500

You're right, these ports come from get_configured_ports(), which is set in parse_ports(), which uses port_parse_config().

and setting the traffic values conditionally here at https://gitweb.torproject.org/tor.git/tree/src/core/mainloop/connection.c#n1517 . So it doesn't seem nice to me to give them defaults without checking the condition at line 1517.

When you update port_parse_config() to use port_cfg_new(), all the defaults will come from the same place.

So I am not sure what we should do with the extra settings in connection_listener_new() at line 1517. I think we should delete them (stop forcing them on). But that's not a simple refactor, and it needs to happen in two different releases. So I opened #33607 (moved) and #33608 (moved) for that change.

Hii mentors,

You can check my pull request associated to this ticket here on github https://github.com/torproject/tor/pull/1795 .

And thanks for your suggestions.

P.S. This would be my first PR for torproject. Optimistic for the future :)

Thanks, Suraj.

PR @ https://github.com/torproject/tor/pull/1795

Trac:
Status: assigned to needs_review

Thanks for this PR, it looks really good!

It's normal for reviewers to request changes.

I listed the changes I'd like to see in my review on the pull request: https://github.com/torproject/tor/pull/1795#pullrequestreview-374129928

I'm also marking this ticket for extra review, to make sure I didn't miss anything. Someone else will have a look at it after I'm done reviewing it.

Trac:
Keywords: N/A deleted, extra-review added
Reviewer: N/A to teor
Status: needs_review to needs_revision
Actualpoints: N/A to 0.2
Milestone: Tor: unspecified to Tor: 0.4.4.x-final

I made the changes requested. Travic CI and Appveyor checks are greened. Needs a fresh review :)

Additional suggestions and changes are most welcomed.

@https://github.com/torproject/tor/pull/1795

Trac:
Status: needs_revision to needs_review

Replying to MrSquanchee:

I made the changes requested. Travic CI and Appveyor checks are greened. Needs a fresh review :)

Additional suggestions and changes are most welcomed.

@https://github.com/torproject/tor/pull/1795

I think there's a simpler way to free cfg at the end of the function.

What do you think of: https://github.com/torproject/tor/pull/1796/commits/3b7b512908e65ad74e672ea846151f31a0b31c97

(This function is a bit confusing, because it's very big. Over time, we want to split big functions into smaller functions, so they are easier to understand. But we don't need to do that in this pull request.)

If you like my change, then we are almost done!

The final step is to make a changes file: https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n95

I like to copy an existing changes file, and use it as a template. Remember to change the ticket number!

You can also say "Patch by MrSquanchee" at the end of the changes file, if you like.

Here is a good example of a "Code simplification and refactoring" changes file: https://github.com/torproject/tor/blob/master/changes/ticket33349

Run "make check-changes" after you're done, to check the format of the file.

Trac:
Status: needs_review to needs_revision
Reviewer: teor to N/A

Trac:
Actualpoints: 0.2 to 0.3

teor : if you like my changes than we are almost done.

That's a very clever hack something most of us new programmers are unable to.

Added a changes file and added the irritating space before the label on line 6509 config.c

Let me know if I did the changes file correctly ticket#32994.

I think it's ready for a final review :)

I just want to get over my first PR.

@https://github.com/torproject/tor/pull/1795

Trac:
Status: needs_revision to needs_review

I made a squashed PR here:

• master: https://github.com/torproject/tor/pull/1797

I've marked this ticket for extra review, someone else should review it in the next week or so.

Trac:
Reviewer: N/A to ahf

This looks good.

A nice follow up patch to this would be to change some of the integer values that we use with only zero and ones to be C99 bool's instead of integers :-)

Trac:
Status: needs_review to merge_ready

Merged to master.

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

closed

changed time estimate to 8h

added 2h 24m of time spent

mentioned in issue #33607 (moved)

mentioned in issue #33804 (moved)

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

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

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