Skip to content

syscall: Cmsghdr on GNU/Linux has unexpected size #17649

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.

Sign up for GitHub

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

Jump to bottom
Closed
iangudger opened this issue Oct 28, 2016 · 9 comments
Closed

syscall: Cmsghdr on GNU/Linux has unexpected size #17649

iangudger opened this issue Oct 28, 2016 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.8

Comments

@iangudger
Copy link
Contributor

On AMD64, the syscall.Cmsghdr struct is

type Cmsghdr struct {
    Len          uint64
    Level        int32
    Type         int32
    X__cmsg_data [0]uint8
}

Both syscall.UnixRights() and syscall.ParseSocketControlMessage() assume that the zero length array takes no space and that this struct is 16 bytes. This is not the case as since go1.5 trailing zero arrays take 1 byte thus making the struct 24 bytes. We can't change the size of socket control message headers and the X__cmsg_data field doesn't seem to serve any useful purpose, so I suggest we remove the X__cmsg_data field.

Most of the time this isn't a problem and this code is probably correct and safe in practice. It does, however, trip the race detector.

Here is an example:
gorace.go

package gorace

type hdr struct {
    num int64
    X__cmsg_data [0]uint8
}

gorace_test.go

package gorace

import (
    "sync"
    "testing"
    "unsafe"
)

const iterations = 1000

func TestRace(t *testing.T) {
    var before int64
    b := [8]byte{}
    var after int64
    var wg sync.WaitGroup
    wg.Add(3)
    go func() {
        for i := 0; i < iterations; i++ {
            h := (*hdr)(unsafe.Pointer(&b[0]))
            _ = *h
        }
        wg.Done()
    }()
    go func() {
        for i := 0; i < iterations; i++ {
            before++
        }
        wg.Done()
    }()
    go func() {
        for i := 0; i < iterations; i++ {
            after++
        }
        wg.Done()
    }()
    wg.Wait()
}

If one runs this with the race detector enabled, one gets something like this:

==================
WARNING: DATA RACE
Write at 0x00c42000c5f8 by goroutine 9:
  local/gorace.TestRace.func3()
      /home/iangudger/go/src/local/gorace/gorace_test.go:32 +0x67

Previous read at 0x00c42000c5f8 by goroutine 7:
  local/gorace.TestRace.func1()
      /home/iangudger/go/src/local/gorace/gorace_test.go:20 +0x59

Goroutine 9 (running) created at:
  local/gorace.TestRace()
      /home/iangudger/go/src/local/gorace/gorace_test.go:35 +0x1a9
  testing.tRunner()
      /usr/lib/golang/src/testing/testing.go:610 +0xc9

Goroutine 7 (finished) created at:
  local/gorace.TestRace()
      /home/iangudger/go/src/local/gorace/gorace_test.go:23 +0x151
  testing.tRunner()
      /usr/lib/golang/src/testing/testing.go:610 +0xc9
==================
PASS
Found 1 data race(s)
exit status 66
FAIL    local/gorace    1.054s

/cc @rsc
@ianlancetaylor
Copy link
Member

It's not clear to me what your test case shows except that the type hdr is not 8 bytes. I agree that it is not.

But I also agree that the trailing [0]uint8 that appears on all GNU/Linux versions of syscall.Cmsghdr is problematic because it is confusing for people who expect the struct to have the same size as the C type struct cmsghdr. And, that trailing field does not appear for any other OS.

The Go 1 guarantee does not cover the syscall package, but the Go 1 guarantee doc also says that the syscall package is frozen. The current code is problematic and error-prone but is not actually buggy in itself. It's not entirely clear to me whether the Go 1 guarantee permits us to remove it.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Oct 28, 2016
@ianlancetaylor ianlancetaylor changed the title syscall: Incorrect use of unsafe casts for Cmsghdr syscall: Cmsghdr on GNU/Linux has unexpected size Oct 28, 2016
@rsc
Copy link
Contributor

rsc commented Oct 28, 2016

I think UnixRights is fine. It sets a *Cmsghdr to point into a byte slice, but it only reads fields from the Cmsghdr; it does not read the entire struct, so it does not read the padding at the end.

ParseSocketControlMessage is a bigger problem. The "Header: *h" in the middle of the loop does copy too many bytes out of the message, and if the message were right up against a page boundary or another piece of data, you'd get a seg fault or a race detector report.

Two possible fixes:

  1. Delete X__cmsg_data from the struct. This will fix *h but break code that uses &h.X__cmsg_data as the address of the data start.
  2. Change the *h to Cmsghdr{Len: h.Len, Level: h.Level, Type: h.Type}. This will avoid reading the "bad" part of h and not break any other correct uses of Cmsghdr.

I lean toward 2.

@rsc
Copy link
Contributor

rsc commented Oct 28, 2016

/cc @bradfitz for opinion on 1 vs 2

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 28, 2016
@bradfitz
Copy link
Contributor

I don't see any downside with 2.

@ianlancetaylor
Copy link
Member

I'm fine with 2, but just for the record the downside is that this is a lurking trap for anyone else who uses an unsafe.Pointer conversion to copy around a syscall.Cmsghdr value. Even most people looking right at the definition of syscall.Cmsghdr are not going to realize that the copy is going to modify more bytes than they expect.

@iangudger
Copy link
Contributor Author

I searched around, and I couldn't find anyone using X__cmsg_data. It would surprise me if anyone was.

Option 2 may not prevent any code that uses X__cmsg_data from compiling, but I believe that it will still break code that uses &h.X__cmsg_data as the address of the data start.

Removing it is the more correct approach, so I would personally prefer that. I would personally much rather have a compile error than a runtime error.

@bradfitz
Copy link
Contributor

If we broke compatibility and deleted 0-length fields from the ends of structs, would we do them all? e.g.

ztypes_linux_amd64.go-type InotifyEvent struct {
ztypes_linux_amd64.go-  Wd     int32
ztypes_linux_amd64.go-  Mask   uint32
ztypes_linux_amd64.go-  Cookie uint32
ztypes_linux_amd64.go-  Len    uint32
ztypes_linux_amd64.go:  Name   [0]uint8
ztypes_linux_amd64.go-}

@ianlancetaylor
Copy link
Member

We could, but frankly the one in Cmsghdr is a lot more important than the one in InotifyEvent, because it's normal to copy a Cmsghdr value into a buffer.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/32319 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @rsc @ianlancetaylor @iangudger @gopherbot