Opened 16 months ago
Closed 12 months ago
#23873 closed enhancement (implemented)
Remove the return value of node_get_prim_orport()
Reported by: | teor | Owned by: | |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.3.4.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | technical-debt |
Cc: | neel@… | Actual Points: | |
Parent ID: | #24403 | Points: | 1 |
Reviewer: | Sponsor: |
Description (last modified by )
Also, we don't clear the address and port when we fail.
This probably doesn't matter right now, but it matters as soon as we try to change node_get_prim_orport().
We don't check it, so let's remove it, and check for the null address instead.
Child Tickets
Attachments (3)
Change History (19)
comment:1 Changed 14 months ago by
Parent ID: | → #23975 |
---|
comment:2 Changed 14 months ago by
Parent ID: | #23975 → #24403 |
---|
This is something we can do as part of the address refactor in #24403.
comment:3 Changed 14 months ago by
Description: | modified (diff) |
---|---|
Summary: | Tor often forgets to check the return value of node_get_prim_orport() → Remove the return value of node_get_prim_orport() |
#23874 unconditionally clears the address, so all that is left here is to remove the return value.
comment:4 Changed 13 months ago by
Milestone: | Tor: 0.3.3.x-final → Tor: 0.3.4.x-final |
---|
The 0.3.3 freeze deadline has passed, all these children of #24403 belong in 0.3.4
comment:5 follow-up: 6 Changed 13 months ago by
In node_get_prim_orport()
, if a ORPort exists, the return value comes from RETURN_IPV4_AP()
. It seems RETURN_IPV4_AP()
is a macro which returns 0 on succes. However, there is a node_get_prim_dirport()
which also calls RETURN_IPV4_AP()
for a return value. Should I also modify node_get_prim_dirport()
(remove the return value and modify functions depending on it to check for a NULL
address)?
comment:6 Changed 13 months ago by
Replying to neel:
In
node_get_prim_orport()
, if a ORPort exists, the return value comes fromRETURN_IPV4_AP()
. It seemsRETURN_IPV4_AP()
is a macro which returns 0 on succes. However, there is anode_get_prim_dirport()
which also callsRETURN_IPV4_AP()
for a return value. Should I also modifynode_get_prim_dirport()
(remove the return value and modify functions depending on it to check for aNULL
address)?
Yes, you should modify both the ORPort and DirPort functions.
The functions that call them need to check the returned address using "tor_addr_port_is_valid()". Most callers will pass 0 for "for_listening", because they will be connecting to the address, rather than listening on it,
Most of the callers already do a validity check, or they ignore invalid addresses later on.
We should make sure each function handles invalid addresses correctly.
comment:7 Changed 13 months ago by
Cc: | neel@… added |
---|
Changed 13 months ago by
Attachment: | b23873-p001.diff added |
---|
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 1)
comment:9 Changed 13 months ago by
Status: | new → needs_revision |
---|---|
Type: | defect → enhancement |
Hi, thanks for this patch!
The functions that call them need to check the returned address using "tor_addr_port_is_valid()". Most callers will pass 0 for "for_listening", because they will be connecting to the address, rather than listening on it,
Please revise the patch to use tor_addr_port_is_valid() not tor_addr_is_null().
These checks are around the wrong way, they should say !tor_addr_port_is_valid()
:
} else if (node->ipv6_preferred || !tor_addr_is_null(&ipv4_addr.addr)) {
} else if (!tor_addr_is_null(&ipv4_addr.addr)
Changed 13 months ago by
Attachment: | b23873-p002.diff added |
---|
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 2)
comment:11 Changed 13 months ago by
These checks are around the wrong way, they should say !tor_addr_port_is_valid(), because we want to use IPv6 when there is no valid IPv4:
} else if (node->ipv6_preferred || tor_addr_port_is_valid_ap(&ipv4_addr, 0)) {
} else if (tor_addr_port_is_valid_ap(&ipv4_addr, 0)
Changed 13 months ago by
Attachment: | b23873-p003.diff added |
---|
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 3)
comment:13 Changed 13 months ago by
Status: | needs_revision → needs_review |
---|
Thanks!
Someone should review and compile this patch in the next few weeks.
comment:14 Changed 13 months ago by
Status: | needs_review → merge_ready |
---|
Thanks, neel! This patch looks correct to me. Let's take it early in the 0.3.4.x series.
The only change that I would suggest is that the function documentation should say that callers should check whether the result is valid before using it. Either you can make that change, or I'll do it when I merge; either way is fine with me.
comment:15 Changed 12 months ago by
This patch looks fine to me.
We can merge it as long as it compiles and passes the unit tests.
comment:16 Changed 12 months ago by
Resolution: | → implemented |
---|---|
Status: | merge_ready → closed |
wfm; merging.
Maybe we should just remove the return value as part of the consistency changes in #23975.