Opened 8 months ago

Closed 8 months ago

#28995 closed defect (fixed)

Fix the IPv6 case of tor_ersatz_socketpair

Reported by: kjak Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.5.1-alpha
Severity: Normal Keywords: 035-backport, ipv6
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm, teor Sponsor:

Description

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.

Child Tickets

Change History (10)

comment:1 Changed 8 months ago by kjak

Status: newneeds_review

comment:2 Changed 8 months ago by nickm

Keywords: 035-backport? added
Milestone: Tor: 0.4.0.x-final

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.

comment:3 Changed 8 months ago by 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!

comment:4 Changed 8 months ago by 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.

comment:5 in reply to:  2 ; Changed 8 months ago by teor

Keywords: 035-backport ipv6 added; 035-backport? removed
Version: Tor: 0.3.5.1-alpha

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

comment:6 Changed 8 months ago by 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!

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

comment:8 Changed 8 months ago by teor

Reviewer: nickm, teor
Status: needs_reviewmerge_ready

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.

comment:9 Changed 8 months ago by nickm

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

comment:10 Changed 8 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.