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.

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

vsock: Use net.FileConn to create VirtioSocketConnection #63

Merged
merged 4 commits into from
Oct 12, 2022
Merged

vsock: Use net.FileConn to create VirtioSocketConnection #63

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea8461c
vsock: Use net.FileConn to create VirtioSocketConnection
cfergeau Sep 16, 2022
8dc8720
vsock: Remove ports from VirtioSocketConnection
cfergeau Oct 11, 2022
ff58ded
vsock: Simplify VirtioSocketConnection implementation
cfergeau Oct 11, 2022
12bf645
vsock: Remove dup/dupCh
cfergeau Sep 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 20 additions & 106 deletions socket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import (
"os"
"runtime"
"runtime/cgo"
"syscall"
"time"
"unsafe"

"golang.org/x/sys/unix"
Expand Down Expand Up @@ -106,11 +104,11 @@ func connectionHandler(connPtr, errPtr, cgoHandlerPtr unsafe.Pointer) {
handler := cgoHandler.Value().(func(*VirtioSocketConnection, error))
defer cgoHandler.Delete()
// see: startHandler
conn := newVirtioSocketConnection(connPtr)
if err := newNSError(errPtr); err != nil {
handler(conn, err)
handler(nil, err)
} else {
handler(conn, nil)
conn, err := newVirtioSocketConnection(connPtr)
handler(conn, err)
}
}

Expand All @@ -133,12 +131,7 @@ type VirtioSocketListener struct {
pointer
}

type dup struct {
conn *VirtioSocketConnection
err error
}

var shouldAcceptNewConnectionHandlers = map[unsafe.Pointer]func(conn *VirtioSocketConnection) bool{}
var shouldAcceptNewConnectionHandlers = map[unsafe.Pointer]func(conn *VirtioSocketConnection, err error) bool{}

// NewVirtioSocketListener creates a new VirtioSocketListener with connection handler.
//
Expand All @@ -159,18 +152,8 @@ func NewVirtioSocketListener(handler func(conn *VirtioSocketConnection, err erro
},
}

dupCh := make(chan dup, 1)
go func() {
for dup := range dupCh {
go handler(dup.conn, dup.err)
}
}()
shouldAcceptNewConnectionHandlers[ptr] = func(conn *VirtioSocketConnection) bool {
dupConn, err := conn.dup()
dupCh <- dup{
conn: dupConn,
err: err,
}
shouldAcceptNewConnectionHandlers[ptr] = func(conn *VirtioSocketConnection, err error) bool {
go handler(conn, err)
return true // must be connected
}

Expand All @@ -185,8 +168,8 @@ func shouldAcceptNewConnectionHandler(listenerPtr, connPtr, devicePtr unsafe.Poi
_ = devicePtr // NOTO(codehex): Is this really required? How to use?

// see: startHandler
conn := newVirtioSocketConnection(connPtr)
return (C.bool)(shouldAcceptNewConnectionHandlers[listenerPtr](conn))
conn, err := newVirtioSocketConnection(connPtr)
return (C.bool)(shouldAcceptNewConnectionHandlers[listenerPtr](conn, err))
}

// VirtioSocketConnection is a port-based connection between the guest operating system and the host computer.
Expand All @@ -202,27 +185,21 @@ func shouldAcceptNewConnectionHandler(listenerPtr, connPtr, devicePtr unsafe.Poi
//
// see: https://developer.apple.com/documentation/virtualization/vzvirtiosocketconnection?language=objc
type VirtioSocketConnection struct {
sourcePort uint32
destinationPort uint32
fileDescriptor uintptr
file *os.File
laddr net.Addr // local
raddr net.Addr // remote
net.Conn
laddr *Addr // local
raddr *Addr // remote
}

var _ net.Conn = (*VirtioSocketConnection)(nil)

func newVirtioSocketConnection(ptr unsafe.Pointer) *VirtioSocketConnection {
func newVirtioSocketConnection(ptr unsafe.Pointer) (*VirtioSocketConnection, error) {
vzVirtioSocketConnection := C.convertVZVirtioSocketConnection2Flat(ptr)
err := unix.SetNonblock(int(vzVirtioSocketConnection.fileDescriptor), true)
file := os.NewFile((uintptr)(vzVirtioSocketConnection.fileDescriptor), "")
defer file.Close()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is unnecessary because of closed the file descriptor in the virtualization framework.

Suggested change
defer file.Close()

Copy link
Contributor Author

@cfergeau cfergeau Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't explicitly close the file, go garbage collector may do it for us: https://pkg.go.dev/os#File.Fd

If f is garbage collected, a finalizer may close the file descriptor, making it invalid;

This is problematic if the following sequence happens:

  • file := os.NewFile((uintptr)(vzVirtioSocketConnection.fileDescriptor), "")
  • virtualization framework closes vzVirtioSocketConnection.fileDescriptor
  • another parts of the code needs a file descriptor, the OS/system library decides to reuse the same fd as vzVirtioSocketConnection.fileDescriptor
  • the go garbage collector closes the file descriptor used by file, which is no longer the expected one and will cause unexpected issues

Copy link
Owner

@Code-Hex Code-Hex Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why needs to call close method from your comment.

I think garbage collection is unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://gist.githubusercontent.com/cfergeau/46a4a4fda70a83dbe2a9bf519cbabd28/raw/530c7d9a97e7a823fb324552bda470ad11fbbab9/fd.go
This shows the race which can happen when you call os.NewFile() on a file descriptor, and then other code closes the file descriptor.

By calling Close() in newVirtioSocketConnection, we know we are closing the right file descriptor. The garbage collector won't call Close() on it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If vzVirtioSocketConnection.fileDescriptor is owned by the virtualization framework, the code below would even be more correct. Since net.FileConn() will also Dup() the file descriptor, this feels a bit overkill :-/

diff --git a/socket.go b/socket.go
index 173cae6..5770618 100644
--- a/socket.go
+++ b/socket.go
@@ -12,6 +12,7 @@ import (
        "os"
        "runtime"
        "runtime/cgo"
+       "syscall"
        "unsafe"
 
        "golang.org/x/sys/unix"
@@ -192,7 +193,11 @@ type VirtioSocketConnection struct {
 
 func newVirtioSocketConnection(ptr unsafe.Pointer) (*VirtioSocketConnection, error) {
        vzVirtioSocketConnection := C.convertVZVirtioSocketConnection2Flat(ptr)
-       file := os.NewFile((uintptr)(vzVirtioSocketConnection.fileDescriptor), "")
+       fd, err := syscall.Dup(int(vzVirtioSocketConnection.fileDescriptor))
+       if err != nil {
+               return nil, err
+       }
+       file := os.NewFile(uintptr(fd), "")
        defer file.Close()
        rawConn, err := net.FileConn(file)
        if err != nil {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net.FileConn will dup file descriptor. https://pkg.go.dev/net#FileConn

FileConn returns a copy of the network connection corresponding to the open file f.

Copy link
Contributor Author

@cfergeau cfergeau Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I believe the original file descriptor should be closed in the framework (in Objective-C) instead of Go side.

The problem is that when os.NewFile(fd) is used, go will always call Close(fd). If our code don't call Close(), then the go runtime/go garbage collector will call Close() automatically. If the go garbage collector closes the file, we don't control when it will do so.

But I agree to call Close method for duplicated file descriptor in Go

Are you referring to net.FileConn(file) ? Or to another file descriptor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our code don't call Close(), then the go runtime/go garbage collector will call Close() automatically.

This is done here: https://github.com/golang/go/blob/4bcf94b0232db65ed5df47e0127cdbc8866aec64/src/os/file_unix.go#L196

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cfergeau I know the Go call Close method in finalizer. Bu the timing is different with using defer.

Yes. about net.FileConn which dup from original file descriptor.

Are you referring to net.FileConn(file) ? Or to another file descriptor?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind either now. Think about it after we have a problem.

rawConn, err := net.FileConn(file)
if err != nil {
fmt.Printf("set nonblock: %s\n", err.Error())
return nil, err
}
conn := &VirtioSocketConnection{
sourcePort: (uint32)(vzVirtioSocketConnection.sourcePort),
destinationPort: (uint32)(vzVirtioSocketConnection.destinationPort),
fileDescriptor: (uintptr)(vzVirtioSocketConnection.fileDescriptor),
file: os.NewFile((uintptr)(vzVirtioSocketConnection.fileDescriptor), ""),
Conn: rawConn,
laddr: &Addr{
CID: unix.VMADDR_CID_HOST,
Port: (uint32)(vzVirtioSocketConnection.destinationPort),
Expand All @@ -232,40 +209,7 @@ func newVirtioSocketConnection(ptr unsafe.Pointer) *VirtioSocketConnection {
Port: (uint32)(vzVirtioSocketConnection.sourcePort),
},
}
return conn
}

func (v *VirtioSocketConnection) dup() (*VirtioSocketConnection, error) {
nfd, err := syscall.Dup(int(v.fileDescriptor))
if err != nil {
return nil, &net.OpError{
Op: "dup",
Net: "vsock",
Source: v.laddr,
Addr: v.raddr,
Err: err,
}
}

dupConn := new(VirtioSocketConnection)
*dupConn = *v
dupConn.fileDescriptor = uintptr(nfd)
dupConn.file = os.NewFile(uintptr(nfd), v.file.Name())
dupConn.laddr = v.laddr
dupConn.raddr = v.raddr

return dupConn, nil
}

// Read reads data from connection of the vsock protocol.
func (v *VirtioSocketConnection) Read(b []byte) (n int, err error) { return v.file.Read(b) }

// Write writes data to the connection of the vsock protocol.
func (v *VirtioSocketConnection) Write(b []byte) (n int, err error) { return v.file.Write(b) }

// Close will be called when caused something error in socket.
func (v *VirtioSocketConnection) Close() error {
return v.file.Close()
return conn, nil
}

// LocalAddr returns the local network address.
Expand All @@ -274,44 +218,14 @@ func (v *VirtioSocketConnection) LocalAddr() net.Addr { return v.laddr }
// RemoteAddr returns the remote network address.
func (v *VirtioSocketConnection) RemoteAddr() net.Addr { return v.raddr }

// SetDeadline sets the read and write deadlines associated
// with the connection. It is equivalent to calling both
// SetReadDeadline and SetWriteDeadline.
func (v *VirtioSocketConnection) SetDeadline(t time.Time) error { return v.file.SetDeadline(t) }

// SetReadDeadline sets the deadline for future Read calls
// and any currently-blocked Read call.
// A zero value for t means Read will not time out.
func (v *VirtioSocketConnection) SetReadDeadline(t time.Time) error {
return v.file.SetReadDeadline(t)
}

// SetWriteDeadline sets the deadline for future Write calls
// and any currently-blocked Write call.
// Even if write times out, it may return n > 0, indicating that
// some of the data was successfully written.
// A zero value for t means Write will not time out.
func (v *VirtioSocketConnection) SetWriteDeadline(t time.Time) error {
return v.file.SetWriteDeadline(t)
}

// DestinationPort returns the destination port number of the connection.
func (v *VirtioSocketConnection) DestinationPort() uint32 {
return v.destinationPort
return v.laddr.Port
}

// SourcePort returns the source port number of the connection.
func (v *VirtioSocketConnection) SourcePort() uint32 {
return v.sourcePort
}

// FileDescriptor returns the file descriptor associated with the socket.
//
// Data is sent by writing to the file descriptor.
// Data is received by reading from the file descriptor.
// A file descriptor of -1 indicates a closed connection.
func (v *VirtioSocketConnection) FileDescriptor() uintptr {
return v.fileDescriptor
return v.raddr.Port
}

// Addr represents a network end point address for the vsock protocol.
Expand Down