Opened 2 years ago
Closed 20 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)
Change History (24)
comment:1 Changed 23 months ago by
Milestone: | Tor: 0.3.3.x-final → Tor: 0.3.4.x-final |
---|
comment:2 Changed 21 months ago by
Keywords: | 034-triage-20180328 added |
---|
comment:3 Changed 21 months ago by
Keywords: | 034-removed-20180328 added |
---|
Per our triage process, these tickets are pending removal from 0.3.4.
comment:4 Changed 20 months ago by
Cc: | neel@… added |
---|---|
Owner: | set to neel |
Status: | new → assigned |
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 20 months ago by
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 20 months ago by
Attachment: | b24734-001.patch added |
---|
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)
comment:7 Changed 20 months ago by
Status: | assigned → needs_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 20 months ago by
Attachment: | b24734-002.patch added |
---|
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 1)
comment:8 Changed 20 months ago by
I have a revised patch which initializes the ap
value: b24734-002.patch
.
comment:9 Changed 20 months ago by
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, andtor_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 20 months ago by
Milestone: | Tor: 0.3.4.x-final → Tor: 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 follow-up: 13 Changed 20 months ago by
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 20 months ago by
Milestone: | Tor: unspecified → Tor: 0.3.4.x-final |
---|
comment:13 Changed 20 months ago by
Replying to neel:
I have one question: should I place
tor_assert(ap)
before I initializeap
, after, or just removetor_assert(ap)
completely?
You must assert that ap is not null before you initialise it.
Changed 20 months ago by
Attachment: | b24734-003.patch added |
---|
[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 3)
comment:15 Changed 20 months ago by
Status: | needs_revision → needs_review |
---|
comment:17 Changed 20 months ago by
Reviewer: | → mikeperry |
---|
comment:18 follow-up: 19 Changed 20 months ago by
Status: | needs_review → merge_ready |
---|
Ok I took Neel's patches and put them in a branch. I also noticed a couple logic issues:
- 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.
- 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 follow-up: 20 Changed 20 months ago by
Replying to mikeperry:
Ok I took Neel's patches and put them in a branch. I also noticed a couple logic issues:
- 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.
- 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 Changed 20 months ago by
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:
- 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.
- 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 20 months ago by
Resolution: | → implemented |
---|---|
Status: | merge_ready → closed |
Squashed, tested, and merged to master!
(But next time let's use github so we get travis to run our tests too.)
The 0.3.3 freeze deadline has passed, all these children of #24403 belong in 0.3.4