Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto: TestDialUnix and unnamed socket name in Go #31413

Closed
Helflym opened this issue Apr 11, 2019 · 5 comments
Closed

x/crypto: TestDialUnix and unnamed socket name in Go #31413

Helflym opened this issue Apr 11, 2019 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Helflym
Copy link
Contributor

Helflym commented Apr 11, 2019

Hi all,

I'm trying to port x/crypto/ssh/test for aix/ppc64 and I've a problem with the test TestDialUnix.
This test is creating a unix socket and will try to dial/accept before checking which addresses are returned.

The remote address after the accept (ie on the server side) both on Linux and AIX seems to be a unnamed socket (if I correctly understand the principle of unnamed socket which is a socket with no address).
However, on Linux, the result is an address named "@".

On Linux,
This comes from syscall.anyToSockaddr:
(gdb) bt
#0 syscall.anyToSockaddr (rsa=0xc000034c90, ~r1=..., ~r2=...) at /home/chigotc/go/goroot/src/syscall/syscall_linux.go:450
#1 0x000000000048a2ee in syscall.Accept4 (fd=3, flags=526336, nfd=, sa=..., err=...) at /home/chigotc/go/goroot/src/syscall/syscall_linux.go:548
#2 0x000000000049a0bf in internal/poll.accept (s=3, ~r1=, ~r2=..., ~r3=..., ~r4=...) at /home/chigotc/go/goroot/src/internal/poll/sock_cloexec.go:17
#3 0x00000000004996d6 in internal/poll.(*FD).Accept (fd=0xc0000bc000, ~r0=, ~r1=..., ~r2=..., ~r3=...) at /home/chigotc/go/goroot/src/internal/poll/fd_unix.go:377
#4 0x00000000004c9782 in net.(*netFD).accept (fd=0xc0000bc000, netfd=, err=...) at /home/chigotc/go/goroot/src/net/fd_unix.go:238
#5 0x00000000004e1102 in net.(*UnixListener).accept (ln=0xc000093350, ~r0=, ~r1=...) at /home/chigotc/go/goroot/src/net/unixsock_posix.go:162
#6 0x00000000004dfcb8 in net.(*UnixListener).Accept (l=0xc000093350, ~r0=..., ~r1=...) at /home/chigotc/go/goroot/src/net/unixsock.go:260
#7 0x00000000004eae3f in main.testDial.func1 (l=..., x=..., testData=...) at /home/chigotc/go/goprogs/dialunnamed.go:37
(gdb) p *rsa
$5 = {Addr = {Family = 1, Data = '\000' <repeats 13 times>}, Pad = '\000' <repeats 95 times>

In Linux's anyToSockaddr, if the first character of the path is '\0', this one is being transformed into "@".
Therefore, the final sockaddr looks like:
(gdb) p *sa
$8 = {Name = 0x61a2c0 "@", raw = {Family = 0, Path = '\000' <repeats 107 times>}}
Looking at the comment, this transformation is made for abstract unix sockets, but it seems to also impact unnamed sockets.

On aix/ppc64, we also have rsa with an empty data.
However, without any changes the final sockaddr will have an empty Name:
(gdb) p *sa
$2 = {Name = 0x0 "", raw = {Len = 0 '\000', Family = 0 '\000', Path = '\000' <repeats 1022 times>}}
The test will therefore fail as it want explicitly the address to be "@".

I don't know how BSD Oses are reacting in this case. Is the kernel already returning "@" ? Or is it fixed somewhere in the code ? I didn't find anything.

But more important, is Go expected unnamed socket to be named "@" or "" ?

Note that AIX doesn't support abstract unix socket. But if I'm transforming the "" address to a "@" address, everything seems to work.

Ps: This can be reproduced without ssh package actually. It's just that ssh tests explicitly check that the address returned is "@".

@bradfitz @hanwen

@gopherbot gopherbot added this to the Unreleased milestone Apr 11, 2019
@hanwen
Copy link
Contributor

hanwen commented Apr 11, 2019

I can't remember why this is here. Could you loop in the original author , see here

https://go-review.googlesource.com/c/crypto/+/38614/7

The protocol spec that this is supposed to implement doesn't mention the "@", so the test is probably overdoing things.
(see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL#L235)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2019
@Helflym
Copy link
Contributor Author

Helflym commented Apr 11, 2019

I'll try to reach him out. It might add it because it was only tested on Linux which forces this "@".
But I'm still wondering how can this test passed on BSD builders ?? Are they run as root and therefore skipped ?

@hanwen
Copy link
Contributor

hanwen commented Apr 11, 2019 via email

@Helflym
Copy link
Contributor Author

Helflym commented Apr 11, 2019

Akihiro's answer:

However, on Linux, an empty address is transformed to "@" because of abstract unix socket handler in syscall package. Was it the reason behind this check ?

Yes, I think that was the reason, but please feel free to open a PR for changing the address to empty if @ is problematic

I'll change the test in order to have both "@" and "" accepted and the same time I'll submit the CL for AIX.

@gopherbot
Copy link

Change https://golang.org/cl/171917 mentions this issue: crypto: add port for aix/ppc64

bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 25, 2020
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Fixes golang/go#31413

Change-Id: I52105280a2237f23cd91b8ec92fd89cf62564572
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants