Fix the IPv6 case of tor_ersatz_socketpair

[kjak]

In get_local_listener which is used by tor_ersatz_socketpair, sin6_family is currently being set to AF_INET in the IPv6 case when AF_INET6 should be used instead.

This code was introduced in commit 9b24609af003cb79091e628c179cf617ff41aae7 from this past August, so this is not a brand-new problem.

I tested this on my OpenBSD box by forcing an IPv6 socket to be used for tor_ersatz_socketpair and running the test suite. There is a test in the test suite that tests tor_ersatz_socketpair: it (of course) fails using the current AF_INET code but it passes when using AF_INET6 instead.

PR to follow to change AF_INET to AF_INET6 in the IPv6 case.

[kjak]

PR: ​https://github.com/torproject/tor/pull/631

[nickm]

Looks good, but it will need a changes file before we can merge it. I can write one, or somebody else can add one: see doc/HACKING/CodingStandards.md, under the heading How we log changes.

I wonder, is this worth backporting to 0.3.5, or should we just take it in 0.4.0? I don't think we actually _call_ this function with AF_INET6 anywhere, so it might not be necessary to backport. But maybe we should backport anyway, to avoid problems down the road if we decide to backport something else that starts using AF_INET6 here.

If we're thinking of a backport, the patch should be based on maint-0.3.5 instead of master.

[kjak]

I think it should be backported. get_local_listener is called with AF_INET6 on IPv6-only systems, so it appears that tor_ersatz_socketpair is broken there (but I have not directly tested on such a system).

I want to avoid making this more complicated than it needs to be, but I'm a bit confused on how this works here. Should I make a new PR based on maint-0.3.5 and keep the current PR (based on master) open? Or should I close the latter one? Or something else?

I can take a crack at making a changes file and including it in whatever PRs are open.

Thanks!

[nickm]

Thanks! A single new PR on maint-0.3.5 with a changes file in it should be just perfect here; if the one against master is redundant, you can feel free to close it.

There are some trivial syntactic correctness checks for changes files -- "make check-changes" (included in make check) will test them for you.

[teor]

Replying to nickm:

I wonder, is this worth backporting to 0.3.5, or should we just take it in 0.4.0? I don't think we actually _call_ this function with AF_INET6 anywhere, so it might not be necessary to backport.

The family argument must be AF_UNIX. It is checked and ignored. Instead, ersatz_domain is passed to get_local_listener().

It took me a while to work this out, because the function comments in socketpair.c are missing or incomplete.
Maybe we should add function comments, using the tor_socketpair() comment as a starting point?
​https://github.com/torproject/tor/blob/701eaef980de4f7dbb5c31c4fee9b7e1e266d7a1/src/lib/net/socket.c#L450

[kjak]

New PR: ​https://github.com/torproject/tor/pull/636

This contains the fix and a changes file (checked with make check-changes), and it is for merging into maint-0.3.5.

I have closed the previous PR (631).

Thanks!

[teor]

Thanks for the patch, someone should review it in the next week or so.

Replying to teor:

It took me a while to work this out, because the function comments in socketpair.c are missing or incomplete.

I opened #29015 to fix this documentation.

[teor]

Oh, actually, this is a one-line patch. It looks fine.

Travis CI has passed, but the Appveyor queue is 13 hours long, because we just did a release. Let's merge once Appveyor has finished.

[nickm]

Appveyor seems to like it; merging to 0.3.5 and forward.