Replace getsockname with tor_getsockname

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 legacy/trac#18100 (moved), we left behind duplicate code in destination_from_socket, because it was a backport, and the changes required to deduplicate it were complex.

changed milestone to %Tor: 0.3.4.x-final in legacy/trac

added api in Legacy / Trac code-simplification in Legacy / Trac component::core tor/tor in Legacy / Trac easy in Legacy / Trac intro in Legacy / Trac milestone::Tor: 0.3.4.x-final in Legacy / Trac owner::eewayhsu in Legacy / Trac points::small in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::dgoulet in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::enhancement in Legacy / Trac labels

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

Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.???

Trac:
Keywords: easy deleted, easy intro added

Trac:
Points: N/A to small

Trac:
Username: eewayhsu
Status: new to assigned
Owner: N/A to eewayhsu

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

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

Trac:
Milestone: Tor: 0.3.??? to Tor: unspecified
Keywords: easy intro deleted, tor-03-unspecified-201612, intro, easy added

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

Trac:
Reviewer: N/A to N/A
Keywords: N/A deleted, api, code-simplification added

Trac:
Username: callumw

0001-changed-getsockname-calls-to-tor_getsockname.patch

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

Trac:
Username: callumw

Trac:
Username: callumw
Status: assigned to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.3.3.x-final

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Username: 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.

Trac:
Username: callumw

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

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

Trac:
Username: 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

Trac:
Username: callumw

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?

(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.)

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

Trac:
Username: callumw

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.

Trac:
Username: callumw

0001-implemented-new-method-tor_addr_from_getsockname.patch

Trac:
Username: 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.

Trac:
Username: callumw

Trac:
Status: needs_revision to needs_review

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().

Trac:
Status: needs_review to needs_revision

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.

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final
Status: needs_revision to needs_review

Trac:
Keywords: N/A deleted, review-group-31 added

lgtm;

Trac:
Status: needs_review to merge_ready
Reviewer: N/A to dgoulet

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

Trac:
Keywords: review-group-31 deleted, N/A added

Merging to master (which is now 0.3.4)

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

moved from legacy/trac#18105 (moved)

added Feature First Contribution labels and removed 1 deleted label

removed 1 deleted label