Opened 3 years ago

Closed 7 months ago

#18105 closed enhancement (implemented)

Replace getsockname with tor_getsockname

Reported by: teor Owned by: eewayhsu
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, intro, api, code-simplification
Cc: Actual Points:
Parent ID: Points: small
Reviewer: dgoulet Sponsor:

Description

There's a lot of duplicate code in Tor that calls getsockname, then stuffs the address in a tor_addr_t.

Let's cleanup that code by replacing it with tor_getsockname where that makes sense.

For example, in #18100, we left behind duplicate code in destination_from_socket, because it was a backport, and the changes required to deduplicate it were complex.

Child Tickets

Attachments (2)

0001-changed-getsockname-calls-to-tor_getsockname.patch (6.0 KB) - added by callumw 11 months ago.
renamed all references to getsockname() to use the tor_getsockname() wrapper function
0001-implemented-new-method-tor_addr_from_getsockname.patch (2.1 KB) - added by callumw 10 months ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

It is impossible that we will fix all 277 currently open 028 tickets before 028 releases. Time to move some out. This is my first pass through the "new" and "reopened" tickets, looking for things to move to ???.

comment:2 Changed 3 years ago by nickm

Keywords: intro added

comment:3 Changed 3 years ago by nickm

Points: small

comment:4 Changed 3 years ago by eewayhsu

Owner: set to eewayhsu
Status: newassigned

comment:5 Changed 23 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:6 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:7 Changed 16 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:8 Changed 15 months ago by nickm

Keywords: api code-simplification added

Changed 11 months ago by callumw

renamed all references to getsockname() to use the tor_getsockname() wrapper function

comment:9 Changed 11 months ago by callumw

Status: assignedneeds_review

comment:10 Changed 11 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:11 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

I don't think that's what we're going for here. The idea was to change tor_getsockname, or to create a new wrapper for tor_getsockname, so that it would put its result into a tor_addr_t rather than into a struct sockaddr.

comment:12 Changed 11 months ago by callumw

Okay I see what is needed now. I think I dove into this ticket without fully understanding it. Cheers.

comment:13 Changed 11 months ago by callumw

Would this suffice for a new tor_getsockname function?

MOCK_IMPL(int,
tor_getsockname,(tor_socket_t sock, struct sockaddr *sock_address,

tor_addr_t *tor_address, socklen_t *address_len))

{

if (getsockname(sock, address, address_len) < 0) { return -1 }

return tor_addr_from_sockaddr(&tor_address, (struct sockaddr*)&sock_address, NULL);

}

I have done it this way so that each call to tor_getsockname can be dealt with individually as they are now with getsockname.

comment:14 Changed 11 months ago by nickm

The idea would be to have a new, different function that only outputs the tor_addr_t -- not the sockaddr or the address_len.

comment:15 in reply to:  14 ; Changed 11 months ago by callumw

Replying to nickm:

The idea would be to have a new, different function that only outputs the tor_addr_t -- not the sockaddr or the address_len.

So would we want to pass the sockaddr and address_len by value instead of by reference? Otherwise I do not know another way to derive the tor_addr_t.

Cheers

comment:16 Changed 10 months ago by callumw

I am finding it challenging to figure out the best way to solve this problem. I think that's perhaps due to my lack of understanding of the code-base and the context of the enhancement. Is there a good way for me to get assistance with this?

Thanks

comment:17 in reply to:  15 ; Changed 10 months ago by teor

Replying to callumw:

Replying to nickm:

The idea would be to have a new, different function that only outputs the tor_addr_t -- not the sockaddr or the address_len.

So would we want to pass the sockaddr and address_len by value instead of by reference? Otherwise I do not know another way to derive the tor_addr_t.

Hi, thanks for your persistence, sorry it has taken a while for someone to reply.

struct sockaddr_storage is large enough to hold an IPv4 or IPv6 address. So we can pass a local variable of that type to getsockname(), even if we don't know the size of the address. On return, it will have the right address in it.

Similarly, we can initialise a local socklen_t variable to the size of sockaddr_storage. On return, it will have the right size in it.

