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/sys/unix: SockaddrL2 byte order mismatch #39045

Open
jpas opened this issue May 13, 2020 · 3 comments
Open

x/sys/unix: SockaddrL2 byte order mismatch #39045

jpas opened this issue May 13, 2020 · 3 comments

Comments

@jpas
Copy link

@jpas jpas commented May 13, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.1 linux/amd64

Does this issue reproduce with the latest release?

It reproduces on the latest version of x/sys.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jpas/.cache/go-build"
GOENV="/home/jpas/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jpas/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/nix/store/gvw1mfpdrk7i82884yhxf9lf5j3c12zm-go-1.14.1/share/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/nix/store/gvw1mfpdrk7i82884yhxf9lf5j3c12zm-go-1.14.1/share/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="/nix/store/k930l9lckxjf4zmafv4aapvdbd2sibmj-gcc-wrapper-9.3.0/bin/cc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build011582959=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"github.com/pkg/errors"
	. "golang.org/x/sys/unix"
	"os"
	"os/exec"
	"strconv"
)

func bug(addr [6]uint8) error {
	fd, err := Socket(AF_BLUETOOTH, SOCK_SEQPACKET, BTPROTO_L2CAP)
	if err != nil {
		return err
	}
	defer Close(fd)

	saIn := SockaddrL2{Addr: addr, PSM: 17}
	if err := bind(fd, &saIn); err != nil {
		return err
	}

	pp, err := Getsockname(fd)
	if err != nil {
		return err
	}

	saOut := pp.(*SockaddrL2)
	fmt.Printf("expected: %#v\n", saIn.Addr)
	fmt.Printf("actual  : %#v\n", saOut.Addr)

	return nil
}

func bind(fd int, sa *SockaddrL2) error {
	// We try to bind twice since bluez's input plugin may cause this address to
	// already be in use.
	if Bind(fd, sa) == nil {
		return nil
	}

	// By restarting the bluetooth service, we introduce a race between our program
	// and bluez to bind.
	exec.Command("systemctl", "restart", "bluetooth").Run()

	return Bind(fd, sa)
}

func parseBdAddr(addr string) ([6]uint8, error) {
	var bd [6]uint8

	for i := range bd {
		j := 3 * i
		b, err := strconv.ParseUint(addr[j:j+2], 16, 8)
		if err != nil {
			return bd, errors.Wrap(err, "addr malformed")
		}
		bd[i] = uint8(b)
	}

	return bd, nil
}

func main() {
	if os.Getuid() != 0 {
		fmt.Println("please run as root")
		os.Exit(1)
	}

	if len(os.Args) != 2 {
		fmt.Printf("usage: %s 11:22:33:44:55:66\n", os.Args[0])
		os.Exit(1)
	}

	addr, err := parseBdAddr(os.Args[1])
	if err != nil {
		fmt.Println(err)
		os.Exit(1)
	}

	if err := bug(addr); err != nil {
		fmt.Println(err)
		os.Exit(1)
	}
}

There are some underlying issues with the "input" plugin for Bluez and binding to Bluetooth sockets. They can be mitigated by restarting the bluetooth daemon and racing to bind the socket.

What did you expect to see?

expected: [6]uint8{0x80, 0x32, 0x53, 0x37, 0x22, 0x19}
actual  : [6]uint8{0x80, 0x32, 0x53, 0x37, 0x22, 0x19}

What did you see instead?

expected: [6]uint8{0x80, 0x32, 0x53, 0x37, 0x22, 0x19}
actual  : [6]uint8{0x19, 0x22, 0x37, 0x53, 0x32, 0x80}

Change proposals

Remove the three lines reversing the order of the bytes, which would mean that SockaddrL2.Addr will be expected to be in network order, not the order that it would usually appear as a string.

Alternatively, we could add the same three lines to anyToSockaddr here, so that the order is the same as what is expected as input to Bind and Connect.

The first option seems like the better option to me since it is in line with RawSockaddrL2 and the underlying syscall API. But the second one may be "more friendly" as the byte order matches the string's ordering.

@gopherbot gopherbot added this to the Unreleased milestone May 13, 2020
@cagedmantis cagedmantis changed the title x/sys: SockaddrL2 byte order mismatch x/sys/unix: SockaddrL2 byte order mismatch May 18, 2020
@cagedmantis cagedmantis modified the milestones: Unreleased, Backlog May 20, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented May 20, 2020

@gopherbot
Copy link

@gopherbot gopherbot commented May 22, 2020

Change https://golang.org/cl/234997 mentions this issue: unix: don't swap byteorder of SockaddrL2.Addr

@gkelly
Copy link

@gkelly gkelly commented May 22, 2020

Change proposals

Remove the three lines reversing the order of the bytes, which would mean that SockaddrL2.Addr will be expected to be in network order, not the order that it would usually appear as a string.

Alternatively, we could add the same three lines to anyToSockaddr here, so that the order is the same as what is expected as input to Bind and Connect.

The first option seems like the better option to me since it is in line with RawSockaddrL2 and the underlying syscall API. But the second one may be "more friendly" as the byte order matches the string's ordering.

I agree that there's shouldn't be a byteorder swap in https://github.com/golang/sys/blob/master/unix/syscall_linux.go#L502-L504. The host endianness isn't accounted for, so the correct behavior is to let this be a caller problem instead of doing the order determination and swap.

I have uploaded https://go-review.googlesource.com/c/sys/+/234997 to address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants