x/sys/unix: ioctl wrapper signatures do not match C on Solaris and AIX

[nshalman]

(Can be assigned to me unless some kind soul has the bandwidth to work on it before me.)

Copied from WireGuard/wireguard-go#39 (comment)

It looks like ioctl has the wrong signature in Go. According to https://illumos.org/man/2/ioctl req is an int. In Go, that's int32, not int or uint as the Go code has now. Presumably it's too late to fix that, which is unfortunate.

Is it too late?

Regarding ioctls using negative numbers:

The easiest way to do this is probably just by adding & 0xffffffff onto the end of the constant definition.
(The lazy version, by the way, is just unix.IoctlLifreq(tun.ipfd, unix.SIOCGLIFMTU & 0xffffffff, &l)).

Affected functions:

$ git grep "req uint" unix/syscall_solaris.go
unix/syscall_solaris.go://sys   ioctlRet(fd int, req uint, arg uintptr) (ret int, err error) = libc.ioctl
unix/syscall_solaris.go://sys   ioctlPtrRet(fd int, req uint, arg unsafe.Pointer) (ret int, err error) = libc.ioctl
unix/syscall_solaris.go:func ioctl(fd int, req uint, arg uintptr) (err error) {
unix/syscall_solaris.go:func ioctlPtr(fd int, req uint, arg unsafe.Pointer) (err error) {
unix/syscall_solaris.go:func IoctlSetTermio(fd int, req uint, value *Termio) error {
unix/syscall_solaris.go:func IoctlGetTermio(fd int, req uint) (*Termio, error) {
unix/syscall_solaris.go:func IoctlSetIntRetInt(fd int, req uint, arg int) (int, error) {
unix/syscall_solaris.go:func IoctlSetString(fd int, req uint, val string) error {
unix/syscall_solaris.go:func IoctlLifreq(fd int, req uint, l *Lifreq) error {
unix/syscall_solaris.go:func IoctlSetStrioctlRetInt(fd int, req uint, s *Strioctl) (int, error) {

To anyone labeling this issue, this affects both solaris and illumos.

While I'm likely going to be the one doing the work, I could definitely use some guidance on what approach will make the most sense.

[bcmills]

(CC @golang/solaris @golang/illumos)

[cherrymui]

Do you propose to change the signature of the Ioctl functions on Solaris/Illomos?

cc @ianlancetaylor for whether this type of API change is acceptable.

[ianlancetaylor]

There is no reason for Go function signatures to precisely match C function signatures. It's true that the C function takes a 32-bit integer, but it's fine for the Go function to take a 32- or 64-bit integer depending on the platform. Either way you have to pass a magic constant anyhow. The Go code does the right thing when invoking the system call.

Closing because I don't think there is anything to do. Please comment if you disagree.

[nshalman]

From a usability and readability perspective, it seems to me like one would expect to be able to write

err := unix.IoctlLifreq(tun.ipfd, unix.SIOCGLIFMTU, &l)

I'm fairly certain, at least for that ioctl, that my code is the only consumer.
Is it too late to change things to make that possible?

The following feels very clunky (as elaborated here: WireGuard/wireguard-go#39 (comment)) :

reqnum := int(unix.SIOCGLIFMTU)
err := unix.IoctlLifreq(tun.ipfd, uint(reqnum), &l)

I suppose the alternative is to recommend something like:

// some helpful comment explaining why this is necessary
SIOCGLIFMTU := unix.SIOCGLIFMTU & 0xffffffff
// ...
err := unix.IoctlLifreq(tun.ipfd, SIOCGLIFMTU, &l)

But that too feels kind of gross.

I do agree with the reviewer of my other code that the two lines including the extra assignment to do the ioctl looks gross.

If nothing else, it would be nice to clean up the test case with at least a better explanation and ideally a cleaner function call.

[nshalman]

Sorry, to be explicit, by "the test case" I mean TestLifreqGetMTU
and by the comment I mean (https://github.com/golang/sys/blob/master/unix/syscall_solaris_test.go#L370-L372)

	// SIOCGLIFMTU is negative which confuses the compiler if used inline:
	// Using "unix.IoctlLifreq(ip_fd, unix.SIOCGLIFMTU, &l)" results in
	// "constant -1065850502 overflows uint"

[cherrymui]

Maybe we could change unix.SIOCGLIFMTU to be a positive number that fits in a uint?

[bcmills]

Heh. Today I learned that the POSIX ioctl in <stropts.h> doesn't match the Linux ioctl in <sys/ioctl.h>. POSIX specifies the (signed) int signature, whereas Linux defines it as unsigned long.

Actually, https://www.gnu.org/software/gnulib/manual/html_node/ioctl.html links to three different LSB specifications, two of which use int and one of which uses unsigned long. 😵‍💫

The Solaris and Illumos system calls defined in C match the POSIX signature and header file. So I agree that the signatures in x/sys/unix seem to be wrong, but I also agree with @zx2c4 that it's probably too late to fix that. 😞

[nshalman]

Maybe we could change unix.SIOCGLIFMTU to be a positive number that fits in a uint?

It gets defined in unix/zerrors_solaris_amd64.go which is auto-generated by unix/mkerrors.sh so that might be tricky.

[cherrymui]

The script can be changed, if we decide to change the constant.

[nshalman]

Heh. Today I learned that the POSIX ioctl in <stropts.h> doesn't match the Linux ioctl in <sys/ioctl.h>. POSIX specifies the (signed) int signature, whereas Linux defines it as unsigned long.

Thank you for looking into this.

Actually, https://www.gnu.org/software/gnulib/manual/html_node/ioctl.html links to three different LSB specifications, two of which use int and one of which uses unsigned long. 😵‍💫

The Solaris and Illumos system calls defined in C match the POSIX signature and header file. So I agree that the signatures in x/sys/unix seem to be wrong, but I also agree with @zx2c4 that it's probably too late to fix that. 😞

Any suggestions? @cherrymui suggested we could change the way mkerrors.sh defines the constants. Would doing that be backwards compatible with existing code? (My experiments in the go playground suggest not, but I could be doing it wrong...)

[bcmills]

Neither change would be entirely compatible, unfortunately. I'm not sure whether it's worth a breaking change to fix, but in my opinion if it is worthwhile it would be cleaner to change the signature on Solaris to match the C system call signature than to give the constant a value that isn't exactly equal to the value of the C constant.

[zx2c4]

You mean change req uint into req int? Yea, seems reasonable.

[nshalman]

Some of these ioctls are very very very likely only in use by my code. It should be fine to do a breaking change on those (in my opinion.)
If anyone else is using them, they will probably appreciate the cleanup that the breaking change allows.

[nshalman]

For the Lifreq, it could look something like this?

diff --git a/unix/syscall_solaris.go b/unix/syscall_solaris.go
index d3444b6..eb01da4 100644
--- a/unix/syscall_solaris.go
+++ b/unix/syscall_solaris.go
@@ -1120,8 +1120,8 @@ func (l *Lifreq) GetLifruUint() uint {
        return *(*uint)(unsafe.Pointer(&l.Lifru[0]))
 }

-func IoctlLifreq(fd int, req uint, l *Lifreq) error {
-       return ioctlPtr(fd, req, unsafe.Pointer(l))
+func IoctlLifreq(fd int, req int, l *Lifreq) error {
+       return ioctlPtr(fd, uint(req), unsafe.Pointer(l))
 }

 // Strioctl Helpers
diff --git a/unix/syscall_solaris_test.go b/unix/syscall_solaris_test.go
index 6c2b906..dc46bd5 100644
--- a/unix/syscall_solaris_test.go
+++ b/unix/syscall_solaris_test.go
@@ -367,17 +367,13 @@ func TestLifreqGetMTU(t *testing.T) {
        if err != nil {
                t.Fatalf("could not open udp socket: %v", err)
        }
-       // SIOCGLIFMTU is negative which confuses the compiler if used inline:
-       // Using "unix.IoctlLifreq(ip_fd, unix.SIOCGLIFMTU, &l)" results in
-       // "constant -1065850502 overflows uint"
-       reqnum := int(unix.SIOCGLIFMTU)
        var l unix.Lifreq
        for link, mtu := range tc {
                err = l.SetName(link)
                if err != nil {
                        t.Fatalf("Lifreq.SetName(%q) failed: %v", link, err)
                }
-               if err = unix.IoctlLifreq(ip_fd, uint(reqnum), &l); err != nil {
+               if err = unix.IoctlLifreq(ip_fd, unix.SIOCGLIFMTU, &l); err != nil {
                        t.Fatalf("unable to SIOCGLIFMTU: %v", err)
                }
                m := l.GetLifruUint()

I can open a CR for changing the ioctls that I added for wireguard-go. I'm a little more nervous about any other ones that were added for other purposes.

[gopherbot]

Change https://go.dev/cl/476515 mentions this issue: unix: solaris: match ioctl req argument type to libc type

[bcmills]

Hrm. It appears that this issue also affects aix (CC @golang/aix), and I would expect that we should address it in the same way on both platforms.

Checking various references for ioctl(2), I see

• unsigned long
  • linux
  • darwin
  • freebsd
  • netbsd
  • openbsd
  • dragonfly
  • hurd
• int:
  • aix (apparently declared in K&R syntax 😅)
  • solaris
  • illumos
  • zos

[jclulow]

I believe it is an SVR4 and STREAMS heritage thing, so I would expect to see it in operating systems from that part of the family tree.

[zx2c4]

Updating the CL to include aix and zos.