Then, we can call tor_addr_from_sockaddr() to populate the tor_addr_t. And it turns out we don't even need the size for this.

Does that help?

comment:18 Changed 10 months ago by teor

(By the way, I always forget how low-level unix functions work. The man pages are usually a good reference, but man getsockname is missing info about sockaddr_storage, at least on my (macOS) system.)

comment:19 in reply to:  17 Changed 10 months ago by callumw

Replying to teor:

Hi, thanks for your persistence, sorry it has taken a while for someone to reply.

struct sockaddr_storage is large enough to hold an IPv4 or IPv6 address. So we can pass a local variable of that type to getsockname(), even if we don't know the size of the address. On return, it will have the right address in it.

Similarly, we can initialise a local socklen_t variable to the size of sockaddr_storage. On return, it will have the right size in it.

Then, we can call tor_addr_from_sockaddr() to populate the tor_addr_t. And it turns out we don't even need the size for this.

Does that help?

Thanks! This has clarified the issue much more.

I have implemented the function as you suggested and will submit another patch when I can. For now I have hit a compilation problem when attempting to include address.h in compat.h so that the type tor_addr_t can be used in the new function

comment:20 Changed 10 months ago by teor

Ah, C header dependencies. Not fun.

That probably means that this new function:

  • should be in address.c, and
  • should call tor_getsockname(), rather than replacing it.

How about tor_addr_from_getsockname()?
That's a much better name anyway, because it describes exactly what the function does.

comment:21 Changed 10 months ago by callumw

Thanks for the advice regarding the header files. I moved the function to address.c as you recommended and that resolved the issue.

I have attached a new patch file with the new function implementation in. The first test under 'make check' fails but this behaviour can be replicated in a newly checkout-out repo so I assume it is unrelated to my changes here.

comment:22 Changed 10 months ago by nickm

Status: needs_revisionneeds_review

comment:23 in reply to:  21 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

Hi,

Thanks for this revision!

Replying to callumw:

Thanks for the advice regarding the header files. I moved the function to address.c as you recommended and that resolved the issue.

I have attached a new patch file with the new function implementation in. The first test under 'make check' fails but this behaviour can be replicated in a newly checkout-out repo so I assume it is unrelated to my changes here.

make check-spaces will fail due to this line:

if(getsockname(sock, (struct sockaddr *)&sockaddr, &addr_len) < 0){ return -1; }

We have a coding style that puts spaces between if and (, and ) and { and we typically have if blocks on multiple lines. Try to match the existing coding style, and run make check-spaces to confirm.

Code review:

I think we'd be ok with adding .vimrc to .gitignore, but we would want that in a separate commit. Alternately, you can add .vimrc to your own ~/.gitignore_global (there's a git config option for a global ignore file).

We don't need to do the MOCK_IMPL/MOCK_DECL on these functions, because they're not being mocked in the unit tests.

We should use tor_getsockname() in tor_addr_from_getsockname(), so we can mock tor_getsockname() in the unit tests if we need to.

tor_addr_from_sockaddr() returns a port in port_out. So uint16_t port_num needs to be a pointer, and should probably be called something like port_out. Similarly, tor_addr_t *tor_addr should probably be called addr_out. And our convention is to put out parameters at the end of the argument list.

The function needs a comment that describes what each argument does, and what the return value is. You can use the one on tor_addr_from_sockaddr() as an example.

Next Steps:

Once we introduce this function, we can use it to replace code that calls [tor_]getsockname() then tor_addr_from_sockaddr().

comment:24 Changed 8 months ago by nickm

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

This has been sitting in needs_revision for a while, so I'll pick it up. See bug18105 in my public repo. I didn't add a port_out argument, because none of the callers of this function actually care about the port.

comment:25 Changed 8 months ago by nickm

Keywords: review-group-31 added

comment:26 Changed 8 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

lgtm;

comment:27 Changed 8 months ago by nickm

Keywords: review-group-31 removed

Remove merge_ready items that are in 0.3.4 or for backport consideration into 0.3.2 from review-group-31

comment:28 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging to master (which is now 0.3.4)

Note: See TracTickets for help on using tickets.