Remove the return value of fascist_firewall_choose_address_node()

Let's check for the null address instead.

changed milestone to %Tor: 0.3.4.x-final

added 034-removed-20180328 034-triage-20180328 component::core tor/tor ipv6 milestone::Tor: 0.3.4.x-final owner::neel parent::24403 points::0.5 priority::medium resolution::implemented reviewer::mikeperry severity::normal status::closed tor-relay type::defect labels

The 0.3.3 freeze deadline has passed, all these children of #24403 (moved) belong in 0.3.4

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

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

I am interested in taking this ticket.

However, looking at the source code, I see fascist_firewall_choose_address_node() returns a fascist_firewall_choose_address_base(). Would it be okay to just call the latter from the former without a return value?

Trac:
Owner: N/A to neel
Status: new to assigned
Cc: N/A to neel@neelc.org

Please change both functions so they do not return any values.

If they would have returned an error value, make sure that the address they return in the pointer is the null address. I think you can generate a null address using tor_addr_make_null() or similar. See get_pref_orport() and fet_prim_orport() for examples of similar functions.

Trac:
b24734-001.patch

[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)

I have a patch under the filename b24734-001.patch.

Thanks for the patch.

These things need to be fixed:

Change comments like:

If neither address is chosen, return 0, else return 1.

So they say what the functions do now.

Make sure every function initialises the ap value before it returns. For example, the first few lines in fascist_firewall_choose_address_rs(), fascist_firewall_choose_address_node() and fascist_firewall_choose_address_dir_server() leave this value uninitialised. Please make this change in a separate commit or patch file. Leaving return values uninitialised is a potential security issue, and we should backport it to 0.2.9 and later.

Trac:
Status: assigned to needs_revision

Trac:
b24734-002.patch

[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)

I have a revised patch which initializes the ap value: b24734-002.patch​.

In the first commit, when you initialise ap:

• please assert that ap is not NULL before initialising it
• please do the initialisation first, before any of the return statements in the function. Otherwise ap can be returned uninitialised.
• please use the address functions to do the initialisation: tor_addr_make_null(&ap->addr, AF_UNSPEC); for functions that can return IPv4 or IPv6, and tor_addr_make_null(&ap->addr, AF_INET); for functions that can only return IPv4.

In the second commit, please also fix comment lines that aren't true any more:

choose_address returns 1 on success, but get_prim_orport returns 0.

If neither address is chosen, return 0, else return 1.

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

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

I have one question: should I place tor_assert(ap) before I initialize ap, after, or just remove tor_assert(ap) completely?

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

Replying to neel:

I have one question: should I place tor_assert(ap) before I initialize ap, after, or just remove tor_assert(ap) completely?

You must assert that ap is not null before you initialise it.

Trac:
b24734-003.patch

[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 3)

New patch: b24734-003.patch​.

Trac:
Status: needs_revision to needs_review

Can someone else please do the next review on this ticket?

Trac:
Reviewer: N/A to mikeperry

Ok I took Neel's patches and put them in a branch. I also noticed a couple logic issues:

1. Since we're getting rid of the return value, and there was some code relying on it that was changed to use tor_add_port_is_valid_ap(), we need to zero the ap pointer before any early return from these functions for those correctness checks to still be valid. I moved the zeroing code up in a fixup commit for this. I double-checked that tor_addr_port_is_valid_ap() rejects these zeroed addrs. It does, if they are also not AF_INET.
2. There were a couple of places that lacked a tor_assert(ap). So I added them.

I added fixup commits for these.

Neel - be sure to consider calling, surrounding, and future code when you make changes like this.

I think this is ready to go: https://oniongit.eu/mikeperry/tor/commits/bug24734

Trac:
Status: needs_review to merge_ready

Replying to mikeperry:

Ok I took Neel's patches and put them in a branch. I also noticed a couple logic issues:

1. Since we're getting rid of the return value, and there was some code relying on it that was changed to use tor_add_port_is_valid_ap(), we need to zero the ap pointer before any early return from these functions for those correctness checks to still be valid. I moved the zeroing code up in a fixup commit for this.

Thanks, that deals with my second review point in: https://trac.torproject.org/projects/tor/ticket/24734?replyto=18#comment:9

I double-checked that tor_addr_port_is_valid_ap() rejects these zeroed addrs. It does, if they are also not AF_INET.

That's not quite accurate. tor_addr*_is_valid*() rejects addresses/ports when:

• the address is all zeroes, and for_listening is 0
• the port is zero, and for_listenining is 0
• the address not an IPv4 address, and for_listening is 1

Since for_listening is 0 in all the checks added in the patch, the first two cases apply.

2. There were a couple of places that lacked a tor_assert(ap). So I added them.

Thanks, that deals with my first review point in: https://trac.torproject.org/projects/tor/ticket/24734?replyto=18#comment:9

I added fixup commits for these.

Neel - be sure to consider calling, surrounding, and future code when you make changes like this.

I think this is ready to go: https://oniongit.eu/mikeperry/tor/commits/bug24734

I'm happy with it. Does it pass all the unit tests?

Replying to teor:

Replying to mikeperry:

Ok I took Neel's patches and put them in a branch. I also noticed a couple logic issues:

1. Since we're getting rid of the return value, and there was some code relying on it that was changed to use tor_add_port_is_valid_ap(), we need to zero the ap pointer before any early return from these functions for those correctness checks to still be valid. I moved the zeroing code up in a fixup commit for this.

Thanks, that deals with my second review point in: https://trac.torproject.org/projects/tor/ticket/24734?replyto=18#comment:9

I double-checked that tor_addr_port_is_valid_ap() rejects these zeroed addrs. It does, if they are also not AF_INET.

That's not quite accurate. tor_addr*_is_valid*() rejects addresses/ports when:

• the address is all zeroes, and for_listening is 0
• the port is zero, and for_listenining is 0
• the address not an IPv4 address, and for_listening is 1

Since for_listening is 0 in all the checks added in the patch, the first two cases apply.

What I meant was: because this calls tor_addr_make_null with AF_UNSPEC, then the family will not be AF_INET. So tor_addr_is_valid() cannot return false for this null addr, even if for_listening is true.

2. There were a couple of places that lacked a tor_assert(ap). So I added them.

Thanks, that deals with my first review point in: https://trac.torproject.org/projects/tor/ticket/24734?replyto=18#comment:9

I added fixup commits for these.

Neel - be sure to consider calling, surrounding, and future code when you make changes like this.

I think this is ready to go: https://oniongit.eu/mikeperry/tor/commits/bug24734

I'm happy with it. Does it pass all the unit tests?

They do on my system. (Do you know what I have to do to connect travis to oniongit? Does it only check github? And does that require config?)

Squashed, tested, and merged to master!

(But next time let's use github so we get travis to run our tests too.)

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

closed

changed time estimate to 4h

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