Remove the return value of fascist_firewall_choose_address_node()

[teor]

Let's check for the null address instead.

[teor]

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

[nickm]

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

[neel]

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?

[teor]

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.

[neel]

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

[neel]

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

[teor]

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.

[neel]

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

[neel]

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

[teor]

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.

[nickm]

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.

[neel]

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

[teor]

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.

[neel]

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

[neel]

New patch: b24734-003.patch​.

[teor]

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

[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

[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.

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?

[mikeperry]

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

[nickm]

Squashed, tested, and merged to master!

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