Opened 9 months ago

Closed 5 months ago

#24734 closed defect (implemented)

Remove the return value of fascist_firewall_choose_address_node()

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, tor-relay, 034-triage-20180328, 034-removed-20180328
Cc: neel@… Actual Points:
Parent ID: #24403 Points: 0.5
Reviewer: mikeperry Sponsor:

Description

Let's check for the null address instead.

Child Tickets

Attachments (3)

b24734-001.patch (13.8 KB) - added by neel 6 months ago.
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)
b24734-002.patch (16.0 KB) - added by neel 6 months ago.
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)
b24734-003.patch (16.6 KB) - added by neel 6 months ago.
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 3)

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

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

comment:2 Changed 6 months ago by nickm

Keywords: 034-triage-20180328 added

comment:3 Changed 6 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:4 Changed 6 months ago by neel

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

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?

comment:5 Changed 6 months ago by 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.

Changed 6 months ago by neel

Attachment: b24734-001.patch added

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

comment:6 Changed 6 months ago by neel

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

comment:7 Changed 6 months ago by teor

Status: assignedneeds_revision

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.

Changed 6 months ago by neel

Attachment: b24734-002.patch added

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

comment:8 Changed 6 months ago by neel

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

comment:9 Changed 6 months ago by 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.

comment:10 Changed 6 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

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.

comment:11 Changed 6 months ago by neel

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

comment:12 Changed 6 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final

comment:13 in reply to:  11 Changed 6 months ago by 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.

Changed 6 months ago by neel

Attachment: b24734-003.patch added

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

comment:14 Changed 6 months ago by neel

New patch: b24734-003.patch​.

comment:15 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

comment:16 Changed 6 months ago by teor

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

comment:17 Changed 5 months ago by asn

Reviewer: mikeperry

comment:18 Changed 5 months ago by mikeperry

Status: needs_reviewmerge_ready

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

comment:19 in reply to:  18 ; Changed 5 months ago by 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.

  1. 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?

comment:20 in reply to:  19 Changed 5 months ago by 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.

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

comment:21 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Squashed, tested, and merged to master!

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

Note: See TracTickets for help on using tickets.