Opened 11 months ago

Closed 7 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 teor)

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)

b23873-p001.diff (5.5 KB) - added by neel 8 months ago.
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 1)
b23873-p002.diff (5.5 KB) - added by neel 8 months ago.
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 2)
b23873-p003.diff (5.5 KB) - added by neel 8 months ago.
Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 3)

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 months ago by teor

Parent ID: #23975

Maybe we should just remove the return value as part of the consistency changes in #23975.

Last edited 10 months ago by teor (previous) (diff)

comment:2 Changed 9 months ago by teor

Parent ID: #23975#24403

This is something we can do as part of the address refactor in #24403.

comment:3 Changed 9 months ago by teor

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 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:5 Changed 8 months ago by neel

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 in reply to:  5 Changed 8 months ago by teor

Replying to neel:

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

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 8 months ago by neel

Cc: neel@… added

Changed 8 months ago by neel

Attachment: b23873-p001.diff added

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 1)

comment:8 Changed 8 months ago by neel

I have a patch for this in b23873-p001.diff.

comment:9 Changed 8 months ago by teor

Status: newneeds_revision
Type: defectenhancement

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 8 months ago by neel

Attachment: b23873-p002.diff added

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 2)

comment:10 Changed 8 months ago by neel

A new patch is available as b23873-p002.diff​.

comment:11 Changed 8 months ago by teor

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 8 months ago by neel

Attachment: b23873-p003.diff added

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 3)

comment:12 Changed 8 months ago by neel

I have made the requested changes in the file b23873-p003.diff.

comment:13 Changed 8 months ago by teor

Status: needs_revisionneeds_review

Thanks!
Someone should review and compile this patch in the next few weeks.

comment:14 Changed 8 months ago by nickm

Status: needs_reviewmerge_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 7 months ago by teor

This patch looks fine to me.
We can merge it as long as it compiles and passes the unit tests.

comment:16 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

wfm; merging.

Note: See TracTickets for help on using tickets.