Opened 2 years ago

Last modified 4 weeks ago

#18105 needs_revision enhancement

Replace getsockname with tor_getsockname

Reported by: teor Owned by: eewayhsu
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, intro, api, code-simplification
Cc: Actual Points:
Parent ID: Points: small
Reviewer: 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 8 weeks 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 4 weeks ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 23 months 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 23 months ago by nickm

Keywords: intro added

comment:3 Changed 23 months ago by nickm

Points: small

comment:4 Changed 23 months ago by eewayhsu

Owner: set to eewayhsu
Status: newassigned

comment:5 Changed 13 months ago by teor

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

Milestone renamed

comment:6 Changed 12 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 7 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:8 Changed 6 months ago by nickm

Keywords: api code-simplification added

Changed 8 weeks ago by callumw

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

comment:9 Changed 8 weeks ago by callumw

Status: assignedneeds_review

comment:10 Changed 8 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:11 Changed 8 weeks 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 7 weeks 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 6 weeks 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 6 weeks 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 6 weeks 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 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks 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 4 weeks ago by nickm

Status: needs_revisionneeds_review

comment:23 in reply to:  21 Changed 4 weeks 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().

Note: See TracTickets for help on using tickets.