Opened 7 weeks ago
Closed 6 weeks 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 7 weeks ago by
Status: | new → needs_review |
---|
comment:2 follow-up: 5 Changed 6 weeks ago by
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 6 weeks ago by
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 6 weeks ago by
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 follow-up: 7 Changed 6 weeks ago by
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 6 weeks ago by
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 Changed 6 weeks ago by
comment:8 Changed 6 weeks ago by
Reviewer: | → nickm, teor |
---|---|
Status: | needs_review → merge_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:10 Changed 6 weeks ago by
Milestone: | Tor: 0.4.0.x-final → Tor: 0.3.5.x-final |
---|---|
Resolution: | → fixed |
Status: | merge_ready → closed |
PR: https://github.com/torproject/tor/pull/631