Opened 4 months ago

Closed 2 months ago

#32994 closed defect (implemented)

Get all flag defaults from port_cfg_new()

Reported by: teor Owned by: MrSquanchee
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: extra-review, technical-debt, tor-client, easy, intro, outreachy-ipv6
Cc: Actual Points: 0.3
Parent ID: Points: 1
Reviewer: ahf Sponsor:

Description

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.

Child Tickets

Change History (23)

comment:1 Changed 4 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 3 months ago by neel

Cc: neel removed
Owner: neel deleted

comment:3 Changed 3 months ago by neel

Status: assignednew

comment:4 Changed 3 months ago by MrSquanchee

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)

comment:5 Changed 3 months ago by nickm

Owner: set to MrSquanchee
Status: newassigned

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

comment:6 Changed 3 months ago by teor

Keywords: outreachy-ipv6 added

comment:7 Changed 3 months ago by MrSquanchee

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)

comment:8 Changed 3 months ago by nickm

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.

comment:9 in reply to:  8 Changed 3 months ago by teor

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().

comment:10 Changed 3 months ago by MrSquanchee

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.

comment:11 in reply to:  10 Changed 3 months ago by teor

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 and #33608 for that change.

comment:12 Changed 3 months ago by MrSquanchee

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.

comment:13 Changed 2 months ago by MrSquanchee

Status: assignedneeds_review

comment:14 Changed 2 months ago by teor

Actual Points: 0.2
Keywords: extra-review added
Milestone: Tor: unspecifiedTor: 0.4.4.x-final
Reviewer: teor
Status: needs_reviewneeds_revision

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.

comment:15 Changed 2 months ago by MrSquanchee

Status: needs_revisionneeds_review

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

comment:16 in reply to:  15 Changed 2 months ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

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.

comment:17 Changed 2 months ago by teor

Actual Points: 0.20.3

comment:18 Changed 2 months ago by MrSquanchee

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

comment:19 Changed 2 months ago by MrSquanchee

Status: needs_revisionneeds_review

comment:20 Changed 2 months ago by teor

I made a squashed PR here:

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

comment:21 Changed 2 months ago by asn

Reviewer: ahf

comment:22 Changed 2 months ago by ahf

Status: needs_reviewmerge_ready

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

comment:23 Changed 2 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master.

Note: See TracTickets for help on using tickets.